aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 28920: Add support for docker containers to aurora
Date Thu, 08 Jan 2015 21:03:11 GMT


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java, line 109
> > <https://reviews.apache.org/r/28920/diff/7/?file=808132#file808132line109>
> >
> >     It's good form for this type of sanitization to happen here, but at minimum
the same sanitization must be done in `ConfigurationManager` to give the user a good message
and avoid accepting the bad config.
> 
> Steve Niemitz wrote:
>     I do a similar check in SchedulerMain.java, ~line 211.  Should I move the check into
ConfigurationManager?  ExecutorSettings.ctor checks as well.

Doh, i left this comment in the wrong place.  This should have been a request to sanitize
the incoming thrift fields.


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 207
> > <https://reviews.apache.org/r/28920/diff/7/?file=808124#file808124line207>
> >
> >     What are the implications of this?  Does it mean a docker image must be pre-loaded
on the host for it to be used?  Can we cope with the user specifying a bad path?  If not,
what's the fallout - TASK_FAILED?
> 
> Steve Niemitz wrote:
>     The Volume options are for mounting paths on the host into the docker container,
and correspond to the -v flag of docker run (https://docs.docker.com/userguide/dockervolumes/#mount-a-host-directory-as-a-data-volume).
 If the path is bad, it will fail but continue to run the container.  This is goverened by
mesos, and I actually have some plans to enhance their docker integration.

Awesome, please include that link here and in the .md.

Question remains about whether the image must be pre-loaded on the machine.


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 215
> > <https://reviews.apache.org/r/28920/diff/7/?file=808124#file808124line215>
> >
> >     Perhaps this should be called `imageName`?  Without help in the name, i could
be convinced this was a path or URI.
> >     
> >     That said, what is this for?  As a user, can i incorrectly specify/format this
string, or is it for my own purposes?
> 
> Steve Niemitz wrote:
>     I tried to avoid a name with "name" in it to avoid confusion with other "name" fields
that are purely for ID reasons (ok that sentence was a mouthful).  I can enhance the docs
though to be more clear that it expects a docker image name and not a URI/etc.
>     
>     This field is actually the docker container (image) that will be run.

Ah, in that case i agree.


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > docs/deploying-aurora-scheduler.md, line 153
> > <https://reviews.apache.org/r/28920/diff/7/?file=808126#file808126line153>
> >
> >     s/aurora/mesos/?  IIUC it's the slave that does the copy.
> 
> Steve Niemitz wrote:
>     how about s/aurora will/aurora will configure mesos to/?

+1


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > docs/deploying-aurora-scheduler.md, line 155
> > <https://reviews.apache.org/r/28920/diff/7/?file=808126#file808126line155>
> >
> >     From reading the code, it appears this additional path is needed for them both
to be available in the container.  If so, would it be easier to just accept an arbitrary number
of additional assets to copy into the sandbox?  I would find that more generalized, and easier
to understand.
> >     
> >     If you go with the above, i _think_ you can also safely nuke the extra args
plumbing.
> 
> Steve Niemitz wrote:
>     I think we'll still need extra args (its really useful right now to pass ZK config
in), but I like the idea of an arbitrary set of resources.  The only trouble here would be
figuring out which is the one to run and getting the symlinks right.  Let's talk about this
more.

Wouldn't the extra args just be solved with the shim script?  I tell the scheduler to copy
`my_executor.sh` and `executor.pex` into the container, and `my_executor.sh` contains the
extra args.  I like this as a generalized solution to augmenting default executor behavior,
since it avoids feature creep on our side just to save people the shim script.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28920/#review67226
-----------------------------------------------------------


On Jan. 8, 2015, 8:35 p.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2015, 8:35 p.m.)
> 
> 
> Review request for Aurora, Jay Buffington, Kevin Sweeney, and Bill Farner.
> 
> 
> Bugs: AURORA-633
>     https://issues.apache.org/jira/browse/AURORA-633
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change adds support for launching docker containers through aurora.  These changes
are based off of the discussion in https://issues.apache.org/jira/browse/AURORA-633
> 
> As of now, a special thermos_executor.sh script is needed to launch the executor inside
docker containers.  A sample aurora file is in examples/jobs/docker.
> 
> In addition, mesos-slave must be run with `--containerizers=docker,mesos`, the example
upstart config in examples/vagrant/upstart has been updated to reflect this.
> 
> More information is in subsequent review request comments.
> 
> 
> Diffs
> -----
> 
>   Vagrantfile f8b7db8eebdc6a10989de3bc9a2c3e89ce17f5fc 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 08ba1cdf88b712de22c26c04443079282db59ef9

>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh b7ea41719a8f41bb23d0254e732926d89399c77c 
>   examples/vagrant/upstart/mesos-slave.conf 512ce7ecf34042ed68dda55efb2dd0415f8469db

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 72c7545e7f16549f6a9ccb5fb74a06f154a7ea94

>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 5226e3d1b303b1773a057078f2911c5ec2aa97f5

>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java d885b224ec5a1d529347d84e03ba98ab6734a126

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5bf283062c9d119ff91ed45da8b236e36d0fc9aa

>   src/main/python/apache/aurora/config/thrift.py ba94ac3c0cbaf3c91eb1a1d86a244ed6fa3b649c

>   src/main/python/apache/aurora/executor/aurora_executor.py 636b23d30a897b557eb8c3f8733c90b23cb807ef

>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 9df9b4b79c0c7d29c5088409bf15c0d32a621df0

>   src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12

>   src/main/python/apache/aurora/executor/thermos_task_runner.py 5e4bd65537d186459003c0b9434f1b769e04f448

>   src/main/python/apache/thermos/config/schema_base.py f9143cc1b83143d6147f59d90c79435d055d0518

>   src/main/python/apache/thermos/core/runner.py 8aac6b50c66080abbb5308b367e9f74c487f42e3

>   src/main/resources/scheduler/assets/configSummary.html 28878908b0c9381e366b71a3135dfc28c542a890

>   src/main/resources/scheduler/assets/js/services.js b744f375411e09b7f776e4a05ee5961227143439

>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 5e54364a49a208bd5f19b9649633dc8feca591e9

>   src/test/java/org/apache/aurora/scheduler/base/CommandUtilTest.java 876e173ccbac04e4a06a245648c7c6af15eaaa92

>   src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java ddcb511d108220ab5e4efcf3496458f7ab4a20c2

>   src/test/python/apache/aurora/executor/test_thermos_executor.py 503e62f4cac872b14f6985b5bccc3e4dfcf81789

> 
> Diff: https://reviews.apache.org/r/28920/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message