> On Sept. 15, 2016, 1:23 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/common/health_check/shell.py, line 73 > > <https://reviews.apache.org/r/51899/diff/1/?file=1498688#file1498688line73> > > > > 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 > > <https://reviews.apache.org/r/51899/diff/1/?file=1498692#file1498692line24> > > > > 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 > >
