From d19ee28089ef8ae497ede706bef49dc921055fa1 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 2 Sep 2021 21:13:23 +0800 Subject: [PATCH] healthcheck: stop checking when max attempts are reached This commit adds a check to the returned error from calling retryCheck such that when the max number of attempts is reached, the health check will quit. --- healthcheck/healthcheck.go | 32 ++++++++++---- healthcheck/healthcheck_test.go | 77 ++++++++++++++++++++------------- 2 files changed, 70 insertions(+), 39 deletions(-) diff --git a/healthcheck/healthcheck.go b/healthcheck/healthcheck.go index 8f92d9aac..5244eb888 100644 --- a/healthcheck/healthcheck.go +++ b/healthcheck/healthcheck.go @@ -86,6 +86,8 @@ func (m *Monitor) Stop() error { return fmt.Errorf("monitor already stopped") } + log.Info("Health monitor shutting down") + close(m.quit) m.wg.Wait() @@ -166,10 +168,18 @@ func (o *Observation) monitor(shutdown shutdownFunc, quit chan struct{}) { for { select { case <-o.Interval.Ticks(): - o.retryCheck(quit, shutdown) + // retryCheck will return errMaxAttemptsReached when + // the max attempts are reached. In that case we will + // stop the ticker and quit. + if o.retryCheck(quit, shutdown) { + log.Debugf("Health check: max attempts " + + "failed, monitor exiting") + return + } // Exit if we receive the instruction to shutdown. case <-quit: + log.Debug("Health check: monitor quit") return } } @@ -178,8 +188,11 @@ func (o *Observation) monitor(shutdown shutdownFunc, quit chan struct{}) { // retryCheck calls a check function until it succeeds, or we reach our // configured number of attempts, waiting for our back off period between failed // calls. If we fail to obtain a passing health check after the allowed number -// of calls, we will request shutdown. -func (o *Observation) retryCheck(quit chan struct{}, shutdown shutdownFunc) { +// of calls, we will request shutdown. It returns a bool to indicate whether +// the max number of attempts is reached. +func (o *Observation) retryCheck(quit chan struct{}, + shutdown shutdownFunc) bool { + var count int for count < o.Attempts { @@ -197,13 +210,14 @@ func (o *Observation) retryCheck(quit chan struct{}, shutdown shutdownFunc) { "%v", o, o.Timeout) case <-quit: - return + log.Debug("Health check: monitor quit") + return false } // If our error is nil, we have passed our health check, so we // can exit. if err == nil { - return + return false } // If we have reached our allowed number of attempts, this @@ -211,8 +225,7 @@ func (o *Observation) retryCheck(quit chan struct{}, shutdown shutdownFunc) { if count == o.Attempts { shutdown("Health check: %v failed after %v "+ "calls", o, o.Attempts) - - return + return true } log.Infof("Health check: %v, call: %v failed with: %v, "+ @@ -225,7 +238,10 @@ func (o *Observation) retryCheck(quit chan struct{}, shutdown shutdownFunc) { case <-time.After(o.Backoff): case <-quit: - return + log.Debug("Health check: monitor quit") + return false } } + + return false } diff --git a/healthcheck/healthcheck_test.go b/healthcheck/healthcheck_test.go index ed810ed73..b198e60a5 100644 --- a/healthcheck/healthcheck_test.go +++ b/healthcheck/healthcheck_test.go @@ -132,48 +132,58 @@ func TestRetryCheck(t *testing.T) { // expectedShutdown is true if we expect a shutdown to be // triggered because all of our calls failed. expectedShutdown bool + + // maxAttemptsReached specifies whether the max allowed + // attempts are reached from calling retryCheck. + maxAttemptsReached bool }{ { - name: "first call succeeds", - errors: []error{nil}, - attempts: 2, - timeout: time.Hour, - expectedShutdown: false, + name: "first call succeeds", + errors: []error{nil}, + attempts: 2, + timeout: time.Hour, + expectedShutdown: false, + maxAttemptsReached: false, }, { - name: "first call fails", - errors: []error{errNonNil}, - attempts: 1, - timeout: time.Hour, - expectedShutdown: true, + name: "first call fails", + errors: []error{errNonNil}, + attempts: 1, + timeout: time.Hour, + expectedShutdown: true, + maxAttemptsReached: true, }, { - name: "fail then recover", - errors: []error{errNonNil, nil}, - attempts: 2, - timeout: time.Hour, - expectedShutdown: false, + name: "fail then recover", + errors: []error{errNonNil, nil}, + attempts: 2, + timeout: time.Hour, + expectedShutdown: false, + maxAttemptsReached: false, }, { - name: "always fail", - errors: []error{errNonNil, errNonNil}, - attempts: 2, - timeout: time.Hour, - expectedShutdown: true, + name: "always fail", + errors: []error{errNonNil, errNonNil}, + attempts: 2, + timeout: time.Hour, + expectedShutdown: true, + maxAttemptsReached: true, }, { - name: "no calls", - errors: nil, - attempts: 0, - timeout: time.Hour, - expectedShutdown: false, + name: "no calls", + errors: nil, + attempts: 0, + timeout: time.Hour, + expectedShutdown: false, + maxAttemptsReached: false, }, { - name: "call times out", - errors: nil, - attempts: 1, - timeout: 1, - expectedShutdown: true, + name: "call times out", + errors: nil, + attempts: 1, + timeout: 1, + expectedShutdown: true, + maxAttemptsReached: true, }, } @@ -203,8 +213,11 @@ func TestRetryCheck(t *testing.T) { // on us sending errors into the mocked caller's error // channel. done := make(chan struct{}) + retryResult := false go func() { - observation.retryCheck(quit, shutdownFunc) + retryResult = observation.retryCheck( + quit, shutdownFunc, + ) close(done) }() @@ -218,6 +231,8 @@ func TestRetryCheck(t *testing.T) { // check function before we start checking results. <-done + require.Equal(t, test.maxAttemptsReached, retryResult, + "retryCheck returned unexpected error") require.Equal(t, test.expectedShutdown, shutdown, "unexpected shutdown state") })