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




src/main/python/apache/aurora/executor/common/sandbox.py (line 36)
<https://reviews.apache.org/r/51298/#comment213183>

    Please extend this doc string slightly to mention that this path is within 
the host filesystem.



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (lines 117 
- 124)
<https://reviews.apache.org/r/51298/#comment213205>

    Have you considered reading that value from the `MESOS_SANDBOX` environment 
variable?
    
    From the docs: "MESOS_SANDBOX: Path to the mapped sandbox inside of the 
container (determined by the agent flag sandbox_directory) for either mesos 
container with image or docker container. For the case of command task without 
image specified, it is the path to the sandbox on the host filesystem, which is 
identical to MESOS_DIRECTORY. MESOS_DIRECTORY is always the sandbox on the host 
filesystem."



src/main/python/apache/aurora/executor/common/sandbox.py (line 250)
<https://reviews.apache.org/r/51298/#comment213204>

    I believe we don't need that `or mesos_directory` here. It should never 
happen that sanbox_mount_point is None. And if it happens, it is better to 
crash early rather then running into undefined behaviour later on.



src/main/python/apache/aurora/executor/common/sandbox.py (lines 259 - 263)
<https://reviews.apache.org/r/51298/#comment213203>

    We are executing this as root on the host filesystem. It might therefore 
make sense to pass `-n` here to prevent that the `/etc/mtab` is modified.



src/main/python/apache/thermos/core/process.py (lines 189 - 200)
<https://reviews.apache.org/r/51298/#comment213208>

    The `ProcessBase` class is normaly oblivious to the existance of Mesos. Do 
you feel it would make sense to inline this special case at the call-side?
    
    I haven't checked that, but I suppose in its current place we would leak 
the containerizer launch call in the Thermos UI?



src/main/python/apache/thermos/core/process.py (line 420)
<https://reviews.apache.org/r/51298/#comment213181>

    This will point to a wrong path in the `taskfs_isolated` case as the 
profile will be sourced by bash before calling the containerize launch command. 
At that point in time we are still in the host filesystem.
    
    Might be worthwhile to add a test to check that it is working correctly.



src/main/python/apache/thermos/core/process.py (line 437)
<https://reviews.apache.org/r/51298/#comment213175>

    In the `taskfs_isolated` case, `cwd` will be set to a path within the 
container, but we use it before the pivot root is completed. This is a problem 
similar to the thermos_profile issue mentioned above.



src/main/python/apache/thermos/runner/thermos_runner.py (lines 139 - 144)
<https://reviews.apache.org/r/51298/#comment213170>

    Can this be changed in a meaningful way? I have always assumed that the 
observer UI has the hardcoded assumption that logs can be found within 
`sandbox/.logs`.


- Stephan Erb


On Aug. 23, 2016, 10:35 p.m., Joshua Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51298/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2016, 10:35 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> - Add an option to skip the groupadd/useradd calls into the task's filesystem.
> - Mount any configured volumes into the task's filesystem.
> - Clean up http server script used by appc e2e tests.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> dde19a6914c7c7b2178220707f242f61f11f38bd 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 5f091af7636bd94f028f15d63437e305b02f741c 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> 1d713ca3d81a2e7be88b787dfcab328d887be24c 
>   src/main/python/apache/thermos/core/process.py 
> a296fa715ef6fbb8d5feae356914334437f353f1 
>   src/main/python/apache/thermos/runner/thermos_runner.py 
> 441bacdfe93b1805a03a1216762c74db810a9540 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 06601df3bc355a690ff1789b2e2e34484fadefe9 
>   src/test/python/apache/thermos/core/test_process.py 
> 759f783202803c296ce19bb64c59cbe896d40a43 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> b69ddf129c0015a878b089d85d731bc0c26fd55c 
>   src/test/sh/org/apache/aurora/e2e/run-server.sh 
> 76939888bed2e8138671d97f7bc56fd5641008e4 
> 
> Diff: https://reviews.apache.org/r/51298/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Reply via email to