Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 3, 2015, 3:21 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift a5198dfe1c56e05d50e32dd14222a984515c0d07 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 3239eaa139e35e8c3acdacf6375f492de2b5bfee src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review75000 --- @ReviewBot retry - Bill Farner On March 3, 2015, 3:23 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 3, 2015, 3:23 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift a5198dfe1c56e05d50e32dd14222a984515c0d07 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 3239eaa139e35e8c3acdacf6375f492de2b5bfee src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On March 3, 2015, 6:28 p.m., Aurora ReviewBot wrote: Master (782e3e7) is red with this patch. ./build-support/jenkins/build.sh src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.cli.version . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.factory . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api . SUCCESS src.test.python.apache.aurora.common.test_aurora_job_key . SUCCESS src.test.python.apache.aurora.common.test_cluster . SUCCESS src.test.python.apache.aurora.common.test_cluster_option . SUCCESS src.test.python.apache.aurora.common.test_clusters . SUCCESS src.test.python.apache.aurora.common.test_http_signaler . SUCCESS src.test.python.apache.aurora.common.test_pex_version . SUCCESS src.test.python.apache.aurora.common.test_shellify . SUCCESS src.test.python.apache.aurora.common.test_transport . SUCCESS src.test.python.apache.aurora.config.test_base . SUCCESS src.test.python.apache.aurora.config.test_constraint_parsing . SUCCESS src.test.python.apache.aurora.config.test_loader . SUCCESS src.test.python.apache.aurora.config.test_thrift . SUCCESS src.test.python.apache.aurora.executor.common.announcer . SUCCESS src.test.python.apache.aurora.executor.common.directory_sandbox . SUCCESS src.test.python.apache.aurora.executor.common.executor_detector . SUCCESS src.test.python.apache.aurora.executor.common.executor_timeout . SUCCESS src.test.python.apache.aurora.executor.common.health_checker . SUCCESS src.test.python.apache.aurora.executor.common.kill_manager . SUCCESS src.test.python.apache.aurora.executor.common.path_detector . SUCCESS src.test.python.apache.aurora.executor.common.status_checker . SUCCESS src.test.python.apache.aurora.executor.common.task_info . SUCCESS src.test.python.apache.aurora.executor.executor_base . SUCCESS src.test.python.apache.aurora.executor.executor_vars . SUCCESS src.test.python.apache.aurora.executor.gc_executor . FAILURE src.test.python.apache.aurora.executor.status_manager . SUCCESS src.test.python.apache.aurora.executor.thermos_task_runner . SUCCESS src.test.python.apache.thermos.common.test_pathspec . SUCCESS src.test.python.apache.thermos.core.test_runner_integration . SUCCESS src.test.python.apache.thermos.monitoring.test_disk . SUCCESS FAILURE [31m FAILURE[0m I will refresh this build result if you post a review containing @ReviewBot retry Whack-a-mole https://issues.apache.org/jira/browse/AURORA-1162 - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review75003 --- On March 3, 2015, 3:23 p.m., Steve Niemitz wrote:
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review75005 --- @ReviewBot retry - Bill Farner On March 3, 2015, 3:23 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 3, 2015, 3:23 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift a5198dfe1c56e05d50e32dd14222a984515c0d07 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 3239eaa139e35e8c3acdacf6375f492de2b5bfee src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review75050 --- Ship it! Master (782e3e7) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 3, 2015, 3:23 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 3, 2015, 3:23 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift a5198dfe1c56e05d50e32dd14222a984515c0d07 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 3239eaa139e35e8c3acdacf6375f492de2b5bfee src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 9:03 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On March 2, 2015, 6:56 p.m., Bill Farner wrote: src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java, line 133 https://reviews.apache.org/r/31338/diff/8/?file=882131#file882131line133 I think you can drop this since it's == `Resources.NONE` (default). Done On March 2, 2015, 6:56 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java, line 137 https://reviews.apache.org/r/31338/diff/8/?file=882134#file882134line137 remove empty line Done On March 2, 2015, 6:56 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, lines 216-222 https://reviews.apache.org/r/31338/diff/8/?file=882138#file882138line216 This can collapse to: ``` assertTrue(taskInfo.getExecutor().getContainer().getVolumesList().contains(expected)); ``` Done - Steve --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74782 --- On March 2, 2015, 4:59 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 4:59 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74821 --- This patch does not apply cleanly on master (4cca6a6), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 2, 2015, 9:03 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 9:03 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74752 --- @ReviewBot retry - Steve Niemitz On March 2, 2015, 4:59 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 4:59 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74751 --- Master (4cca6a6) is red with this patch. ./build-support/jenkins/build.sh src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.cli.version . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.factory . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api . SUCCESS src.test.python.apache.aurora.common.test_aurora_job_key . SUCCESS src.test.python.apache.aurora.common.test_cluster . SUCCESS src.test.python.apache.aurora.common.test_cluster_option . SUCCESS src.test.python.apache.aurora.common.test_clusters . SUCCESS src.test.python.apache.aurora.common.test_http_signaler . SUCCESS src.test.python.apache.aurora.common.test_pex_version . SUCCESS src.test.python.apache.aurora.common.test_shellify . SUCCESS src.test.python.apache.aurora.common.test_transport . SUCCESS src.test.python.apache.aurora.config.test_base . SUCCESS src.test.python.apache.aurora.config.test_constraint_parsing . SUCCESS src.test.python.apache.aurora.config.test_loader . SUCCESS src.test.python.apache.aurora.config.test_thrift . SUCCESS src.test.python.apache.aurora.executor.common.announcer . SUCCESS src.test.python.apache.aurora.executor.common.directory_sandbox . SUCCESS src.test.python.apache.aurora.executor.common.executor_detector . SUCCESS src.test.python.apache.aurora.executor.common.executor_timeout . SUCCESS src.test.python.apache.aurora.executor.common.health_checker . FAILURE src.test.python.apache.aurora.executor.common.path_detector . SUCCESS src.test.python.apache.aurora.executor.common.task_info . SUCCESS src.test.python.apache.aurora.executor.executor_base . SUCCESS src.test.python.apache.aurora.executor.executor_vars . SUCCESS src.test.python.apache.aurora.executor.status_manager . SUCCESS src.test.python.apache.aurora.executor.thermos_task_runner . SUCCESS src.test.python.apache.thermos.common.test_pathspec . SUCCESS src.test.python.apache.thermos.core.test_runner_integration . SUCCESS src.test.python.apache.thermos.monitoring.test_disk . SUCCESS FAILURE [31m FAILURE[0m I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 2, 2015, 4:59 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 4:59 p.m.) Review request for Aurora, Jay Buffington and Bill Farner.
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 27, 2015, 5:01 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, lines 215-227 https://reviews.apache.org/r/31338/diff/4/?file=876499#file876499line215 Can this instead be an exact comparison of ListVolume? Presumably we should fail if other volumes show up. ``` assertEquals( ImmutableList.of(expected), taskInfo.getExecutor().getContainer().getVolumesList()); ``` Steve Niemitz wrote: I think checking only this specifically is a more robust test. There are other things we may want to mount in by default in the future (for example, we already mount the thermos run directory), and testing for all of them here would make the test to broad. Bill Farner wrote: Hmm...i see your point, but i don't entirely buy it :-) For example, if the code behind this erroneously inserts two `Protos.Volume` entries for each global mount, the test case should trip. Steve Niemitz wrote: imo it's more likely that we'll be adding more features around volumes (default or otherwise) than introducing breaking changes to the code that adds the default volumes. Unit tests that are too fragile I think cause more harm than good. Alrighty, that works for me! - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74507 --- On March 2, 2015, 4:59 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 4:59 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74782 --- Ship it! LGTM mod nits. src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java https://reviews.apache.org/r/31338/#comment121541 I think you can drop this since it's == `Resources.NONE` (default). src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java https://reviews.apache.org/r/31338/#comment121540 remove empty line src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java https://reviews.apache.org/r/31338/#comment121537 This can collapse to: ``` assertTrue(taskInfo.getExecutor().getContainer().getVolumesList().contains(expected)); ``` - Bill Farner On March 2, 2015, 4:59 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated March 2, 2015, 4:59 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74507 --- Overall LGTM, all minor issues. api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/31338/#comment121085 The mode for a volume mount api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/31338/#comment121086 s/Describes a/A/ api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/31338/#comment121087 s/required // src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java https://reviews.apache.org/r/31338/#comment121088 Thanks! This reads much better. src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java https://reviews.apache.org/r/31338/#comment121089 Consider making the 'empty' values defaults in the builder. src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java https://reviews.apache.org/r/31338/#comment121090 EnumSet.allOf(Mode.class) ``` src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java https://reviews.apache.org/r/31338/#comment121091 should this be private? src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java https://reviews.apache.org/r/31338/#comment121092 remove extra newline src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java https://reviews.apache.org/r/31338/#comment121094 s/vp/parser/ src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java https://reviews.apache.org/r/31338/#comment121093 Consider inlining. ``` assertEquals( new Volume(...), parser.doParse(...)); ``` src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java https://reviews.apache.org/r/31338/#comment121095 Can this instead be an exact comparison of ListVolume? Presumably we should fail if other volumes show up. ``` assertEquals( ImmutableList.of(expected), taskInfo.getExecutor().getContainer().getVolumesList()); ``` - Bill Farner On Feb. 25, 2015, 10:35 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 10:35 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 27, 2015, 8:19 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 27, 2015, 5:01 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java, line 36 https://reviews.apache.org/r/31338/diff/4/?file=876495#file876495line36 should this be private? I wanted it to be, but got style checker violations when making it private. Same reason the Builder ctor is public as well. On Feb. 27, 2015, 5:01 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, lines 215-227 https://reviews.apache.org/r/31338/diff/4/?file=876499#file876499line215 Can this instead be an exact comparison of ListVolume? Presumably we should fail if other volumes show up. ``` assertEquals( ImmutableList.of(expected), taskInfo.getExecutor().getContainer().getVolumesList()); ``` I think checking only this specifically is a more robust test. There are other things we may want to mount in by default in the future (for example, we already mount the thermos run directory), and testing for all of them here would make the test to broad. - Steve --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74507 --- On Feb. 25, 2015, 10:35 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 10:35 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74627 --- Master (4cca6a6) is red with this patch. ./build-support/jenkins/build.sh make: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift' :api:classesThriftNote: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. :api:checkPython :api:generateThriftEntitiesJava :api:classesThriftEntities :api:compileJava UP-TO-DATE :api:generateThriftResources :api:processResources UP-TO-DATE :api:classes :api:jar :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2 :processResources :classes :jar :assemble :compileJmhJava :processJmhResources UP-TO-DATE :jmhClasses :checkstyleJmh :jsHint :checkstyleMain[ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java:16:8: Unused import - java.util.ArrayList. [ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java:17:8: Unused import - java.util.List. [ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java:21: 'org.apache.aurora.gen.Volume' should be separated from previous imports. FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':checkstyleMain'. Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.xml * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 38.789 secs I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 27, 2015, 11:16 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 27, 2015, 11:16 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 27, 2015, 5:01 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, lines 215-227 https://reviews.apache.org/r/31338/diff/4/?file=876499#file876499line215 Can this instead be an exact comparison of ListVolume? Presumably we should fail if other volumes show up. ``` assertEquals( ImmutableList.of(expected), taskInfo.getExecutor().getContainer().getVolumesList()); ``` Steve Niemitz wrote: I think checking only this specifically is a more robust test. There are other things we may want to mount in by default in the future (for example, we already mount the thermos run directory), and testing for all of them here would make the test to broad. Hmm...i see your point, but i don't entirely buy it :-) For example, if the code behind this erroneously inserts two `Protos.Volume` entries for each global mount, the test case should trip. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74507 --- On Feb. 27, 2015, 11:16 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 27, 2015, 11:16 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 27, 2015, 11:16 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74103 --- lgtm minus a few nits... src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java https://reviews.apache.org/r/31338/#comment120622 this should be unnecessary, we will have thrown on the previous line? Also can you add a test for too many parts as well (e.g. foo:bar:baz:quux)? src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java https://reviews.apache.org/r/31338/#comment120623 Again, should be unnecessary. src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java https://reviews.apache.org/r/31338/#comment120624 move to next line. Also is this potentially fragile to assume the global mount will be the second mount in the list? It'd be slightly more robust to iterate the mounts and ensure the mount we expect is *somewhere* in the list? - Joshua Cohen On Feb. 25, 2015, 7:15 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 7:15 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 25, 2015, 10:06 p.m., Joshua Cohen wrote: src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java, lines 44-45 https://reviews.apache.org/r/31338/diff/2/?file=876074#file876074line44 this should be unnecessary, we will have thrown on the previous line? Also can you add a test for too many parts as well (e.g. foo:bar:baz:quux)? I added these in to stop some complaints about unused variables, but in retrospect I could have just called vp.doParse(...) and not assigned it. I'll change these. On Feb. 25, 2015, 10:06 p.m., Joshua Cohen wrote: src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, line 214 https://reviews.apache.org/r/31338/diff/2/?file=876075#file876075line214 move to next line. Also is this potentially fragile to assume the global mount will be the second mount in the list? It'd be slightly more robust to iterate the mounts and ensure the mount we expect is *somewhere* in the list? Good point on the ordering, it shouldn't matter. I'll change it. - Steve --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74103 --- On Feb. 25, 2015, 7:15 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 7:15 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review74154 --- Ship it! Master (9442e08) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 25, 2015, 10:35 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 10:35 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 25, 2015, 10:35 p.m.) Review request for Aurora, Jay Buffington and Bill Farner. Changes --- That's what I get for not running tests on trivial changes... Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 6f6124a4c844a1abcf07401d80c3e50eb8b4 config/checkstyle/checkstyle.xml 580d9d3737c0f52da2d33f5b99ef9406b55c5807 docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: Steve Niemitz wrote: I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings. That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future. Re: command line, I chose the format there because it's the same as the docker command line. What about making a new parser to parse that into the above mentioned thrift objects? Stephan Erb wrote: How about using the same format and argument name as Mesos' `--default_container_info`: { type: MESOS, volumes: [ { host_path: ./.private/tmp, container_path: /tmp, mode: RW } ] } See https://mesos.apache.org/documentation/latest/configuration/ Steve Niemitz wrote: JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case. Bill Farner wrote: JSON on the command line also makes me uneasy, but i don't think we have precedent for command line entities with this many knobs (3 are obvious so far). How about 2 approaches as straw men: `-volume_mounts_read_only` and `-volume_mounts_read_write`, both key-value mappings `-volume_mounts_config`, path to a JSON file with entities similar to what Stephan sketched above The first approach has the upside that it fits with the status quo, but it cannot be easily extended if we ever want more attributes. The second would be the first config file we use, but is more future-proof. Joshua Cohen wrote: I think a single arg is easiest to reason about. I think I'm ok with Steve's follow up suggestion to create the thrift types that will be needed for per-job mounts now and use them to aid in arg parsing? That said, I'm not sure we should enforce the docker mount format since in theory these mounts can/will apply to other containers where they may not make as much sense. Steve Niemitz wrote: I think it's simple enough it'd make sense for any container system. host:container:mode is pretty generic. I'm also not as big of a fan about multiple args, and a default of RO if not specified seems reasonable to me. Works for me, but -1 to defaulting. I much prefer explicit over implicit, and this way it's easy to see how to turn a read-only mount into read-write. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73755 --- On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: Steve Niemitz wrote: I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings. That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future. Re: command line, I chose the format there because it's the same as the docker command line. What about making a new parser to parse that into the above mentioned thrift objects? Stephan Erb wrote: How about using the same format and argument name as Mesos' `--default_container_info`: { type: MESOS, volumes: [ { host_path: ./.private/tmp, container_path: /tmp, mode: RW } ] } See https://mesos.apache.org/documentation/latest/configuration/ JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case. - Steve --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73755 --- On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73822 --- src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java https://reviews.apache.org/r/31338/#comment120230 This code will also work for a default Mesos container on a slave with the `filesystem/shared` isolator. Is it worth to generalize here and make mounting work for docker and non-docker containers? - Stephan Erb On Feb. 24, 2015, 3:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 3:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: Steve Niemitz wrote: I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings. That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future. Re: command line, I chose the format there because it's the same as the docker command line. What about making a new parser to parse that into the above mentioned thrift objects? Stephan Erb wrote: How about using the same format and argument name as Mesos' `--default_container_info`: { type: MESOS, volumes: [ { host_path: ./.private/tmp, container_path: /tmp, mode: RW } ] } See https://mesos.apache.org/documentation/latest/configuration/ Steve Niemitz wrote: JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case. Bill Farner wrote: JSON on the command line also makes me uneasy, but i don't think we have precedent for command line entities with this many knobs (3 are obvious so far). How about 2 approaches as straw men: `-volume_mounts_read_only` and `-volume_mounts_read_write`, both key-value mappings `-volume_mounts_config`, path to a JSON file with entities similar to what Stephan sketched above The first approach has the upside that it fits with the status quo, but it cannot be easily extended if we ever want more attributes. The second would be the first config file we use, but is more future-proof. I think a single arg is easiest to reason about. I think I'm ok with Steve's follow up suggestion to create the thrift types that will be needed for per-job mounts now and use them to aid in arg parsing? That said, I'm not sure we should enforce the docker mount format since in theory these mounts can/will apply to other containers where they may not make as much sense. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73755 --- On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: Steve Niemitz wrote: I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings. That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future. Re: command line, I chose the format there because it's the same as the docker command line. What about making a new parser to parse that into the above mentioned thrift objects? Stephan Erb wrote: How about using the same format and argument name as Mesos' `--default_container_info`: { type: MESOS, volumes: [ { host_path: ./.private/tmp, container_path: /tmp, mode: RW } ] } See https://mesos.apache.org/documentation/latest/configuration/ Steve Niemitz wrote: JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case. JSON on the command line also makes me uneasy, but i don't think we have precedent for command line entities with this many knobs (3 are obvious so far). How about 2 approaches as straw men: `-volume_mounts_read_only` and `-volume_mounts_read_write`, both key-value mappings `-volume_mounts_config`, path to a JSON file with entities similar to what Stephan sketched above The first approach has the upside that it fits with the status quo, but it cannot be easily extended if we ever want more attributes. The second would be the first config file we use, but is more future-proof. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73755 --- On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: Steve Niemitz wrote: I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings. That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future. Re: command line, I chose the format there because it's the same as the docker command line. What about making a new parser to parse that into the above mentioned thrift objects? Stephan Erb wrote: How about using the same format and argument name as Mesos' `--default_container_info`: { type: MESOS, volumes: [ { host_path: ./.private/tmp, container_path: /tmp, mode: RW } ] } See https://mesos.apache.org/documentation/latest/configuration/ Steve Niemitz wrote: JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case. Bill Farner wrote: JSON on the command line also makes me uneasy, but i don't think we have precedent for command line entities with this many knobs (3 are obvious so far). How about 2 approaches as straw men: `-volume_mounts_read_only` and `-volume_mounts_read_write`, both key-value mappings `-volume_mounts_config`, path to a JSON file with entities similar to what Stephan sketched above The first approach has the upside that it fits with the status quo, but it cannot be easily extended if we ever want more attributes. The second would be the first config file we use, but is more future-proof. Joshua Cohen wrote: I think a single arg is easiest to reason about. I think I'm ok with Steve's follow up suggestion to create the thrift types that will be needed for per-job mounts now and use them to aid in arg parsing? That said, I'm not sure we should enforce the docker mount format since in theory these mounts can/will apply to other containers where they may not make as much sense. Steve Niemitz wrote: I think it's simple enough it'd make sense for any container system. host:container:mode is pretty generic. I'm also not as big of a fan about multiple args, and a default of RO if not specified seems reasonable to me. Bill Farner wrote: Works for me, but -1 to defaulting. I much prefer explicit over implicit, and this way it's easy to see how to turn a read-only mount into read-write. Cool with me. Ok I think we're all on the same page here. I'll submit an updated patch in a little bit. - Steve --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73755 --- On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/
Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73744 --- Ship it! Master (19378c1) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/ --- (Updated Feb. 24, 2015, 2:07 a.m.) Review request for Aurora, Jay Buffington and Bill Farner. Bugs: AURORA-1107 https://issues.apache.org/jira/browse/AURORA-1107 Repository: aurora Description --- Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run. This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it. I'll implement per-job mounts in a future review. Diffs - docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc Diff: https://reviews.apache.org/r/31338/diff/ Testing --- Thanks, Steve Niemitz