Fix bug with user-defined labels in priorityClass

HCO already supports user-defined labels in priorityClass, but if another change is done to the priorityClass, either a change in a required label, or a change in the spec fields, the user-defined labels are deleted. This is also happens if the other modofication is done in different request. This PR fixes this issue, and makes sure that user-defined labels are stay in place on update of the priorityClass. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
kubevirt · Sep 1, 2024 · 49327c2 · 49327c2
1 parent b878331
commit 49327c2
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 11 deletions.
diff --git a/controllers/operands/kubevirt.go b/controllers/operands/kubevirt.go
@@ -888,8 +888,8 @@ func (kvPriorityClassHooks) updateCr(req *common.HcoRequest, Client client.Clien
 	}
 
 	// at this point we found the object in the cache and we check if something was changed
-	if (pc.Name == found.Name) && (pc.Value == found.Value) &&
-		(pc.Description == found.Description) && hcoutil.CompareLabels(&pc.ObjectMeta, &found.ObjectMeta) {
+	specEquals := (pc.Value == found.Value) && (pc.Description == found.Description)
+	if (pc.Name == found.Name) && specEquals && hcoutil.CompareLabels(&pc.ObjectMeta, &found.ObjectMeta) {
 		return false, false, nil
 	}
 
@@ -899,16 +899,34 @@ func (kvPriorityClassHooks) updateCr(req *common.HcoRequest, Client client.Clien
 		req.Logger.Info("Reconciling an externally updated PriorityClass's Spec to its opinionated values")
 	}
 
-	// something was changed but since we can't patch a priority class object, we remove it
-	err := Client.Delete(req.Ctx, found, &client.DeleteOptions{})
-	if err != nil {
-		return false, false, err
+	// make sure req labels are in place, while allowing user defined labels
+	labels := maps.Clone(found.Labels)
+	if labels == nil {
+		labels = make(map[string]string)
+	}
+	if len(pc.Labels) > 0 {
+		maps.Copy(labels, pc.Labels)
 	}
 
-	// create the new object
-	err = Client.Create(req.Ctx, pc, &client.CreateOptions{})
-	if err != nil {
-		return false, false, err
+	if !specEquals {
+		// something was changed but since we can't patch a priority class object, we remove it
+		err := Client.Delete(req.Ctx, found)
+		if err != nil {
+			return false, false, err
+		}
+
+		// create the new object
+		pc.Labels = labels
+		err = Client.Create(req.Ctx, pc)
+		if err != nil {
+			return false, false, err
+		}
+	} else {
+		found.Labels = labels
+		err := Client.Update(req.Ctx, found)
+		if err != nil {
+			return false, false, err
+		}
 	}
 
 	pc.DeepCopyInto(found)

diff --git a/controllers/operands/kubevirt_test.go b/controllers/operands/kubevirt_test.go
@@ -122,7 +122,7 @@ var _ = Describe("KubeVirt Operand", func() {
 
 		DescribeTable("should return error when there is something wrong", func(initiateErrors func(testClient *commontestutils.HcoTestClient) error) {
 			modifiedResource := NewKubeVirtPriorityClass(hco)
-			modifiedResource.Labels = map[string]string{"foo": "bar"}
+			modifiedResource.Value = 1
 
 			cl := commontestutils.InitClient([]client.Object{modifiedResource})
 			expectedError := initiateErrors(cl)
@@ -166,6 +166,68 @@ var _ = Describe("KubeVirt Operand", func() {
 			}),
 		)
 
+		Context("check labels", func() {
+			It("should add missing labels", func(ctx context.Context) {
+				expectedResource := NewKubeVirtPriorityClass(hco)
+				delete(expectedResource.Labels, hcoutil.AppLabelComponent)
+
+				cl := commontestutils.InitClient([]client.Object{expectedResource})
+				handler := (*genericOperand)(newKvPriorityClassHandler(cl, commontestutils.GetScheme()))
+				res := handler.ensure(req)
+				Expect(res.Err).ToNot(HaveOccurred())
+
+				foundPC := schedulingv1.PriorityClass{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: kvPriorityClass,
+					},
+				}
+
+				Expect(cl.Get(ctx, client.ObjectKeyFromObject(&foundPC), &foundPC)).To(Succeed())
+				Expect(foundPC.Labels).To(HaveKeyWithValue(hcoutil.AppLabelComponent, string(hcoutil.AppComponentCompute)))
+			})
+
+			It("should fix wrong labels", func(ctx context.Context) {
+				expectedResource := NewKubeVirtPriorityClass(hco)
+				expectedResource.Labels[hcoutil.AppLabelComponent] = string(hcoutil.AppComponentStorage)
+
+				cl := commontestutils.InitClient([]client.Object{expectedResource})
+				handler := (*genericOperand)(newKvPriorityClassHandler(cl, commontestutils.GetScheme()))
+				res := handler.ensure(req)
+				Expect(res.Err).ToNot(HaveOccurred())
+
+				foundPC := schedulingv1.PriorityClass{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: kvPriorityClass,
+					},
+				}
+
+				Expect(cl.Get(ctx, client.ObjectKeyFromObject(&foundPC), &foundPC)).To(Succeed())
+				Expect(foundPC.Labels).To(HaveKeyWithValue(hcoutil.AppLabelComponent, string(hcoutil.AppComponentCompute)))
+			})
+
+			It("should keep user-defined labels", func(ctx context.Context) {
+				const customLabel = "custom-label"
+				expectedResource := NewKubeVirtPriorityClass(hco)
+				expectedResource.Labels[customLabel] = "test"
+				expectedResource.Labels[hcoutil.AppLabelComponent] = string(hcoutil.AppComponentStorage)
+
+				cl := commontestutils.InitClient([]client.Object{expectedResource})
+				handler := (*genericOperand)(newKvPriorityClassHandler(cl, commontestutils.GetScheme()))
+				res := handler.ensure(req)
+				Expect(res.Err).ToNot(HaveOccurred())
+
+				foundPC := schedulingv1.PriorityClass{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: kvPriorityClass,
+					},
+				}
+
+				Expect(cl.Get(ctx, client.ObjectKeyFromObject(&foundPC), &foundPC)).To(Succeed())
+				Expect(foundPC.Labels).To(HaveKeyWithValue(customLabel, "test"))
+				Expect(foundPC.Labels).To(HaveKeyWithValue(hcoutil.AppLabelComponent, string(hcoutil.AppComponentCompute)))
+			})
+		})
+
 	})
 
 	Context("KubeVirt", func() {