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

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.


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

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.


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > docs/deploying-aurora-scheduler.md, line 148
> > <https://reviews.apache.org/r/28920/diff/7/?file=808126#file808126line148>
> >
> >     It would be really useful to see a working example of these 
> > configuration parameters in concert.  I suggest you go ahead and wire it up 
> > in the upstart configs we have, and link to them from this doc.
> >     
> >     Taking it a step further, it would be awesome if this was exercised in 
> > `./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh`.

Totally agree, I actually have a jira ticket (in our own jira) to do just this. 
 I assume you mean the vagrant upstart configs?


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

how about s/aurora will/aurora will configure mesos to/?


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

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.


> On Jan. 8, 2015, 6:48 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java, line 108
> > <https://reviews.apache.org/r/28920/diff/7/?file=808132#file808132line108>
> >
> >     I applied your patch and removed these PMD exclusions without any 
> > issue.  Are they needed?
> >     
> >     As a general practice - we avoid decorating the code with hints to code 
> > quality checkers.  This could vary from making the code appease the checker 
> > to disabling the rule.

Ah, so this used to be a != null check that PMD complained about, but since I 
refactored it recently to use Optional PMD stopped complaining.  I'll remove 
these hits.


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

I do a similar check in SchedulerMain.java, ~line 211.  Should I move the check 
into ConfigurationManager?  ExecutorSettings.ctor checks as well.


- Steve


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


On Jan. 6, 2015, 11:32 p.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 11:32 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 
> 5665c69cd7b49c3fd7345074c9f16a3b224496ab 
>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 69983d0140b76c6869cd04e55d760f3e3a1e4262 
>   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