Merge pull request #262 from slintes/health-server-timeout

Fix health server timeout

Author: openshift-merge-bot[bot]

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...)
+}