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

Reply via email to