> On Jan. 14, 2015, 7:55 p.m., Bill Farner wrote:
> > All nits this round.  Last issue for me, as you point out, is 
> > validation/restriction of `ContainerType` in `ConfigurationManager.
> > 
> > Doing a pass on test coverage in the meantime.
> 
> Bill Farner wrote:
>     Test coverage LGTM when supplemented with 
> https://reviews.apache.org/r/29827/
> 
> Steve Niemitz wrote:
>     Is there any need to validate anymore now that the volume configuration 
> is gone?  That's what was going to be validated correct?
> 
> Bill Farner wrote:
>     I would like to see an argument to specify which `ContainerType`s the 
> scheduler should allow.  This way the scheduler will not fatefully accept 
> jobs and launch tasks that are not supported by the slave's command line and 
> environment.
> 
> Steve Niemitz wrote:
>     Do you have any ideas on how to plumb that into the ConfigurationManager? 
>  Since it's a static class.

I replied above with a suggestion.  Copying here to save you from excessive 
clicking in reviewboard:

> Do you have any thoughts on how to get the setting propegated into the 
> ConfigurationManager?

We have some precedent for defining them as static fields in classes like this, 
but have started to backpedal on that as it is hostile to unit testing.  
Perhaps you can create a ConfigurationSettings class, to be injected into 
SchedulerThriftInterface and passed to ConfigurationManager?  One thing to keep 
in mind - there are two primary entrypoints to ConfigurationManager - one for 
thrift, and the other for storage recovery.  The thrift entrypoint tends to do 
validation and storage recovery mostly does backfilling of fields, to make sure 
that we continue to run jobs we have previously accepted.


- Bill


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


On Jan. 14, 2015, 8:20 p.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 8:20 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 
>   config/legacy_untested_classes.txt 33c1d6eb4ea02e01b7002c2c2bae5a3858c8b0e5 
>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 1e31f21998d02fd69ce0db88e6adb3d32cff67fd 
>   examples/vagrant/provision-dev-cluster.sh 
> 7af4b52a6876268a97630279221bb98d9b04efad 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 788ec254270bca074dae91829c7f4fccdc8f8bb0 
>   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/configuration/ConfigurationManager.java
>  01b03508afac37b5a8f0ec5c3da1460695e1ef59 
>   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 
> f7d8977e42aa56188799400bf8e12a6886fb4a8f 
>   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/configuration/ConfigurationManagerTest.java
>  dc2cb37adf32df0a6e4c7ee2ba776ba9f1f3c2f8 
>   
> 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