aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephan Erb <s...@apache.org>
Subject Re: Review Request 58167: Fix Thermos Health Check for MesosContainerizer with `--nosetuid-health-checks`
Date Tue, 04 Apr 2017 07:38:48 GMT

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


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