aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua Cohen" <jco...@twopensource.com>
Subject Re: Review Request 26383: Health Check Disabler
Date Mon, 06 Oct 2014 22:55:47 GMT

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


If we do let the snooze file path remain configurable, please add a test to ensure that people
aren't able to escape the sandbox (e.g. they can't use /etc/shadow as the healthcheck path
;)).


src/main/python/apache/aurora/config/schema/base.py
<https://reviews.apache.org/r/26383/#comment95958>

    Do we need to make this path configurable? I'm trying to think of a reason that someone
might want to change it, and I'm coming up blank, however I could envision a scenario where
someone does something malicious (intentionally or not) like sets this path to something that
shouldn't be removed (e.g. the artifact being run).



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95950>

    These kind of comments are not particularly useful as they're just restating the code
above (which is pretty self explanatory, hurray!).



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95947>

    os.path.join(self.sandbox.root, self.snooze_file_rel_path)



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95948>

    Inline the getmtime call into the snooze age calculation below. Also, use safe_mtime from
twitter commons:
    
    https://github.com/twitter/commons/blob/master/src/python/twitter/common/dirutil/__init__.py#L192



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95949>

    This should probably be logged at the debug level?



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95951>

    Debug here as well.



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95952>

    maybe use safe_delete from twitter commons?
    
    https://github.com/twitter/commons/blob/master/src/python/twitter/common/dirutil/__init__.py#L117



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95954>

    Add some more details here? Aka:
    
    "Health check snooze file found, health checks disabled for another %d seconds."



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95955>

    nit: s/  Performing/ Performing
    
    Also, while we're add it, make sure these logs all have proper punctuation (i.e. end with
periods).



src/main/python/apache/aurora/executor/common/health_checker.py
<https://reviews.apache.org/r/26383/#comment95957>

    I know you didn't originate this pattern, but it seems odd for these defaults to be repeated
on both the HealthChecker and the ThreadedHealthChecker. Perhaps they should be constants,
or maybe ThreadedHealthChecker shouldn't have defaults at all?



src/test/python/apache/aurora/executor/common/test_health_checker.py
<https://reviews.apache.org/r/26383/#comment95959>

    This should move back up top (this won't pass python checkstyle as is, you should be sure
to run:
    
    ./build-support/python/isort-check
    ./build-support/python/checkstyle-check src
    
    to verify everything is kosher).



src/test/python/apache/aurora/executor/common/test_health_checker.py
<https://reviews.apache.org/r/26383/#comment95961>

    This is only used in one test, it doesn't need to be configured globally as part of setUp?


- Joshua Cohen


On Oct. 6, 2014, 9:24 p.m., David Pan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26383/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 9:24 p.m.)
> 
> 
> Review request for Aurora, Joe Smith, Brian Wickman, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> The health check disabler allows health checks for a job to be snoozed temporarily by
touching a snooze file in the job's sandbox.  The path of the snooze file and the snooze duration
can be set in the HealthCheckConfig.  The appropriate unit tests were modified/added.
> 
> The corresponding JIRA ticket is the following:
> https://issues.apache.org/jira/browse/AURORA-795
> 
> 
> Diffs
> -----
> 
>   docs/configuration-reference.md 5166d45ddf95ae5d8afe39dd3b00654ac91857ec 
>   docs/configuration-tutorial.md 67998e9dab6ac429d96d7c0d2df959336b767f32 
>   src/main/python/apache/aurora/config/schema/base.py f12634f103c3eb20e43f37c25d9b0fc3e3d228ec

>   src/main/python/apache/aurora/executor/common/health_checker.py 4980411c847d12655cbb363404707ebd9f0bd163

>   src/test/python/apache/aurora/executor/common/BUILD c7f7a003c865d479ba6e3cd7b5349322f884f653

>   src/test/python/apache/aurora/executor/common/test_health_checker.py aa36415fa891fc523a3a376ffeca5d3cd5ceabec

> 
> Diff: https://reviews.apache.org/r/26383/diff/
> 
> 
> Testing
> -------
> 
> On vagrant in ~/aurora, I ran
> ./pants src/test/python/apache/aurora/executor::
> 
> 
> Thanks,
> 
> David Pan
> 
>


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