Merge pull request #238 from novasbc/configurable_minimum_worker_nodecount_2024-10-02

openshift-merge-bot[bot] · web-flow · commit 2cc3c1bdbc65 · 2025-01-30T14:06:20.000Z
Configurable minimum worker nodecount
diff --git a/.gitignore b/.gitignore
@@ -19,6 +19,7 @@ bin
 
 # editor and IDE paraphernalia
 .idea
+*.iml
 *.swp
 *.swo
 *~
diff --git a/Makefile b/Makefile
@@ -160,7 +160,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust
 .PHONY: generate
 generate: controller-gen protoc ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. Also generate protoc / gRPC code
 	$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..."
-	PATH=$(PATH):$(shell pwd)/bin/proto/bin && $(PROTOC) --go_out=. --go-grpc_out=. pkg/peerhealth/peerhealth.proto
+	PATH='$(PATH)':$(shell pwd)/bin/proto/bin && $(PROTOC) --go_out=. --go-grpc_out=. pkg/peerhealth/peerhealth.proto
 
 .PHONY: fmt
 fmt: ## Run go fmt against code.
diff --git a/api/v1alpha1/selfnoderemediationconfig_types.go b/api/v1alpha1/selfnoderemediationconfig_types.go
@@ -28,6 +28,7 @@ const (
 	ConfigCRName                   = "self-node-remediation-config"
 	defaultWatchdogPath            = "/dev/watchdog"
 	defaultIsSoftwareRebootEnabled = true
+	defaultMinPeersForRemediation  = 1
 )
 
 // SelfNodeRemediationConfigSpec defines the desired state of SelfNodeRemediationConfig
@@ -127,6 +128,15 @@ type SelfNodeRemediationConfigSpec struct {
 	// CustomDsTolerations allows to add custom tolerations snr agents that are running on the ds in order to support remediation for different types of nodes.
 	// +optional
 	CustomDsTolerations []v1.Toleration `json:"customDsTolerations,omitempty"`
+
+	// Minimum number of peer workers/control nodes to attempt to contact before deciding if node is unhealthy or not
+	//	if set to zero, no other peers will be required to be present for remediation action to occur when this
+	//	node has lost API server access.  If an insufficient number of peers are found, we will not attempt to ask
+	//	any peer nodes (if present) whether they see that the current node has been marked unhealthy with a
+	//	SelfNodeRemediation CR
+	// +kubebuilder:default:=1
+	// +kubebuilder:validation:Minimum=0
+	MinPeersForRemediation int `json:"minPeersForRemediation,omitempty"`
 }
 
 // SelfNodeRemediationConfigStatus defines the observed state of SelfNodeRemediationConfig
diff --git a/bundle/manifests/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml b/bundle/manifests/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml
@@ -127,6 +127,17 @@ spec:
                   its peers.
                 minimum: 1
                 type: integer
+              minPeersForRemediation:
+                default: 1
+                description: "Minimum number of peer workers/control nodes to attempt
+                  to contact before deciding if node is unhealthy or not\n\tif set
+                  to zero, no other peers will be required to be present for remediation
+                  action to occur when this\n\tnode has lost API server access.  If
+                  an insufficient number of peers are found, we will not attempt to
+                  ask\n\tany peer nodes (if present) whether they see that the current
+                  node has been marked unhealthy with a\n\tSelfNodeRemediation CR"
+                minimum: 0
+                type: integer
               peerApiServerTimeout:
                 default: 5s
                 description: |-
diff --git a/config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml b/config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml
@@ -125,6 +125,17 @@ spec:
                   its peers.
                 minimum: 1
                 type: integer
