Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Zhitao Li 

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


Ship it!




Ship It!


src/main/python/apache/aurora/executor/common/health_checker.py (line 265)


nit on the name of `isolator`: `isolator` is already a well-defined concept 
within Mesos, and it seems to me that this is not related to that. Maybe 
consider naming this as `wrapped_fn`?


- Zhitao Li


On Sept. 15, 2016, 3:15 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 15, 2016, 3:15 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Stephan Erb 

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


Ship it!




Patch LGTM.

In general, I don't like the trend that Thermos is growing in complexity with 
multiple different places worrying about setuid, fs isolation, etc.  We should 
have an eye on this so that we don't get slowed down by too much complexity and 
bugs in the future.

- Stephan Erb


On Sept. 15, 2016, 5:15 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 15, 2016, 5:15 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Aurora ReviewBot 

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


Ship it!




Master (5069f93) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 15, 2016, 3:15 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 15, 2016, 3:15 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Joshua Cohen 

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

(Updated Sept. 15, 2016, 3:15 p.m.)


Review request for Aurora, Stephan Erb and Zhitao Li.


Changes
---

Review feedback.


Repository: aurora


Description
---

Ensure shell health checkers running for tasks running under an isolated 
fileystem are run within that filesystem.


Diffs (updated)
-

  src/main/python/apache/aurora/common/health_check/shell.py 
35750823553406a96282545066f1291c20347ffa 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
5211f28e4e6c0efd29d7d79058128adb71ec7da8 
  src/main/python/apache/aurora/executor/common/health_checker.py 
5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
  src/main/python/apache/thermos/common/BUILD 
879b812b6a262d6e13b64e662999dd436f039748 
  src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
  src/main/python/apache/thermos/core/process.py 
2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
011464cbe1df00f2a56d4690176e7c2d0d3fd535 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
  src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
290627f8bc38d31ae123cfd1cdd36e9291c2de18 

Diff: https://reviews.apache.org/r/51899/diff/


Testing
---

./build-support/jenkins/build.sh
e2e tests


Thanks,

Joshua Cohen

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Joshua Cohen 


> On Sept. 15, 2016, 1:23 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 73
> > 
> >
> > You pass in `_cmd` and `_isolator_fn` into the `ShellHealthCheck`. Have 
> > you considered just passing the result of `isolator_fn(cmd)` into the 
> > `ShellHealthCheck`? Or is there a reason that this can only be done in 
> > `__call__`?

I need the original command available so we don't leak the `mesos-containerizer 
launch ...` wrapper in the event of a health check failure. I originally just 
passed both the `cmd` and the `wrapped_cmd` but it felt like a very strange 
interface. "Create a `ShellHealthCheck` to run this `cmd` unless I specified 
`wrapped_cmd` then run that, but still use `cmd` when reporting errors."

If you feel strongly I can change it back to that. That said, you're right that 
we don't need to execute `isolator_fn` in `call`. I updated it a bit to clean 
that usage up.


> On Sept. 15, 2016, 1:23 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/common/process_util.py, lines 24-26
> > 
> >
> > Sorry for bringing up that old thing again :-)
> > 
> > I belive this is only needed because we set 'shell:true' below. Have 
> > you tested what happens if we 'shell:false'?
> > 
> > When looking th `ps tree` one sees that we run `sh -c bash -c ''` 
> > so that first sh is completely useless and we could probably eliminate it 
> > by setting shell to false.

I think even if we set `shell: false` we'd still need to wrap it in a `bash -c` 
invocation (because we've always been clear that Process cmdlines are 
explciitly *bash* command lines), and it's the bash wrapper that causes us to 
need to escape the quotes, not the `sh ` wrapper from mesos-containerizer.

I tried a few combinations of `shell: false`, since we're just using it to 
launch a shell anyway it *shouldn't* be necessary. However I wasn't able to get 
it to successfully launch a command. It might be possible, but I'm not sure 
it's worth the effort to suss out the right incantation of splitting the 
command to get it to work. I've add a TODO to investigate if/when time allows.


- Joshua


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


On Sept. 14, 2016, 8:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 14, 2016, 8:49 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-15 Thread Stephan Erb 

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




src/main/python/apache/aurora/common/health_check/shell.py (line 73)


You pass in `_cmd` and `_isolator_fn` into the `ShellHealthCheck`. Have you 
considered just passing the result of `isolator_fn(cmd)` into the 
`ShellHealthCheck`? Or is there a reason that this can only be done in 
`__call__`?



src/main/python/apache/thermos/common/process_util.py (lines 24 - 26)


Sorry for bringing up that old thing again :-)

I belive this is only needed because we set 'shell:true' below. Have you 
tested what happens if we 'shell:false'?

When looking th `ps tree` one sees that we run `sh -c bash -c ''` so 
that first sh is completely useless and we could probably eliminate it by 
setting shell to false.



src/main/python/apache/thermos/common/process_util.py (line 34)


That is the line I was talking about.


- Stephan Erb


On Sept. 14, 2016, 10:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 14, 2016, 10:49 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-14 Thread Aurora ReviewBot 

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


Ship it!




Master (5069f93) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 14, 2016, 8:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51899/
> ---
> 
> (Updated Sept. 14, 2016, 8:49 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Ensure shell health checkers running for tasks running under an isolated 
> fileystem are run within that filesystem.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 35750823553406a96282545066f1291c20347ffa 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 5211f28e4e6c0efd29d7d79058128adb71ec7da8 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
>   src/main/python/apache/thermos/common/BUILD 
> 879b812b6a262d6e13b64e662999dd436f039748 
>   src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
>   src/main/python/apache/thermos/core/process.py 
> 2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 011464cbe1df00f2a56d4690176e7c2d0d3fd535 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 290627f8bc38d31ae123cfd1cdd36e9291c2de18 
> 
> Diff: https://reviews.apache.org/r/51899/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> e2e tests
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Review Request 51899: Ensure shell health checkers running for tasks running under an isolated fileystem are run within that filesystem.

2016-09-14 Thread Joshua Cohen 

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

Review request for Aurora, Stephan Erb and Zhitao Li.


Repository: aurora


Description
---

Ensure shell health checkers running for tasks running under an isolated 
fileystem are run within that filesystem.


Diffs
-

  src/main/python/apache/aurora/common/health_check/shell.py 
35750823553406a96282545066f1291c20347ffa 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
5211f28e4e6c0efd29d7d79058128adb71ec7da8 
  src/main/python/apache/aurora/executor/common/health_checker.py 
5fc845eceac6f0c048d7489fdc4c672b0c609ea0 
  src/main/python/apache/thermos/common/BUILD 
879b812b6a262d6e13b64e662999dd436f039748 
  src/main/python/apache/thermos/common/process_util.py PRE-CREATION 
  src/main/python/apache/thermos/core/process.py 
2134d4ff05861d4eaee9bc7ea4763e76ce63288c 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
011464cbe1df00f2a56d4690176e7c2d0d3fd535 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
bb6ea69dd94298c5b8cf4d5f06d06eea7790d66e 
  src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
290627f8bc38d31ae123cfd1cdd36e9291c2de18 

Diff: https://reviews.apache.org/r/51899/diff/


Testing
---

./build-support/jenkins/build.sh
e2e tests


Thanks,

Joshua Cohen