Mostly cleanup + clarifications

Author: mark-dell-usa

controllers/tests/controller/selfnoderemediation_controller_test.go

@@ -1269,10 +1269,15 @@ func configureUnhealthyNodeAsControlNode() {
 func configureSimulatedPeerResponses(simulateResponses bool) {
 	By("Start simulating peer responses", func() {
 		orgValue := apiCheck.ShouldSimulatePeerResponses
+		orgResponses := apiCheck.SnapshotSimulatedPeerResponses()
 		apiCheck.ShouldSimulatePeerResponses = simulateResponses
+		if simulateResponses {
+			apiCheck.ClearSimulatedPeerResponses()
+		}
 
 		DeferCleanup(func() {
 			apiCheck.ShouldSimulatePeerResponses = orgValue
+			apiCheck.RestoreSimulatedPeerResponses(orgResponses)
 		})
 	})
 }
@@ -1357,21 +1362,13 @@ func addNodes(nodes []newNodeConfig) {
 			for _, pod := range np.pods {
 				By(fmt.Sprintf("Create pod '%s' under node '%s'", pod.name, np.nodeName), func() {
 					_ = createGenericSelfNodeRemediationPod(createdNode, pod.name)
-					apiCheck.SimulatePeerResponses = append(apiCheck.SimulatePeerResponses, pod.simulatedResponse)
+					apiCheck.AppendSimulatedPeerResponse(pod.simulatedResponse)
 				})
 
 			}
 
 		}
 
-		DeferCleanup(func() {
-			By("Removing the additional peer nodes when all relevant tests are complete", func() {
-				Expect(k8sClient.Delete(context.Background(), getNode(shared.Peer2NodeName))).To(Succeed())
-				Expect(k8sClient.Delete(context.Background(), getNode(shared.Peer3NodeName))).To(Succeed())
-			})
-			return
-		})
-
 	})
 }
 

controllers/tests/controller/suite_test.go

