Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of jcohen@twopensource.com
 designates 209.85.192.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20141009203431.24818.10437@reviews.apache.org>
References: <20141009145313.24817.15764@reviews.apache.org>
	<20141009203431.24818.10437@reviews.apache.org>
Date: Thu, 9 Oct 2014 14:12:47 -0700
Message-ID: 
 <CAMnduq-Tp2_3pSvV4vFyE=K2DFkx3ebPGgvy1c+hRNEkiOMM9g@mail.gmail.com>
Subject: Re: Review Request 26383: Health Check Disabler
From: Joshua Cohen <jcohen@twopensource.com>
To: Aurora <reviews@aurora.incubator.apache.org>,
 Kevin Sweeney <kevints@apache.org>
Cc: Joe Smith <yasumoto7@gmail.com>, Brian Wickman <wickman@apache.org>,
	Zameer Manji <zmanji@twopensource.com>, Bill Farner <wfarner@apache.org>,
	David Pan <david.pan2@gmail.com>
Content-Type: multipart/alternative; boundary=001a11c1bf52f0fa32050503e401

--001a11c1bf52f0fa32050503e401
Content-Type: text/plain; charset=UTF-8

I feel like we should err on the side of correctness here, rather than
simplicity? The dangers of someone accidentally leaving health checks
disabled indefinitely (on a service that has opted in to health checks) are
not insignificant.

On Thu, Oct 9, 2014 at 1:34 PM, Kevin Sweeney <kevints@apache.org> wrote:

>
>
> > On Oct. 9, 2014, 7:53 a.m., Bill Farner wrote:
> > > src/main/python/apache/aurora/executor/common/health_checker.py, line
> 66
> > > <
> https://reviews.apache.org/r/26383/diff/3/?file=716355#file716355line66>
> > >
> > >     FWIW i actually meant to suggest that the snooze has no concept of
> time at all.  If the file is there, don't perform health checks.  When you
> want to re-enable health checks, delete the file.  Happy to hear what
> others think about that.
> >
> > Zameer Manji wrote:
> >     Doesn't this mean user error can disable health checks forever? I
> think we should treat disabling health checking as an exceptional state
> (since the proceses has opted in to health checking) and therefore require
> user action (increasing mtime) to continue to stay in this state.
> >
> > Kevin Sweeney wrote:
> >     Presumably we can trust the user here - health checking is after all
> opt-in via {{thermos.ports[health]}}. Making it a simple on/off switch
> greatly simplifies the code on our end.
> >
> > Zameer Manji wrote:
> >     I'm very worried about introducing a feature which can allow
> unhealthy instances live forever. Furthermore this information isn't
> exposed on the Aurora UI or Observer UI so there isn't an easy way to check
> if an instance has health checking disabled or not. A user can be
> completely oblivious that health checking of their instance has been
> disabled.
> >
> > David Pan wrote:
> >     The motivation behind the health check disabler is for another
> project I am working on, which is the JVM Heap Dumper.  Long story short,
> the JVM Heap Dumper needs to disable health checks for the task that is
> being heap dumped.  Under normal circumstances, the heap dumper can
> re-enable the health checks.  However, if the heap dumper dies
> unexpectedly, the health checks will remain disabled forever if we don't
> implement time control.  We don't want the disabling and enabling of health
> checks to be a manual process.
>
> I still agree with Bill here - the simpler implementation (skip health
> check if the file exists) is easier to reason about and a cleaner API.
>
> A process running in the sandbox could implement some form of timeout
> logic itself (for example, the application could expose an endpoint that
> forks the equivalent of "touch .healthchecksnooze; sleep 60; rm
> .healthchecksnooze") but I like the idea of keeping the executor API here
> simple to explain and reason about.
>
>
> - Kevin
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26383/#review55985
> -----------------------------------------------------------
>
>
> On Oct. 8, 2014, 6:56 p.m., David Pan wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/26383/
> > -----------------------------------------------------------
> >
> > (Updated Oct. 8, 2014, 6:56 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
> appropriate unit tests were modified/added.
> >
> > The corresponding JIRA ticket is the following:
> > https://issues.apache.org/jira/browse/AURORA-795
> >
> >
> > Diffs
> > -----
> >
> >   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
> >
> >
>
>

--001a11c1bf52f0fa32050503e401--