+              minPeersForRemediation:
+                default: 1
+                description: "Minimum number of peer workers/control nodes to attempt
+                  to contact before deciding if node is unhealthy or not\n\tif set
+                  to zero, no other peers will be required to be present for remediation
+                  action to occur when this\n\tnode has lost API server access.  If
+                  an insufficient number of peers are found, we will not attempt to
+                  ask\n\tany peer nodes (if present) whether they see that the current
+                  node has been marked unhealthy with a\n\tSelfNodeRemediation CR"
+                minimum: 0
+                type: integer
               peerApiServerTimeout:
                 default: 5s
                 description: |-
diff --git a/controllers/selfnoderemediationconfig_controller.go b/controllers/selfnoderemediationconfig_controller.go
@@ -153,6 +153,7 @@ func (r *SelfNodeRemediationConfigReconciler) syncConfigDaemonSet(ctx context.Co
 	data.Data["PeerRequestTimeout"] = snrConfig.Spec.PeerRequestTimeout.Nanoseconds()
 	data.Data["MaxApiErrorThreshold"] = snrConfig.Spec.MaxApiErrorThreshold
 	data.Data["EndpointHealthCheckUrl"] = snrConfig.Spec.EndpointHealthCheckUrl
+	data.Data["MinPeersForRemediation"] = snrConfig.Spec.MinPeersForRemediation
 	data.Data["HostPort"] = snrConfig.Spec.HostPort
 	data.Data["IsSoftwareRebootEnabled"] = fmt.Sprintf("\"%t\"", snrConfig.Spec.IsSoftwareRebootEnabled)
 
diff --git a/controllers/tests/config/selfnoderemediationconfig_controller_test.go b/controllers/tests/config/selfnoderemediationconfig_controller_test.go
@@ -238,6 +238,7 @@ var _ = Describe("SNR Config Test", func() {
 			Expect(createdConfig.Spec.ApiServerTimeout.Seconds()).To(BeEquivalentTo(5))
 			Expect(createdConfig.Spec.ApiCheckInterval.Seconds()).To(BeEquivalentTo(15))
 			Expect(createdConfig.Spec.PeerUpdateInterval.Seconds()).To(BeEquivalentTo(15 * 60))
+			Expect(createdConfig.Spec.MinPeersForRemediation).To(BeEquivalentTo(1))
 		})
 	})
 
diff --git a/controllers/tests/config/suite_test.go b/controllers/tests/config/suite_test.go
@@ -120,14 +120,16 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 
 	certReader = certificates.NewSecretCertStorage(k8sClient, ctrl.Log.WithName("SecretCertStorage"), shared.Namespace)
+
 	apiConnectivityCheckConfig := &apicheck.ApiConnectivityCheckConfig{
-		Log:                ctrl.Log.WithName("api-check"),
-		MyNodeName:         shared.UnhealthyNodeName,
-		CheckInterval:      shared.ApiCheckInterval,
-		MaxErrorsThreshold: shared.MaxErrorThreshold,
-		Peers:              peers,
-		Cfg:                cfg,
-		CertReader:         certReader,
+		Log:                    ctrl.Log.WithName("api-check"),
+		MyNodeName:             shared.UnhealthyNodeName,
+		CheckInterval:          shared.ApiCheckInterval,
+		MaxErrorsThreshold:     shared.MaxErrorThreshold,
+		MinPeersForRemediation: shared.MinPeersForRemediation,
+		Peers:                  peers,
+		Cfg:                    cfg,
+		CertReader:             certReader,
 	}
 	apiCheck := apicheck.New(apiConnectivityCheckConfig, nil)
 	err = k8sManager.Add(apiCheck)
diff --git a/controllers/tests/controller/selfnoderemediation_controller_test.go b/controllers/tests/controller/selfnoderemediation_controller_test.go
@@ -66,6 +66,10 @@ var _ = Describe("SNR Controller", func() {
 		k8sClient.ShouldSimulateFailure = false
 		k8sClient.ShouldSimulatePodDeleteFailure = false
 		isAdditionalSetupNeeded = false
+
+		By("Restore default settings for api connectivity check")
+		apiConnectivityCheckConfig.MinPeersForRemediation = shared.MinPeersForRemediation
+
 		deleteRemediations()
 		deleteSelfNodeRemediationPod()
 		//clear node's state, this is important to remove taints, label etc.
@@ -451,6 +455,19 @@ var _ = Describe("SNR Controller", func() {
 				verifyWatchdogNotTriggered()
 			})
 		})
