----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58167/#review170961 -----------------------------------------------------------
Thanks for the patch! Code change itself looks good to me. Regarding the tests, I am not a great fan of the heavy way of mocking. However that is not really your mistake but rather us being careless before :) src/test/python/apache/aurora/executor/common/test_health_checker.py Lines 576 (patched) <https://reviews.apache.org/r/58167/#comment243802> Call this something like `make_...` or `create...` to make it a little bit more clearer that we create something externally rather than just setting a property somewhere. src/test/python/apache/aurora/executor/common/test_health_checker.py Lines 585-589 (original), 604-608 (patched) <https://reviews.apache.org/r/58167/#comment243801> Same as below, please drop or at least inline those to make the intent clearer. src/test/python/apache/aurora/executor/common/test_health_checker.py Lines 668-672 (patched) <https://reviews.apache.org/r/58167/#comment243800> Your test does not seem to rely on those specific values. To make the test intent clearer, you could drop them and just use the default values. src/test/python/apache/aurora/executor/common/test_health_checker.py Lines 630-632 (original), 699-703 (patched) <https://reviews.apache.org/r/58167/#comment243803> The way the health checker is currently called there can be no reasonable case where the command is ever None (this used to be different IIRC). You could therefore drop the `if other is not None` guard here. - Stephan Erb On April 4, 2017, 7:47 a.m., Charles Raimbert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58167/ > ----------------------------------------------------------- > > (Updated April 4, 2017, 7:47 a.m.) > > > Review request for Aurora, Stephan Erb and Zameer Manji. > > > Bugs: AURORA-1909 > https://issues.apache.org/jira/browse/AURORA-1909 > > > Repository: aurora > > > Description > ------- > > With MesosContainerizer, the health check is performed using a > "mesos-containerizer launch" process, but there is actually a code bug in the > way of getting the user under which to run the health check process: > https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/health_checker.py#L370 > ``` > health_check_user = (os.getusername() if self._nosetuid_health_checks > else assigned_task.task.job.role) > ``` > > If the scheduler is configured with `--nosetuid-health-checks` then > "os.getusername()" is executed, but the "os" python module does not present > any "getusername()" function, which leads the Thermos execution to abort as > follow: > ``` > D0323 01:08:15.453372 16 aurora_executor.py:159] Task started. > E0323 01:08:15.571124 16 aurora_executor.py:121] Traceback (most recent call > last): > File "apache/aurora/executor/aurora_executor.py", line 119, in _run > self._start_status_manager(driver, assigned_task) > File "apache/aurora/executor/aurora_executor.py", line 168, in > _start_status_manager > status_checker = status_provider.from_assigned_task(assigned_task, > self._sandbox) > File "apache/aurora/executor/common/health_checker.py", line 370, in > from_assigned_task > health_check_user = (os.getusername() if self._nosetuid_health_checks > AttributeError: 'module' object has no attribute 'getusername' > ``` > > Following the existing unit testing pattern from test_health_checker.py, a > test case was added to cover the `--nosetuid-health-checks` case for > MesosContainerizer. > > > Diffs > ----- > > src/main/python/apache/aurora/executor/common/health_checker.py > 5bb4768ee3cf571f72580777c291448d372223c0 > src/test/python/apache/aurora/executor/common/test_health_checker.py > adf3ac070c9121d0c6b09bc4dcaee2dc2eb89bc2 > > > Diff: https://reviews.apache.org/r/58167/diff/1/ > > > Testing > ------- > > With Vagrant: > - ./pants test.pytest src/{test,main}/python:: -- -v > - ./build-support/jenkins/build.sh > - ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Charles Raimbert > >
