aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Niemitz" <st...@tellapart.com>
Subject Re: Review Request 28920: Add support for docker containers to aurora
Date Thu, 08 Jan 2015 20:34:24 GMT


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


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