----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/#review144324 -----------------------------------------------------------
Looks good to me in general. A couple of general remarks below. src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (lines 98 - 102) <https://reviews.apache.org/r/47853/#comment210349> Looks like Mesos is able to compute a default value for this using some makefile constants: https://github.com/apache/mesos/blob/e8ebbe5fe4189ef7ab046da2276a6abee41deeb2/src/slave/flags.cpp#L193-L198 We should at least document that the expected value here is based on the one passed to Mesos. src/main/python/apache/aurora/executor/common/sandbox.py (lines 189 - 198) <https://reviews.apache.org/r/47853/#comment210345> Those two calls have the implicit assumptions: * the role user does not exist within container. * uid and gid as available on the host do not collide with pre-existing ones in the container The first point could be problematic for people migrating from the existing Docker support in Aurora, as it requires images with the pre-existing users. src/main/python/apache/aurora/executor/thermos_task_runner.py (lines 268 - 270) <https://reviews.apache.org/r/47853/#comment210348> This code is risky to operator error: If the operator forgets to set the optional containerizer path, users can launch container images whose filesystem isolation will not be applied. I would prefer if we throw an error (here, or at another appropriate place) if we are supposed to use a filesystem image without a containerizer path. src/main/python/apache/thermos/core/process.py (lines 188 - 190) <https://reviews.apache.org/r/47853/#comment210346> See my comment above. src/main/python/apache/thermos/core/process.py (lines 408 - 416) <https://reviews.apache.org/r/47853/#comment210347> I am slightly confused here. The way I understand the code `self._sandbox` points to a path of the host filesystem. So, if we set `cwd` to that path, what does it point to when we are running within the container? Is this related to the bind mount that you mention in the description of this patch? Additional things that come to mind (but that are out of scope of this particular review request) * documentation * `aurora task ssh` could probably be update to use the containerizer launch command as well. - Stephan Erb On Aug. 1, 2016, 7:22 p.m., Joshua Cohen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47853/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2016, 7:22 p.m.) > > > Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > This changes the approach to launching tasks with filesystem images in the > unified containerizer. Instead of adding an `Image` to the `MesosContainer`, > we instead add the task filesystem as a `Volume` with an associated image. > This image is mounted in the mesos directory under the `taskfs` path. The > executor, on start up does the following: > > 1. Creates user/group under the taskfs root. > 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that > root as well as mounting procfs. > 3. From there, task execution is essentially unchanged minus some slight > changes to the environment depending on whether we're running in a pivoted > root. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 1d66208490aff6ea8af4c737845fa2cf13617529 > examples/vagrant/upstart/aurora-scheduler.conf > 954ddb48e923e0a2a29c415975c4f69afcad37b5 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > c5159314543cba15ac5e525ad0c31dedd398f8bf > src/main/python/apache/aurora/executor/common/sandbox.py > 6d8b7f58b639a60cc5a0c0c9ef98dfaaa8a64486 > src/main/python/apache/aurora/executor/thermos_task_runner.py > 3896e3841562600379705dbf78a6f62728246348 > src/main/python/apache/thermos/core/BUILD > 1094664e112cc71af37835f32037e9eb6d047202 > src/main/python/apache/thermos/core/process.py > 1791b5ff9a36eef7470bef9a6ebbafaf0ab05ca3 > src/main/python/apache/thermos/core/runner.py > fe971edaa2448afaf0fc342e11bc370de96ef5e4 > src/main/python/apache/thermos/runner/thermos_runner.py > 0d06e8e2ac78d26ba8f63744853eb5ce3f6aced6 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 > src/test/python/apache/aurora/executor/common/test_sandbox.py > 63f46e25bdd6fa387dd64975d7b95ee2659f5874 > src/test/python/apache/thermos/core/test_process.py > 77f644c09116266ce02479b9a80403aa68767bd6 > src/test/sh/org/apache/aurora/e2e/Dockerfile > 1b91f02725c1a6762cda2aaa587456341df4ba5a > src/test/sh/org/apache/aurora/e2e/Dockerfile.netcat PRE-CREATION > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > bf6ef69401782d6c65a2d72e9270e52d1ad9fb9f > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > edeafbea288c95c19c82aede09717840b569528d > src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora > 9569eec2a32e0ea5212517c0082fc906036d1e57 > src/test/sh/org/apache/aurora/e2e/run-server.sh PRE-CREATION > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 47bd94d1ce2aeffaefb67ac8325fc2f1d21c934c > > Diff: https://reviews.apache.org/r/47853/diff/ > > > Testing > ------- > > Lots of manual testing, e2e tests, etc. > > I didn't add much coverage on the thermos side of things because it seemed > like this was better served by the e2e tests than by doing a bunch of > subprocess.check_call mocking. On the e2e front I created a new Dockerfile > that sets up a much slimmer filesystem image that explicitly does not include > python to ensure that the executor's filesystem is truly isolated. > > > Thanks, > > Joshua Cohen > >
