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 Mon, 05 Jan 2015 21:53:39 GMT


> On Jan. 5, 2015, 8:41 p.m., Joshua Cohen wrote:
> > This is 99% just style nits. Unfortunately our Java styleguide isn't published,
but I'm working on rectifying that!

Thanks for all the feedback, new patch set coming up!


> On Jan. 5, 2015, 8:41 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines 223-224
> > <https://reviews.apache.org/r/28920/diff/4/?file=801606#file801606line223>
> >
> >     Do we need to plumb the wrapper path all the way through to the CommandUtil?
What if instead of passing both along we figure out the correct path to use here at start
up and passed that along directly?
> >     
> >     I.e. we could do something like...
> >     
> >        Optional<String> executorPath = Optional.of(THERMOS_EXECUTOR_PATH.get()).or(Optional.of(THERMOS_EXECUTOR_WRAPPER_PATH.get()));
> >        
> >        if (!executorPath.isPresent()) {
> >          throw new IllegalStateException(...);
> >        }
> >        
> >        bind(ExecutorSettings.class).toInstance(new ExecutorSettings(
> >            executorPath.get(),
> >            ...));

We need both paths in the ExecutorSettings so they can both get added to the CommandInfo as
resources (for docker).


> On Jan. 5, 2015, 8:41 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/executor/test_thermos_executor.py, line 193
> > <https://reviews.apache.org/r/28920/diff/4/?file=801621#file801621line193>
> >
> >     This seems like a change in semantics... the sandbox_provider previously was
expected to be a factory function that returned the sandbox, now you're passing in the sandbox
itself? Why the change?

The UserOverrideDirectorySandboxProvider has a constructor which takes arguments, so the provider
can no longer be parameterless.  I feel like this is more true to a factory pattern anyways,
instead of passing in a "factory factory" we're now just passing in a factory.


- Steve


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


On Jan. 5, 2015, 8:25 p.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2015, 8:25 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/async/TaskScheduler.java ead9d28100673440168a32d114ecaa15874978a6

>   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