> 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
> 
>

Reply via email to