> On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 200-209
> > <https://reviews.apache.org/r/47853/diff/2/?file=1453189#file1453189line200>
> >
> >     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.

They do have implicit assumptions, I'm ok with that though, I wasn't building 
this functionality under the assumption that a Docker image being used via the 
DockerContainerizer would work out of the box with the new containerizer (at 
the very least you'll no longer require python in your image).

That said, it's fairly ssafe to check for exit code 4 (per the manpage: UID 
exists) from the useradd call and continue in that case. For groupadd we can 
pass the -f flag to simply exit 0 if the group exists.


> On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, lines 408-416
> > <https://reviews.apache.org/r/47853/diff/2/?file=1453192#file1453192line408>
> >
> >     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?

Good catch. This is a leftover from my original impl that bind mounted the 
sandbox path into the task's filesystem. The new mesos-containerizer launch 
subcommand does not have that behavior though, so I'll need to restore that 
bind mount.


> On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, lines 
> > 98-102
> > <https://reviews.apache.org/r/47853/diff/2/?file=1453188#file1453188line98>
> >
> >     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.

Added a note to the help text.


> On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/thermos_task_runner.py, lines 268-270
> > <https://reviews.apache.org/r/47853/diff/2/?file=1453190#file1453190line268>
> >
> >     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.

Added some code to fail fast if task is using an image but path to 
mesos-containerizer is not provided.


- Joshua


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


On Aug. 2, 2016, 8:55 p.m., Joshua Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47853/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2016, 8:55 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
> 
>

Reply via email to