Apply Feedback

- clarify messages, remove unused method
- additional unit tests

Signed-off-by: Michael Shitrit <[email protected]>

Author: mshitrit

api/v1alpha1/selfnoderemediationconfig_webhook.go

@@ -212,15 +212,15 @@ func (r *SelfNodeRemediationConfig) validatePeerTimeoutSafety() admission.Warnin
 			"PeerRequestTimeout (%s) is less than ApiServerTimeout + MinimumBuffer (%s + %s = %s). "+
 				"This configuration may lead to race conditions where peer health checks time out "+
 				"before API server checks complete, potentially causing premature remediation. "+
-				"Consider increasing PeerRequestTimeout to at least %s for safer operation.",
+				"Overriding PeerRequestTimeout to %s for safer operation.",
 			peerRequestTimeoutDuration,
 			apiServerTimeoutDuration,
 			MinimumBuffer,
 			minimumSafePeerTimeout,
 			minimumSafePeerTimeout,
 		)
 		warnings = append(warnings, warningMsg)
-		selfNodeRemediationConfigLog.Info("PeerRequestTimeout safety warning",
+		selfNodeRemediationConfigLog.Info("PeerRequestTimeout safety warning, overriding PeerRequestTimeout to minimumSafeTimeout",
 			"peerRequestTimeout", peerRequestTimeoutDuration,
 			"apiServerTimeout", apiServerTimeoutDuration,
 			"minimumSafeTimeout", minimumSafePeerTimeout)

pkg/apicheck/check_test.go

@@ -0,0 +1,83 @@
+package apicheck
+
+import (
+	"testing"
+	"time"
+
+	"github.com/go-logr/logr"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
+
+	"github.com/medik8s/self-node-remediation/api/v1alpha1"
+)
+
+func TestApiCheck(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "ApiCheck Suite")
+}
+
+var _ = Describe("ApiConnectivityCheck", func() {
+	var (
+		apiCheck     *ApiConnectivityCheck
+		config       *ApiConnectivityCheckConfig
+		fakeRecorder *record.FakeRecorder
+		log          logr.Logger
+	)
+
+	BeforeEach(func() {
+		log = ctrl.Log.WithName("test")
+		fakeRecorder = record.NewFakeRecorder(10)
+
+		config = &ApiConnectivityCheckConfig{
+			Log:                log,
+			MyNodeName:         "test-node",
+			ApiServerTimeout:   5 * time.Second,
+			PeerRequestTimeout: 7 * time.Second,
+			Recorder:           fakeRecorder,
+		}
+
+		apiCheck = &ApiConnectivityCheck{
+			config: config,
+		}
+	})
+
+	Describe("getEffectivePeerRequestTimeout", func() {
+		Context("when PeerRequestTimeout is safe", func() {
+			It("should return the configured PeerRequestTimeout", func() {
+				// ApiServerTimeout=5s, PeerRequestTimeout=7s, MinimumBuffer=2s
+				// 7s >= (5s + 2s), so it's safe
+				effectiveTimeout := apiCheck.getEffectivePeerRequestTimeout()
+
+				Expect(effectiveTimeout).To(Equal(7 * time.Second))
+
+				// Should not emit any events
+				Expect(len(fakeRecorder.Events)).To(Equal(0))
+			})
+		})
+
+		Context("when PeerRequestTimeout is unsafe", func() {
+			It("should return adjusted timeout and emit warning event", func() {
+				config.PeerRequestTimeout = 6 * time.Second // Less than 5s + 2s = 7s
+
+				effectiveTimeout := apiCheck.getEffectivePeerRequestTimeout()
+
+				expectedMinimumTimeout := config.ApiServerTimeout + v1alpha1.MinimumBuffer // 7s
+				Expect(effectiveTimeout).To(Equal(expectedMinimumTimeout))
+
+				// Should emit a warning event
+				Expect(len(fakeRecorder.Events)).To(Equal(1))
+				event := <-fakeRecorder.Events
+				Expect(event).To(ContainSubstring("Warning"))
+				Expect(event).To(ContainSubstring("PeerTimeoutAdjusted"))
+				Expect(event).To(ContainSubstring("6s")) // configured timeout
+				Expect(event).To(ContainSubstring("5s")) // API server timeout
+				Expect(event).To(ContainSubstring("7s")) // safe timeout
+			})
+
+		})
+
+	})
+})

pkg/peerhealth/client_server_test.go

@@ -23,8 +23,9 @@ var _ = Describe("Checking health using grpc client and server", func() {
 	var phServer *Server
 	var cancel context.CancelFunc
 	var phClient *Client
+	var apiServerTimeout = 5 * time.Second
 
-	BeforeEach(func() {
+	JustBeforeEach(func() {
 
 		By("creating test node")
 		node := &v1.Node{
@@ -49,7 +50,7 @@ var _ = Describe("Checking health using grpc client and server", func() {
 		}
 
 		By("Creating server")
-		phServer, err = NewServer(k8sClient, reader, ctrl.Log.WithName("peerhealth test").WithName("phServer"), 9000, certReader, 7*time.Second)
+		phServer, err = NewServer(k8sClient, reader, ctrl.Log.WithName("peerhealth test").WithName("phServer"), 9000, certReader, apiServerTimeout)
 		Expect(err).ToNot(HaveOccurred())
 
 		By("Starting server")
@@ -132,15 +133,18 @@ var _ = Describe("Checking health using grpc client and server", func() {
 
 		BeforeEach(func() {
 			reader.delay = &apiCallDelay
+			apiServerTimeout = 3 * time.Second
+
 		})
 
 		AfterEach(func() {
 			reader.delay = nil
+			apiServerTimeout = 5 * time.Second
 		})
 
 		It("should return API error", func() {
 			By("calling isHealthy")
-			// The health server code has a hardcoded timeout of 3s for the API call!
+			// The health server code has a 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)

pkg/peerhealth/server.go

@@ -154,18 +154,6 @@ func (s *Server) listWithTimeoutHandling(apiCtx context.Context, snrs *v1alpha1.
 	}
 }
 
-func (s *Server) getNode(ctx context.Context, nodeName string) (*corev1.Node, error) {
-	apiCtx, cancelFunc := context.WithTimeout(ctx, s.apiServerTimeout)
-	defer cancelFunc()
-
-	node := &corev1.Node{}
-	if err := s.c.Get(apiCtx, client.ObjectKey{Name: nodeName}, node); err != nil {
-		s.log.Error(err, "api error")
-		return nil, err
-	}
-	return node, nil
-}
-
 func toResponse(status selfNodeRemediationApis.HealthCheckResponseCode) (*HealthResponse, error) {
 	return &HealthResponse{
 		Status: int32(status),