Fix issues with high availability detection

* add the missing "watch" permission for nodes. * make the status field a pointer to bool, to distinglish between false and field not set. * On mutation webhook, fix the eviction strategy if SNO becam HA. * fix an issue where the status field is not set if no node is added or deleted. * fix unit test (assert old HCO instead of HCO from client) * fix issue in the node controller, to ignore error if HCO is not present. Signed-off-by: Nahshon Unna-Tsameret <[email protected]>
kubevirt · Dec 15, 2024 · fca9890 · fca9890
1 parent 70de1bc
commit fca9890
Show file tree

Hide file tree

Showing 14 changed files with 148 additions and 88 deletions.
diff --git a/api/v1beta1/hyperconverged_types.go b/api/v1beta1/hyperconverged_types.go
@@ -736,7 +736,7 @@ type HyperConvergedStatus struct {
 	// InfrastructureHighlyAvailable describes whether the cluster has only one worker node
 	// (false) or more (true).
 	// +optional
-	InfrastructureHighlyAvailable bool `json:"infrastructureHighlyAvailable,omitempty"`
+	InfrastructureHighlyAvailable *bool `json:"infrastructureHighlyAvailable,omitempty"`
 }
 
 type Version struct {

diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/controllers/hyperconverged/hyperconverged_controller.go b/controllers/hyperconverged/hyperconverged_controller.go
@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -423,6 +424,9 @@ func (r *ReconcileHyperConverged) doReconcile(req *common.HcoRequest) (reconcile
 	if init {
 		r.eventEmitter.EmitEvent(req.Instance, corev1.EventTypeNormal, "InitHCO", "Initiating the HyperConverged")
 		r.setInitialConditions(req)
+
+		req.Instance.Status.InfrastructureHighlyAvailable = ptr.To(hcoutil.GetClusterInfo().IsInfrastructureHighlyAvailable())
+		req.StatusDirty = true
 	}
 
 	r.setLabels(req)

diff --git a/controllers/nodes/nodes_controller.go b/controllers/nodes/nodes_controller.go
@@ -3,22 +3,22 @@ package nodes
 import (
 	"context"
 
+	operatorhandler "github.com/operator-framework/operator-lib/handler"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/workqueue"
-	"sigs.k8s.io/controller-runtime/pkg/event"
-
-	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
-
-	operatorhandler "github.com/operator-framework/operator-lib/handler"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
+	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
 	hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
 )
 
@@ -113,11 +113,20 @@ func (r *ReconcileNodeCounter) Reconcile(ctx context.Context, _ reconcile.Reques
 	}
 	err = r.client.Get(ctx, hcoKey, hco)
 	if err != nil {
+		if errors.IsNotFound(err) {
+			return reconcile.Result{}, nil
+		}
 		return reconcile.Result{}, err
 	}
 
-	if hco.Status.InfrastructureHighlyAvailable != clusterInfo.IsInfrastructureHighlyAvailable() {
-		hco.Status.InfrastructureHighlyAvailable = clusterInfo.IsInfrastructureHighlyAvailable()
+	if !hco.ObjectMeta.DeletionTimestamp.IsZero() {
+		return reconcile.Result{}, nil
+	}
+
+	if hco.Status.InfrastructureHighlyAvailable == nil ||
+		*hco.Status.InfrastructureHighlyAvailable != clusterInfo.IsInfrastructureHighlyAvailable() {
+
+		hco.Status.InfrastructureHighlyAvailable = ptr.To(clusterInfo.IsInfrastructureHighlyAvailable())
 		err = r.client.Status().Update(ctx, hco)
 		if err != nil {
 			return reconcile.Result{}, err

diff --git a/controllers/nodes/nodes_controller_test.go b/controllers/nodes/nodes_controller_test.go
@@ -37,9 +37,6 @@ var _ = Describe("NodesController", func() {
 	Describe("Reconcile NodesController", func() {
 
 		BeforeEach(func() {
-			hcoutil.GetClusterInfo = func() hcoutil.ClusterInfo {
-				return commontestutils.ClusterInfoMock{}
-			}
 			_ = os.Setenv(hcoutil.OperatorNamespaceEnv, commontestutils.Namespace)
 		})
 
@@ -49,6 +46,9 @@ var _ = Describe("NodesController", func() {
 
 		Context("Node Count Change", func() {
 			It("Should update InfrastructureHighlyAvailable to true if there are two or more worker nodes", func() {
+				hcoutil.GetClusterInfo = func() hcoutil.ClusterInfo {
+					return commontestutils.ClusterInfoMock{}
+				}
 				hco := commontestutils.NewHco()
 				numWorkerNodes := 3
 				var nodesArray []client.Object
@@ -66,8 +66,6 @@ var _ = Describe("NodesController", func() {
 
 				resources := []client.Object{hco, nodesArray[0], nodesArray[1], nodesArray[2]}
 				cl := commontestutils.InitClient(resources)
-				logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)).WithName("nodes_controller_test")
-				Expect(hcoutil.GetClusterInfo().Init(context.TODO(), cl, logger)).To(Succeed())
 				r := &ReconcileNodeCounter{
 					client: cl,
 				}
@@ -82,12 +80,14 @@ var _ = Describe("NodesController", func() {
 					cl.Get(context.TODO(),
 						types.NamespacedName{Name: commontestutils.Name, Namespace: commontestutils.Namespace},
 						latestHCO),
-				).ToNot(HaveOccurred())
+				).To(Succeed())
 
-				Expect(latestHCO.Status.InfrastructureHighlyAvailable).To(BeTrue())
-				Expect(res).To(Equal(reconcile.Result{}))
+				Expect(latestHCO.Status.InfrastructureHighlyAvailable).To(HaveValue(BeTrue()))
 			})
 			It("Should update InfrastructureHighlyAvailable to false if there is only one worker node", func() {
+				hcoutil.GetClusterInfo = func() hcoutil.ClusterInfo {
+					return commontestutils.ClusterInfoSNOMock{}
+				}
 				hco := commontestutils.NewHco()
 				workerNode := &corev1.Node{
 					ObjectMeta: metav1.ObjectMeta{
@@ -99,8 +99,6 @@ var _ = Describe("NodesController", func() {
 				}
 				resources := []client.Object{hco, workerNode}
 				cl := commontestutils.InitClient(resources)
-				logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)).WithName("nodes_controller_test")
-				Expect(hcoutil.GetClusterInfo().Init(context.TODO(), cl, logger)).To(Succeed())
 				r := &ReconcileNodeCounter{
 					client: cl,
 				}
@@ -115,11 +113,34 @@ var _ = Describe("NodesController", func() {
 					cl.Get(context.TODO(),
 						types.NamespacedName{Name: commontestutils.Name, Namespace: commontestutils.Namespace},
 						latestHCO),
-				).ToNot(HaveOccurred())
+				).To(Succeed())
 
-				Expect(hco.Status.InfrastructureHighlyAvailable).To(BeFalse())
+				Expect(latestHCO.Status.InfrastructureHighlyAvailable).To(HaveValue(BeFalse()))
 				Expect(res).To(Equal(reconcile.Result{}))
 			})
+
+			It("Should not return error if the HyperConverged CR is not exist", func() {
+				workerNode := &corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "worker",
+						Labels: map[string]string{
+							"node-role.kubernetes.io/worker": "",
+						},
+					},
+				}
+				resources := []client.Object{workerNode}
+				cl := commontestutils.InitClient(resources)
+				logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)).WithName("nodes_controller_test")
+				Expect(hcoutil.GetClusterInfo().Init(context.TODO(), cl, logger)).To(Succeed())
+				r := &ReconcileNodeCounter{
+					client: cl,
+				}
+
+				// Reconcile to update HCO's status with the correct InfrastructureHighlyAvailable value
+				res, err := r.Reconcile(context.TODO(), request)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(res.Requeue).To(BeFalse())
+			})
 		})
 
 	})

diff --git a/deploy/cluster_role.yaml b/deploy/cluster_role.yaml
@@ -857,6 +857,7 @@ rules:
   verbs:
   - get
   - list
+  - watch
 - apiGroups:
   - ""
   resources:

diff --git a/...rged/1.14.0/manifests/kubevirt-hyperconverged-operator.v1.14.0.clusterserviceversion.yaml b/...rged/1.14.0/manifests/kubevirt-hyperconverged-operator.v1.14.0.clusterserviceversion.yaml
@@ -327,6 +327,7 @@ spec:
           verbs:
           - get
           - list
+          - watch
         - apiGroups:
           - ""
           resources:

diff --git a/...rged/1.14.0/manifests/kubevirt-hyperconverged-operator.v1.14.0.clusterserviceversion.yaml b/...rged/1.14.0/manifests/kubevirt-hyperconverged-operator.v1.14.0.clusterserviceversion.yaml
@@ -9,7 +9,7 @@ metadata:
     certified: "false"
     console.openshift.io/disable-operand-delete: "true"
     containerImage: quay.io/kubevirt/hyperconverged-cluster-operator:1.14.0-unstable
-    createdAt: "2024-12-13 05:05:26"
+    createdAt: "2024-12-15 07:54:32"
     description: A unified operator deploying and controlling KubeVirt and its supporting
       operators with opinionated defaults
     features.operators.openshift.io/cnf: "false"
@@ -327,6 +327,7 @@ spec:
           verbs:
           - get
           - list
+          - watch
         - apiGroups:
           - ""
           resources:

diff --git a/docs/api.md b/docs/api.md
@@ -249,7 +249,7 @@ HyperConvergedStatus defines the observed state of HyperConverged
 | dataImportSchedule | DataImportSchedule is the cron expression that is used in for the hard-coded data import cron templates. HCO generates the value of this field once and stored in the status field, so will survive restart. | string |  | false |
 | dataImportCronTemplates | DataImportCronTemplates is a list of the actual DataImportCronTemplates as HCO update in the SSP CR. The list contains both the common and the custom templates, including any modification done by HCO. | [][DataImportCronTemplateStatus](#dataimportcrontemplatestatus) |  | false |
 | systemHealthStatus | SystemHealthStatus reflects the health of HCO and its secondary resources, based on the aggregated conditions. | string |  | false |
-| infrastructureHighlyAvailable | InfrastructureHighlyAvailable describes whether the cluster has only one worker node (false) or more (true). | bool |  | false |
+| infrastructureHighlyAvailable | InfrastructureHighlyAvailable describes whether the cluster has only one worker node (false) or more (true). | *bool |  | false |
 
 [Back to TOC](#table-of-contents)
 

diff --git a/pkg/components/components.go b/pkg/components/components.go
@@ -516,7 +516,7 @@ func GetClusterPermissions() []rbacv1.PolicyRule {
 		{
 			APIGroups: emptyAPIGroup,
 			Resources: stringListToSlice("nodes"),
-			Verbs:     stringListToSlice("get", "list"),
+			Verbs:     stringListToSlice("get", "list", "watch"),
 		},
 		{
 			APIGroups: emptyAPIGroup,

diff --git a/pkg/webhooks/mutator/hyperConvergedMutator.go b/pkg/webhooks/mutator/hyperConvergedMutator.go
@@ -5,8 +5,6 @@ import (
 	"fmt"
 	"net/http"
 
-	hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
-
 	"gomodules.xyz/jsonpatch/v2"
 	admissionv1 "k8s.io/api/admission/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -79,23 +77,7 @@ func (hcm *HyperConvergedMutator) mutateHyperConverged(_ context.Context, req ad
 		}
 	}
 
-	if hc.Spec.EvictionStrategy == nil {
-		ci := hcoutil.GetClusterInfo()
-		if ci.IsInfrastructureHighlyAvailable() {
-			patches = append(patches, jsonpatch.JsonPatchOperation{
-				Operation: "add",
-				Path:      "/spec/evictionStrategy",
-				Value:     kubevirtcorev1.EvictionStrategyLiveMigrate,
-			})
-		} else {
-			patches = append(patches, jsonpatch.JsonPatchOperation{
-				Operation: "add",
-				Path:      "/spec/evictionStrategy",
-				Value:     kubevirtcorev1.EvictionStrategyNone,
-			})
-		}
-
-	}
+	patches = mutateEvictionStrategy(hc, patches)
 
 	if hc.Spec.MediatedDevicesConfiguration != nil {
 		if len(hc.Spec.MediatedDevicesConfiguration.MediatedDevicesTypes) > 0 && len(hc.Spec.MediatedDevicesConfiguration.MediatedDeviceTypes) == 0 { //nolint SA1019
@@ -122,3 +104,28 @@ func (hcm *HyperConvergedMutator) mutateHyperConverged(_ context.Context, req ad
 
 	return admission.Allowed("")
 }
+
+func mutateEvictionStrategy(hc *hcov1beta1.HyperConverged, patches []jsonpatch.JsonPatchOperation) []jsonpatch.JsonPatchOperation {
+	if hc.Status.InfrastructureHighlyAvailable == nil { // New HyperConverged CR
+		return patches
+	}
+
+	var value = kubevirtcorev1.EvictionStrategyNone
+	if hc.Spec.EvictionStrategy == nil {
+		if *hc.Status.InfrastructureHighlyAvailable {
+			value = kubevirtcorev1.EvictionStrategyLiveMigrate
+		}
+	} else if *hc.Status.InfrastructureHighlyAvailable && *hc.Spec.EvictionStrategy == kubevirtcorev1.EvictionStrategyNone {
+		value = kubevirtcorev1.EvictionStrategyLiveMigrate
+	} else {
+		return patches
+	}
+
+	patches = append(patches, jsonpatch.JsonPatchOperation{
+		Operation: "replace",
+		Path:      "/spec/evictionStrategy",
+		Value:     value,
+	})
+
+	return patches
+}
diff --git a/pkg/webhooks/mutator/hyperConvergedMutator_test.go b/pkg/webhooks/mutator/hyperConvergedMutator_test.go
@@ -47,9 +47,11 @@ var _ = Describe("test HyperConverged mutator", func() {
 					Name:      util.HyperConvergedName,
 					Namespace: HcoValidNamespace,
 				},
-				Spec: v1beta1.HyperConvergedSpec{},
+				Spec: v1beta1.HyperConvergedSpec{
+					EvictionStrategy: ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
+				},
 			}
-			cr.Spec.EvictionStrategy = ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate)
+
 			cli = commontestutils.InitClient(nil)
 			mutator = initHCMutator(s, cli)
 		})
@@ -267,13 +269,9 @@ var _ = Describe("test HyperConverged mutator", func() {
 
 			DescribeTable("check EvictionStrategy default", func(SNO bool, strategy *kubevirtcorev1.EvictionStrategy, patches []jsonpatch.JsonPatchOperation) {
 				if SNO {
-					hcoutil.GetClusterInfo = func() hcoutil.ClusterInfo {
-						return &commontestutils.ClusterInfoSNOMock{}
-					}
+					cr.Status.InfrastructureHighlyAvailable = ptr.To(false)
 				} else {
-					hcoutil.GetClusterInfo = func() hcoutil.ClusterInfo {
-						return &commontestutils.ClusterInfoMock{}
-					}
+					cr.Status.InfrastructureHighlyAvailable = ptr.To(true)
 				}
 
 				cr.Spec.EvictionStrategy = strategy
@@ -289,48 +287,52 @@ var _ = Describe("test HyperConverged mutator", func() {
 					true,
 					nil,
 					[]jsonpatch.JsonPatchOperation{jsonpatch.JsonPatchOperation{
-						Operation: "add",
+						Operation: "replace",
 						Path:      "/spec/evictionStrategy",
 						Value:     kubevirtcorev1.EvictionStrategyNone,
 					}},
 				),
 				Entry("should not override EvictionStrategy if set and on SNO - 1",
 					true,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyNone),
+					ptr.To(kubevirtcorev1.EvictionStrategyNone),
 					nil,
 				),
 				Entry("should not override EvictionStrategy if set and on SNO - 2",
 					true,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyLiveMigrate),
+					ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
 					nil,
 				),
 				Entry("should not override EvictionStrategy if set and on SNO - 3",
 					true,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyExternal),
+					ptr.To(kubevirtcorev1.EvictionStrategyExternal),
 					nil,
 				),
 				Entry("should set EvictionStrategyLiveMigrate if not set and not on SNO",
 					false,
 					nil,
 					[]jsonpatch.JsonPatchOperation{jsonpatch.JsonPatchOperation{
-						Operation: "add",
+						Operation: "replace",
 						Path:      "/spec/evictionStrategy",
 						Value:     kubevirtcorev1.EvictionStrategyLiveMigrate,
 					}},
 				),
 				Entry("should not override EvictionStrategy if set and not on SNO - 1",
 					false,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyNone),
-					nil,
+					ptr.To(kubevirtcorev1.EvictionStrategyNone),
+					[]jsonpatch.JsonPatchOperation{jsonpatch.JsonPatchOperation{
+						Operation: "replace",
+						Path:      "/spec/evictionStrategy",
+						Value:     kubevirtcorev1.EvictionStrategyLiveMigrate,
+					}},
 				),
 				Entry("should not override EvictionStrategy if set and not on SNO - 2",
 					false,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyLiveMigrate),
+					ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
 					nil,
 				),
 				Entry("should not override EvictionStrategy if set and not on SNO - 3",
 					false,
-					pointerEvictionStrategy(kubevirtcorev1.EvictionStrategyExternal),
+					ptr.To(kubevirtcorev1.EvictionStrategyExternal),
 					nil,
 				),
 			)
@@ -468,8 +470,3 @@ func initHCMutator(s *runtime.Scheme, testClient client.Client) *HyperConvergedM
 
 	return mutator
 }
-
-func pointerEvictionStrategy(strategy kubevirtcorev1.EvictionStrategy) *kubevirtcorev1.EvictionStrategy {
-	str := strategy
-	return &str
-}
-Original file line number
+Diff line change
@@ Expand Up / @@ -857,6 +857,7 @@ rules: @@
       verbs:
       - get
       - list
+      - watch
     - apiGroups:
       - ""
       resources:
@@ Expand Down @@