----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28920/#review68845 -----------------------------------------------------------
Sorry for the delay in reply. I've been thinking a lot about this patch and it's really looking good. My comments are focused mostly on backwards-compatibility as well as the desire to not favor Docker over other future containerizers Mesos may add. docs/deploying-aurora-scheduler.md <https://reviews.apache.org/r/28920/#comment113341> Latest version doesn't require a wrapper script, update docs to reflect? docs/deploying-aurora-scheduler.md <https://reviews.apache.org/r/28920/#comment113339> Philosophical question: if there's already a hard requirement that the container have Python 2.7 why not require that the executor be baked in as well? Maybe it's worth calling out as a TODO, but you don't have to answer it now. examples/vagrant/provision-dev-cluster.sh <https://reviews.apache.org/r/28920/#comment113340> nit: sh -c indirection is unnecessary here. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java <https://reviews.apache.org/r/28920/#comment113726> I don't see a reason the executor can't do this itself, by reading the Container field in AssignedTask and environment variables. I'd prefer not to introduce a new channel to send configuration from the scheduler to the executor with this review. @wickman might be better able to answer whether this is feasible. src/main/python/apache/thermos/config/schema_base.py <https://reviews.apache.org/r/28920/#comment113732> Can we match the union-like behavior the IDL defines here, something like: ```py class Container(Struct): docker = Docker class Docker(Struct): image = Required(String) ``` This will be more easily extensible to more container types without requiring backwards-incompatible changes. src/main/python/apache/thermos/core/runner.py <https://reviews.apache.org/r/28920/#comment113733> Naive question: since we have this block here can we drop the preamble from the scheduler? - Kevin Sweeney On Jan. 15, 2015, 4:08 p.m., Steve Niemitz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28920/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2015, 4:08 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/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java > 7eafe074b686d55ad96018006cf4acfa823513c3 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > ad9126c32893080e128d086ea3bfd7ad23d27b89 > src/test/python/apache/aurora/client/cli/test_status.py > e531fa06e508d9792af51c62e67120c21baa7a81 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 503e62f4cac872b14f6985b5bccc3e4dfcf81789 > > Diff: https://reviews.apache.org/r/28920/diff/ > > > Testing > ------- > > > Thanks, > > Steve Niemitz > >
