Support Images with Entrypoint defined

[jschneiderhan]

I'm having trouble using a container with an entrypoint defined because instead of passing the arguments as is, /bin/sh -c is being added to the cmd.

In my particular case, I am trying to run an image based off (flynn/slugrunner)[https://github.com/flynn/slugrunner/blob/master/Dockerfile]:

FROM progrium/cedarish
MAINTAINER Jonathan Rudenberg <jonathan@titanous.com>

ADD ./runner/ /runner
ADD ./build/sdutil /bin/sdutil
ENTRYPOINT ["/runner/init"]

So if I wanted to run this container, I could specify something like the following:

docker run -e SLUG_URL=http://example.com/slug.tgz flynn/slugrunner start web

... which would end up calling something like this inside the container:

/runner/init start web

When /bin/sh -c is added to the cmd parameters passed to marathon, the container ends up trying to execute:

/runner/init /bin/sh -c 'start web'

[jschneiderhan]

What are the main motivations for adding '/bin/sh -c' to the front of commands? I want to make sure I keep them in mind as I brainstorm ideas to propose.

[solidsnack]

This has to do with the semantics of Mesos's CommandInfo -- the command.value field is meant to be interpreted by the shell and is a single string.

Maybe it would work if the options field were processed in a special way: if // is present, we take all following arguments and move them to the end of the run command. They would be treated as strings not subject to shell interpretation in any way (so $TZ would be treated as the literal string, not expanded to the host's timezone).

[jschneiderhan]

@solidsnack oh wow - the functionality has been added already! Thank you very much! I'll give the updated code a shot today. This solution will definitely fulfill my use-case.

[jschneiderhan]

Sorry, just realized the branch hasn't been merged yet.

[solidsnack]

Please do ping us back and let us know how it goes.

[jschneiderhan]

@solidsnack it worked great!

I was getting the following error:

Jul  8 13:50:52 vagrant deimos[22122]: deimos.cli() Unhandled failure in launch#012Traceback (most recent call last):#012  File "/home/vagrant/deimos/deimos/__init__.py", line 72, in cli#012    result = deimos.containerizer.stdio(containerizer, *argv[1:])#012  File "/home/vagrant/deimos/deimos/containerizer/__init__.py", line 97, in stdio#012    return method(recordio.read(proto), *args[1:])#012  File "/home/vagrant/deimos/deimos/containerizer/docker.py", line 73, in launch#012    options, trailing_argv = split_on(options, "//")#012  File "/home/vagrant/deimos/deimos/containerizer/docker.py", line 337, in split_on#012    preceding = list(takewhile(lambda _: _ != element, iterable))#012NameError: global name 'takewhile' is not defined

To fix it, I added the following line to the top of deimos/containerizer/docker.py:

from itertools import takewhile, dropwhile

I'm not sure if that is a valid addition to the code base or an environment problem on my end (I know next to nothing about python).

[solidsnack]

Oops. You did the right thing. Not sure how it got committed this way.

[jschneiderhan]

Ok good. This solution definitely works for my use case. Thanks for adding the functionality!

[solidsnack]

If you make a pull request against https://github.com/mesosphere/deimos/tree/argv-in-options containing the itertools change I would be happy to merge it in and credit you.

[jschneiderhan]

@solidsnack Thanks! PR submitted https://github.com/mesosphere/deimos/pull/33

[jschneiderhan]

Awesome! I think this is all set, right? I'm assuming I should close the issue since the use case has been satisfied. Thanks for everything @solidsnack!

For anyone who stumbles upon this with a similar need, my REST call to marathon looks like the following and it works like a charm:

{
  "container": {
    "image": "docker:///flynn/slugrunner",
    "options": ["//", "start", "web"]
  },
  "id": "ubuntu",
  "instances": "1",
  "cpus": ".5",
  "mem": "512",
  "cmd": "",
  "env": {
    "SLUG_URL": "https://example.com/myappslug.tgz",
    "PORT": "8080",
    "APPVAR1": "value1",
    "APPVAR2": "value2",
    "APPVAR3": "value3"
  }
}

[jplock]

Can we add this to the formal documentation as well? This is very useful and I'd hate to see this get lost in an issue.

[jschneiderhan]

@jplock I added a few lines as a starting point https://github.com/mesosphere/deimos/pull/35

[solidsnack]

@jschneiderhan With a small change I can merge that. Please see line notes.

[jschneiderhan]

Cool. I'll let you guys determine if the documentation requirement is fulfilled and close the issue.

Thanks again!

[solidsnack]

Thanks for your patch.

[dangra]

hey guys, just come up with a complete different solution to the same issue a day ago. I didn't know you were after the same problem, sorry for not sharing early, I want to share my solution in case someone else find it useful: https://github.com/scrapinghub/deimos/commit/63329aca575e3ca9a0977acfbb3cbb1b425e1c75

The main advantage is that it can be used from frameworks that doesn't support the "container" field yet, like Chronos. The only drawback is adding "json" to the equation, but that is fine in my case.

thanks. great work!