Fix health server timeout

We observed cases when the k8s client code had some delay somewhere,
which exceeded the configured k8s client timeout, and even the
health client timeout on the caller side, causing unneeded fencing.
To fix this, the health server monitors the context itself now,
and new unit tests cover this usecase.

Signed-off-by: Marc Sluiter <[email protected]>

Author: slintes

pkg/peerhealth/client_server_test.go

@@ -2,6 +2,7 @@ package peerhealth
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -78,7 +79,7 @@ var _ = Describe("Checking health using grpc client and server", func() {
 
 			By("calling isHealthy")
 			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-			defer (cancel)()
+			defer cancel()
 			resp, err := phClient.IsHealthy(ctx, &HealthRequest{
 				NodeName: nodeName,
 			})
@@ -110,7 +111,7 @@ var _ = Describe("Checking health using grpc client and server", func() {
 
 			By("calling isHealthy")
 			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-			defer (cancel)()
+			defer cancel()
 			Eventually(func() bool {
 				resp, err := phClient.IsHealthy(ctx, &HealthRequest{
 					NodeName: nodeName,
@@ -122,4 +123,60 @@ var _ = Describe("Checking health using grpc client and server", func() {
 
 	})
 
+	Describe("with a peer running into API timeout", func() {
+
+		var (
+			apiCallDelay       = 7 * time.Second
+			peerRequestTimeout = 5 * time.Second // must be lower than apiCallDelay for this test!
+		)
+
+		BeforeEach(func() {
+			reader.delay = &apiCallDelay
+		})
+
+		AfterEach(func() {
+			reader.delay = nil
+		})
+
+		It("should return API error", func() {
+			By("calling isHealthy")
+			// The health server code has a hardcoded timeout of 3s for the API call!
+			// When we add a delay of > 3s to the API call, that 3s timeout needs to be respected,
+			// to not exceed the peerRequestTimeout (5s) of the client.
+			ctx, cancel := context.WithTimeout(context.Background(), peerRequestTimeout)
+			defer cancel()
+			resp, err := phClient.IsHealthy(ctx, &HealthRequest{
+				NodeName: nodeName,
+			})
+
+			// wait for having more logs from async server code
+			time.Sleep(10 * time.Second)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(api.HealthCheckResponseCode(resp.Status)).To(Equal(api.ApiError))
+		})
+
+	})
+
+	Describe("with a peer running into API error", func() {
+
+		BeforeEach(func() {
+			reader.err = fmt.Errorf("some API error")
+		})
+
+		AfterEach(func() {
+			reader.err = nil
+		})
+
+		It("should return API error", func() {
+			By("calling isHealthy")
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+			resp, err := phClient.IsHealthy(ctx, &HealthRequest{
+				NodeName: nodeName,
+			})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(api.HealthCheckResponseCode(resp.Status)).To(Equal(api.ApiError))
+		})
+	})
 })

pkg/peerhealth/server.go

@@ -112,11 +112,9 @@ func (s *Server) IsHealthy(ctx context.Context, request *HealthRequest) (*Health
 	apiCtx, cancelFunc := context.WithTimeout(ctx, apiServerTimeout)
 	defer cancelFunc()
 
-	// list snrs from all ns
-	// don't use cache, because this also tests API server connectivity!
 	snrs := &v1alpha1.SelfNodeRemediationList{}
-	if err := s.reader.List(apiCtx, snrs); err != nil {
-		s.log.Error(err, "api error, failed to list snrs")
+	if err := s.listWithTimeoutHandling(apiCtx, snrs); err != nil {
+		s.log.Info("failed to list SelfNodeRemediations, returning ApiError", "cause", err.Error())
 		return toResponse(selfNodeRemediationApis.ApiError)
 	}
 
@@ -136,6 +134,28 @@ func (s *Server) IsHealthy(ctx context.Context, request *HealthRequest) (*Health
 	return toResponse(selfNodeRemediationApis.Healthy)
 }
 
+// listWithTimeoutHandling wraps a reader list method with additional context timeout handling.
+// This is needed because we observed cases when the client code had some delay somewhere, which exceeded the
+// configured timeout, and even the timeout on the caller side, causing unneeded fencing actions.
+func (s *Server) listWithTimeoutHandling(apiCtx context.Context, snrs *v1alpha1.SelfNodeRemediationList) error {
+	// run the list call async
+	var listErr error
+	listDone := make(chan struct{})
+	go func() {
+		// don't use cached client but the reader, because this also tests API server connectivity!
+		listErr = s.reader.List(apiCtx, snrs)
+		close(listDone)
+	}()
+
+	// wait until context expired or list call returned
+	select {
+	case <-apiCtx.Done():
+		return fmt.Errorf("timed out")
+	case <-listDone:
+		return listErr
+	}
+}
+
 func (s *Server) getNode(ctx context.Context, nodeName string) (*corev1.Node, error) {
 	apiCtx, cancelFunc := context.WithTimeout(ctx, apiServerTimeout)
 	defer cancelFunc()

pkg/peerhealth/suite_test.go

@@ -3,6 +3,7 @@ package peerhealth
 import (
 	"context"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
@@ -32,7 +33,7 @@ const nodeName = "somenode"
 
 var cfg *rest.Config
 var k8sClient client.Client
-var reader client.Reader
+var reader *ReaderWrapper
 var testEnv *envtest.Environment
 var snrReconciler *controllers.SelfNodeRemediationReconciler
 var cancelFunc context.CancelFunc
@@ -73,8 +74,12 @@ var _ = BeforeSuite(func() {
 	k8sClient = k8sManager.GetClient()
 	Expect(k8sClient).ToNot(BeNil())
 
-	reader = k8sManager.GetAPIReader()
-	Expect(reader).ToNot(BeNil())
+	originalReader := k8sManager.GetAPIReader()
+	Expect(originalReader).ToNot(BeNil())
+
+	reader = &ReaderWrapper{
+		Reader: originalReader,
+	}
 
 	// we need a reconciler for getting last SNR namespace
 	snrReconciler = &controllers.SelfNodeRemediationReconciler{
@@ -101,3 +106,25 @@ var _ = AfterSuite(func() {
 	cancelFunc()
 	Expect(testEnv.Stop()).To(Succeed())
 })
+
+type ReaderWrapper struct {
+	client.Reader
+	mu    sync.RWMutex
+	delay *time.Duration
+	err   error
+}
+
+func (r *ReaderWrapper) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	r.mu.RLock()
+	d, e := r.delay, r.err
+	r.mu.RUnlock()
+
+	if d != nil {
+		time.Sleep(*d)
+	}
+	if e != nil {
+		return e
+	}
+
+	return r.Reader.List(ctx, list, opts...)
+}