@@ -168,6 +168,9 @@ var _ = BeforeSuite(func() {
 
 	apiCheck = shared.NewApiConnectivityCheckWrapper(apiConnectivityCheckConfig, nil)
 
+	// default to simulation so the ApiConnectivityCheck doesn't try to dial peers before tests install handlers
+	apiCheck.ShouldSimulatePeerResponses = true
+
 	err = k8sManager.Add(apiCheck)
 	Expect(err).ToNot(HaveOccurred())
 

controllers/tests/shared/shared.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net"
+	"sync"
 	"time"
 
 	"github.com/google/uuid"
@@ -57,11 +58,11 @@ type K8sClientWrapper struct {
 }
 
 type ApiConnectivityCheckWrapper struct {
-	apicheck.ApiConnectivityCheck
+	*apicheck.ApiConnectivityCheck
 	ShouldSimulatePeerResponses bool
 
-	// store responses that we should override for any peer responses
-	SimulatePeerResponses []selfNodeRemediation.HealthCheckResponseCode
+	responsesMu            sync.Mutex
+	simulatedPeerResponses []selfNodeRemediation.HealthCheckResponseCode
 }
 
 func (kcw *K8sClientWrapper) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) (err error) {
@@ -107,29 +108,75 @@ func GetRandomIpAddress() (randomIP string) {
 }
 
 func NewApiConnectivityCheckWrapper(ck *apicheck.ApiConnectivityCheckConfig, controlPlaneManager *controlplane.Manager) (ckw *ApiConnectivityCheckWrapper) {
+	inner := apicheck.New(ck, controlPlaneManager)
 	ckw = &ApiConnectivityCheckWrapper{
-		ApiConnectivityCheck:        *apicheck.New(ck, controlPlaneManager),
+		ApiConnectivityCheck:        inner,
 		ShouldSimulatePeerResponses: false,
-		SimulatePeerResponses:       []selfNodeRemediation.HealthCheckResponseCode{},
+		simulatedPeerResponses:      []selfNodeRemediation.HealthCheckResponseCode{},
 	}
 
-	ckw.ApiConnectivityCheck.SetHealthStatusFunc(func(endpointIp corev1.PodIP, results chan<- selfNodeRemediation.HealthCheckResponseCode) {
-		switch {
-		case ckw.ShouldSimulatePeerResponses:
-			for _, code := range ckw.SimulatePeerResponses {
-				results <- code
-			}
-
+	inner.SetHealthStatusFunc(func(endpointIp corev1.PodIP, results chan<- selfNodeRemediation.HealthCheckResponseCode) {
+		if ckw.ShouldSimulatePeerResponses {
+			results <- ckw.nextSimulatedPeerResponse()
 			return
-		default:
-			ckw.ApiConnectivityCheck.GetDefaultPeerHealthCheckFunc()(endpointIp, results)
-			break
 		}
+
+		ckw.GetDefaultPeerHealthCheckFunc()(endpointIp, results)
 	})
 
 	return
 }
 
+func (ckw *ApiConnectivityCheckWrapper) nextSimulatedPeerResponse() selfNodeRemediation.HealthCheckResponseCode {
+	ckw.responsesMu.Lock()
+	defer ckw.responsesMu.Unlock()
+
+	if len(ckw.simulatedPeerResponses) == 0 {
+		return selfNodeRemediation.RequestFailed
+	}
+
+	code := ckw.simulatedPeerResponses[0]
+	if len(ckw.simulatedPeerResponses) > 1 {
+		ckw.simulatedPeerResponses = append([]selfNodeRemediation.HealthCheckResponseCode{}, ckw.simulatedPeerResponses[1:]...)
+	}
+
+	return code
+}
+
+func (ckw *ApiConnectivityCheckWrapper) AppendSimulatedPeerResponse(code selfNodeRemediation.HealthCheckResponseCode) {
+	ckw.responsesMu.Lock()
+	defer ckw.responsesMu.Unlock()
+	ckw.simulatedPeerResponses = append(ckw.simulatedPeerResponses, code)
+}
+
+func (ckw *ApiConnectivityCheckWrapper) ClearSimulatedPeerResponses() {
+	ckw.responsesMu.Lock()
+	defer ckw.responsesMu.Unlock()
+	ckw.simulatedPeerResponses = nil
+}
+
+func (ckw *ApiConnectivityCheckWrapper) SnapshotSimulatedPeerResponses() []selfNodeRemediation.HealthCheckResponseCode {
+	ckw.responsesMu.Lock()
+	defer ckw.responsesMu.Unlock()
+	if len(ckw.simulatedPeerResponses) == 0 {
+		return nil
+	}
+	snapshot := make([]selfNodeRemediation.HealthCheckResponseCode, len(ckw.simulatedPeerResponses))
+	copy(snapshot, ckw.simulatedPeerResponses)
+	return snapshot
+}
+
+func (ckw *ApiConnectivityCheckWrapper) RestoreSimulatedPeerResponses(codes []selfNodeRemediation.HealthCheckResponseCode) {
+	ckw.responsesMu.Lock()
+	defer ckw.responsesMu.Unlock()
+	if len(codes) == 0 {
+		ckw.simulatedPeerResponses = nil
+		return
+	}
+	ckw.simulatedPeerResponses = make([]selfNodeRemediation.HealthCheckResponseCode, len(codes))
+	copy(ckw.simulatedPeerResponses, codes)
+}
+
 func GenerateTestConfig() *selfnoderemediationv1alpha1.SelfNodeRemediationConfig {
 	return &selfnoderemediationv1alpha1.SelfNodeRemediationConfig{
 		TypeMeta: metav1.TypeMeta{

pkg/apicheck/check.go

@@ -230,15 +230,22 @@ func (c *ApiConnectivityCheck) getPeersResponse(role peers.Role) peers.Response
 		c.config.Log.Info("Ignoring api-server error, error count below threshold", "current count", c.errorCount, "threshold", c.config.MaxErrorsThreshold)
 		return peers.Response{IsHealthy: true, Reason: peers.HealthyBecauseErrorsThresholdNotReached}
 	}
-	c.config.Log.Info("Error count was above threshold, we will continue and attempt to get the addressess" +
-		" for our peers, I consider myself a WORKER at the moment")
+	roleName := "worker"
+	if role == peers.ControlPlane {
+		roleName = "control-plane"
+	}
+
+	c.config.Log.Info("Error count was above threshold, attempting to collect peer addresses",
+		"role", roleName)
 
-	// MES: This gets called even if the current node is a control plane node.  Hopefully
-	//	in an actual environment it is returning actual worker peers
+	// MES: This gets called even if the current node is a control plane node. Hopefully
+	// in an actual environment it is returning actual worker peers when the role is worker.
 	peersToAsk := c.config.Peers.GetPeersAddresses(role)
 
-	c.config.Log.Info("Error count exceeds threshold, trying to ask other peer nodes if I'm healthy",
-		"minPeersRequired", c.config.MinPeersForRemediation, "actualNumPeersFound", len(peersToAsk))
+	c.config.Log.Info("Error count exceeds threshold, querying peer nodes for health",
+		"role", roleName,
+		"minPeersRequired", c.config.MinPeersForRemediation,
+		"actualNumPeersFound", len(peersToAsk))
 
 	// We check to see if we have at least the number of peers that the user has configured as required.
 	//  If we don't have this many peers (for instance there are zero peers, and the default value is set