> 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 > >
