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

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

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")
 		})