+
+		Context("no peer found and MinPeersForRemediation is configured to 0", func() {
+			BeforeEach(func() {
+				By("Set MinPeersForRemedation to zero which should trigger the watchdog before the test")
+				apiConnectivityCheckConfig.MinPeersForRemediation = 0
+			})
+
+			It("Does not receive peer communication and since configured to need zero peers, initiates a reboot",
+				func() {
+					verifyWatchdogTriggered()
+				})
+		})
+
 	})
 
 	Context("Configuration is missing", func() {
diff --git a/controllers/tests/controller/suite_test.go b/controllers/tests/controller/suite_test.go
@@ -53,13 +53,14 @@ import (
 // http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
 
 var (
-	testEnv                 *envtest.Environment
-	dummyDog                watchdog.FakeWatchdog
-	unhealthyNode, peerNode = &v1.Node{}, &v1.Node{}
-	cancelFunc              context.CancelFunc
-	k8sClient               *shared.K8sClientWrapper
-	fakeRecorder            *record.FakeRecorder
-	snrConfig               *selfnoderemediationv1alpha1.SelfNodeRemediationConfig
+	testEnv                    *envtest.Environment
+	dummyDog                   watchdog.FakeWatchdog
+	unhealthyNode, peerNode    = &v1.Node{}, &v1.Node{}
+	cancelFunc                 context.CancelFunc
+	k8sClient                  *shared.K8sClientWrapper
+	fakeRecorder               *record.FakeRecorder
+	snrConfig                  *selfnoderemediationv1alpha1.SelfNodeRemediationConfig
+	apiConnectivityCheckConfig *apicheck.ApiConnectivityCheckConfig
 )
 
 var unhealthyNodeNamespacedName = client.ObjectKey{
@@ -152,14 +153,15 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 
 	rebooter := reboot.NewWatchdogRebooter(dummyDog, ctrl.Log.WithName("rebooter"))
-	apiConnectivityCheckConfig := &apicheck.ApiConnectivityCheckConfig{
-		Log:                ctrl.Log.WithName("api-check"),
-		MyNodeName:         shared.UnhealthyNodeName,
-		CheckInterval:      shared.ApiCheckInterval,
-		MaxErrorsThreshold: shared.MaxErrorThreshold,
-		Peers:              peers,
-		Rebooter:           rebooter,
-		Cfg:                cfg,
+	apiConnectivityCheckConfig = &apicheck.ApiConnectivityCheckConfig{
+		Log:                    ctrl.Log.WithName("api-check"),
+		MyNodeName:             shared.UnhealthyNodeName,
+		CheckInterval:          shared.ApiCheckInterval,
+		MaxErrorsThreshold:     shared.MaxErrorThreshold,
+		Peers:                  peers,
+		Rebooter:               rebooter,
+		Cfg:                    cfg,
+		MinPeersForRemediation: shared.MinPeersForRemediation,
 	}
 	apiCheck := apicheck.New(apiConnectivityCheckConfig, nil)
 	err = k8sManager.Add(apiCheck)
diff --git a/controllers/tests/shared/shared.go b/controllers/tests/shared/shared.go
@@ -17,9 +17,10 @@ import (
 )
 
 const (
-	PeerUpdateInterval = 30 * time.Second
-	ApiCheckInterval   = 1 * time.Second
-	MaxErrorThreshold  = 1
+	PeerUpdateInterval     = 30 * time.Second
+	ApiCheckInterval       = 1 * time.Second
+	MaxErrorThreshold      = 1
+	MinPeersForRemediation = 1
 	// CalculatedRebootDuration is the mock calculator's result
 	CalculatedRebootDuration = 3 * time.Second
 	Namespace                = "self-node-remediation"
@@ -57,7 +58,7 @@ func GenerateTestConfig() *selfnoderemediationv1alpha1.SelfNodeRemediationConfig
 			Name:      selfnoderemediationv1alpha1.ConfigCRName,
 			Namespace: Namespace,
 		},
-		Spec: selfnoderemediationv1alpha1.SelfNodeRemediationConfigSpec{},
+		Spec: selfnoderemediationv1alpha1.SelfNodeRemediationConfigSpec{MinPeersForRemediation: 1},
 	}
 }
 
diff --git a/e2e/self_node_remediation_test.go b/e2e/self_node_remediation_test.go
@@ -152,7 +152,8 @@ var _ = Describe("Self Node Remediation E2E", func() {
 
 				It("should not remediate", func() {
 					utils.CheckNoReboot(context.Background(), k8sClientSet, nodeUnderTest, oldBootTime, testNamespace)
-					checkSnrLogs(nodeUnderTest, []string{"failed to check api server", "Peer told me I'm healthy."}, testStartTime)
+					checkSnrLogs(nodeUnderTest, []string{"failed to check api server", "There is at least one peer " +
+						"who thinks this node healthy"}, testStartTime)
 				})
 			})
 
diff --git a/install/self-node-remediation-deamonset.yaml b/install/self-node-remediation-deamonset.yaml
@@ -72,6 +72,8 @@ spec:
             value: {{.EndpointHealthCheckUrl}}
           - name: HOST_PORT
             value: "{{.HostPort}}"
+          - name: MIN_PEERS_FOR_REMEDIATION
+            value: "{{.MinPeersForRemediation}}"
         image: {{.Image}}
         imagePullPolicy: Always
         volumeMounts:
diff --git a/pkg/apicheck/check.go b/pkg/apicheck/check.go
@@ -50,6 +50,7 @@ type ApiConnectivityCheckConfig struct {
 	PeerRequestTimeout        time.Duration
 	PeerHealthPort            int
 	MaxTimeForNoPeersResponse time.Duration
+	MinPeersForRemediation    int
 }
 
 func New(config *ApiConnectivityCheckConfig, controlPlaneManager *controlplane.Manager) *ApiConnectivityCheck {
@@ -129,12 +130,31 @@ func (c *ApiConnectivityCheck) getWorkerPeersResponse() peers.Response {
 
 	c.config.Log.Info("Error count exceeds threshold, trying to ask other nodes if I'm healthy")
 	peersToAsk := c.config.Peers.GetPeersAddresses(peers.Worker)
-	if peersToAsk == nil || len(peersToAsk) == 0 {
-		c.config.Log.Info("Peers list is empty and / or couldn't be retrieved from server, nothing we can do, so consider the node being healthy")
+
+	// 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
+	//  which requires at least one peer), we don't want to remediate. In this case we have some confusion
+	//  and don't want to remediate a node when we shouldn't.  Note: It would be unusual for MinPeersForRemediation
+	//  to be greater than 1 unless the environment has specific requirements.
+	if len(peersToAsk) < c.config.MinPeersForRemediation {
+		c.config.Log.Info("Ignoring api-server error as we have an insufficient number of peers found, "+
+			"so we aren't going to attempt to contact any to check for a SelfNodeRemediation CR"+
+			" - we will consider it as if there was no CR present & as healthy.", "minPeersRequired",
+			c.config.MinPeersForRemediation, "actualNumPeersFound", len(peersToAsk))
+
 		// TODO: maybe we need to check if this happens too much and reboot
 		return peers.Response{IsHealthy: true, Reason: peers.HealthyBecauseNoPeersWereFound}
 	}
 
+	// If we make it here and there are no peers, we can't proceed because we need at least one peer
+	//  to check.  So it doesn't make sense to continue on - we'll mark as unhealthy and exit fast
+	if len(peersToAsk) == 0 {
+		c.config.Log.Info("Marking node as unhealthy due to being isolated.  We don't have any peers to ask "+
+			"and MinPeersForRemediation must be greater than zero", "minPeersRequired",
+			c.config.MinPeersForRemediation, "actualNumPeersFound", len(peersToAsk))
+		return peers.Response{IsHealthy: false, Reason: peers.UnHealthyBecauseNodeIsIsolated}
+	}
+
 	apiErrorsResponsesSum := 0
 	nrAllPeers := len(peersToAsk)
 	// peersToAsk is being reduced at every iteration, iterate until no peers left to ask
@@ -146,39 +166,57 @@ func (c *ApiConnectivityCheck) getWorkerPeersResponse() peers.Response {
 		if healthyResponses+unhealthyResponses+apiErrorsResponses > 0 {
 			c.timeOfLastPeerResponse = time.Now()
 		}
+		c.config.Log.Info("Aggregate peer health responses", "healthyResponses", healthyResponses,
+			"unhealthyResponses", unhealthyResponses, "apiErrorsResponses", apiErrorsResponses)
 
 		if healthyResponses > 0 {
-			c.config.Log.Info("Peer told me I'm healthy.")
+			c.config.Log.Info("There is at least one peer who thinks this node healthy, so we'll respond "+
+				"with a healthy status", "healthyResponses", healthyResponses, "reason",
+				"peers.HealthyBecauseCRNotFound")
 			c.errorCount = 0
 			return peers.Response{IsHealthy: true, Reason: peers.HealthyBecauseCRNotFound}
 		}
 
 		if unhealthyResponses > 0 {
-			c.config.Log.Info("Peer told me I'm unhealthy!")
+			c.config.Log.Info("I got at least one peer who thinks I'm unhealthy, so we'll respond "+
+				"with unhealthy", "unhealthyResponses", unhealthyResponses, "reason",
+				"peers.UnHealthyBecausePeersResponse")
 			return peers.Response{IsHealthy: false, Reason: peers.UnHealthyBecausePeersResponse}
 		}
 
 		if apiErrorsResponses > 0 {
-			c.config.Log.Info("Peer can't access the api-server")
+			c.config.Log.Info("If you see this, I didn't get any healthy or unhealthy peer responses, "+
+				"instead they told me they can't access the API server either", "apiErrorsResponses",
+				apiErrorsResponses)
 			apiErrorsResponsesSum += apiErrorsResponses
 			// TODO: consider using [m|n]hc.spec.maxUnhealthy instead of 50%
 			if apiErrorsResponsesSum > nrAllPeers/2 { // already reached more than 50% of the peers and all of them returned api error
 				// assuming this is a control plane failure as others can't access api-server as well
-				c.config.Log.Info("More than 50% of the nodes couldn't access the api-server, assuming this is a control plane failure")
+				c.config.Log.Info("More than 50% of the nodes couldn't access the api-server, assuming "+
+					"this is a control plane failure, so we are going to return healthy in that case",
+					"reason", "HealthyBecauseMostPeersCantAccessAPIServer")
 				return peers.Response{IsHealthy: true, Reason: peers.HealthyBecauseMostPeersCantAccessAPIServer}
 			}
 		}
 
 	}
 
+	c.config.Log.Info("We have attempted communication with all known peers, and haven't gotten either: " +
+		"a peer that believes we are healthy, a peer that believes we are unhealthy, or we haven't decided that " +
+		"there is a control plane failure")
+
 	//we asked all peers
 	now := time.Now()
 	// MaxTimeForNoPeersResponse check prevents the node from being considered unhealthy in case of short network outages
 	if now.After(c.timeOfLastPeerResponse.Add(c.config.MaxTimeForNoPeersResponse)) {
-		c.config.Log.Error(fmt.Errorf("failed health check"), "Failed to get health status peers. Assuming unhealthy")
+		c.config.Log.Error(fmt.Errorf("failed health check"), "Failed to get health status peers. "+
+			"Assuming unhealthy", "reason", "UnHealthyBecauseNodeIsIsolated")
 		return peers.Response{IsHealthy: false, Reason: peers.UnHealthyBecauseNodeIsIsolated}
 	} else {
-		c.config.Log.Info("Ignoring no peers response error, time is below threshold for no peers response", "time without peers response (seconds)", now.Sub(c.timeOfLastPeerResponse).Seconds(), "threshold (seconds)", c.config.MaxTimeForNoPeersResponse.Seconds())
+		c.config.Log.Info("Ignoring no peers response error, time is below threshold for no peers response",
+			"time without peers response (seconds)", now.Sub(c.timeOfLastPeerResponse).Seconds(),
+			"threshold (seconds)", c.config.MaxTimeForNoPeersResponse.Seconds(),
+			"reason", "HealthyBecauseNoPeersResponseNotReachedTimeout")
 		return peers.Response{IsHealthy: true, Reason: peers.HealthyBecauseNoPeersResponseNotReachedTimeout}
 	}
 
diff --git a/pkg/controlplane/manager.go b/pkg/controlplane/manager.go
@@ -61,17 +61,34 @@ func (manager *Manager) IsControlPlaneHealthy(workerPeerResponse peers.Response,
 	switch workerPeerResponse.Reason {
 	//reported unhealthy by worker peers
 	case peers.UnHealthyBecausePeersResponse:
+		manager.log.Info("We are deciding the control plane is not healthy because the peer response was UnHealthyBecausePeersResponse")
 		return false
 	case peers.UnHealthyBecauseNodeIsIsolated:
+		manager.log.Info("While trying to determine if the control plane is healthy, the peer response was "+
+			"UnHealthyBecauseNodeIsIsolated, so we are returning true if we could reach other control plane nodes",
+			"canOtherControlPlanesBeReached", canOtherControlPlanesBeReached)
 		return canOtherControlPlanesBeReached
 	//reported healthy by worker peers
 	case peers.HealthyBecauseErrorsThresholdNotReached, peers.HealthyBecauseCRNotFound, peers.HealthyBecauseNoPeersResponseNotReachedTimeout:
+		manager.log.Info("We are deciding that the control plane is healthy because either: "+
+			"HealthyBecauseErrorsThresholdNotReached "+
+			", HealthyBecauseCRNotFound,  or HealthyBecauseNoPeersResponseNotReachedTimeout",
+			"reason", workerPeerResponse.Reason)
 		return true
 	//controlPlane node has connection to most workers, we assume it's not isolated (or at least that the controlPlane node that does not have worker peers quorum will reboot)
 	case peers.HealthyBecauseMostPeersCantAccessAPIServer:
-		return manager.isDiagnosticsPassed()
+		didDiagnosticsPass := manager.isDiagnosticsPassed()
+		manager.log.Info("The peers couldn't access the API server, so we are returning whether "+
+			"diagnostics passed", "didDiagnosticsPass", didDiagnosticsPass)
+		return didDiagnosticsPass
 	case peers.HealthyBecauseNoPeersWereFound:
-		return manager.isDiagnosticsPassed() && canOtherControlPlanesBeReached
+		didDiagnosticsPass := manager.isDiagnosticsPassed()
+
+		manager.log.Info("We couldn't find any peers so we are returning didDiagnosticsPass && "+
+			"canOtherControlPlanesBeReached", "didDiagnosticsPass", didDiagnosticsPass,
+			"canOtherControlPlanesBeReached", canOtherControlPlanesBeReached)
+
+		return didDiagnosticsPass && canOtherControlPlanesBeReached
 
 	default:
 		errorText := "node is considered unhealthy by worker peers for an unknown reason"

-Original file line number
+Diff line change
 # editor and IDE paraphernalia
 .idea
 +*.iml
 *.swp
 *.swo
 *~