aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@apache.org>
Subject Re: Review Request 51298: A few executor fixes for filesystem isolation:
Date Fri, 26 Aug 2016 03:50:59 GMT


> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 165-168
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line165>
> >
> >     The `pop` is unnecessary. Luckily the Python interpreter is already protecting
us from this mistake
> >     
> >     ```
> >     >>> def f(x, **kwargs):
> >     ...     print x
> >     ...
> >     >>> f(x=1, **{'x': 2})
> >     Traceback (most recent call last):
> >       File "<stdin>", line 1, in <module>
> >     TypeError: f() got multiple values for keyword argument 'x'
> >     ```

I'm not popping it for fear of it being set twice, I'm poppinig it because I want to be sure
we pass `None` for the user value for this sandbox.


> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 253-257
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line253>
> >
> >     That `os.path.join(mesos_directory, container_path)` looks slightly strange.
Either we have to stip the leading `/` of container path for this one as well, or the `join`
is not necessary as (from the docs): 'If a component is an absolute path, all previous components
are thrown away and joining continues from the absolute path component.'
> >     
> >     Optional style nitpick: I feel the function could be easier to understand if
we get rid of the list comprehension and move the computation of the two paths to the `for`
loop below, so that declaration and usage are closer together.

So this was based on some confusion on my part. I was under the mistaken impression that Mesos
would always mount volumes relative to `$MESOS_DIRECTORY` (thus why I was trying to join the
paths). It was only working due to the fact that I was not stripping the leading slash from
the container path, which as you point out causes us to throw away all previous path parts,
meaning we were mounting that absolute path which is how Mesos *actually* mounts volumes into
the container's mount namespace. Now that I've cleared that up everything works as expected
without the os.path.join.

As for the rest, thanks for the nudge, that definitely was not the cleanest code.


> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 262-263
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line262>
> >
> >     If you feel fancy, you could shorten this via `lstrip`:
> >     
> >     ```
> >     >>> '/foo/bar'.lstrip('/')
> >     'foo/bar'
> >     >>> 'foo/bar'.lstrip('/')
> >     'foo/bar'
> >     ```

Ahh, nice, thanks!


> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, lines 482-483
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486012#file1486012line482>
> >
> >     We are now sourcing the thermos profile twice: Once within the container image
and once on the outside. 
> >     
> >     Probably does no harm.

I've cleaned this up so that we only source it once regardless.


- Joshua


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


On Aug. 25, 2016, 9:26 p.m., Joshua Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51298/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2016, 9:26 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/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f

>   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

>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113

> 
> Diff: https://reviews.apache.org/r/51298/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message