[release 1.12] Fix bug with user-defined labels in priorityClass (#3246)

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. As part of the fix, HCO
now does not remove and re-create the priorityClass, if only label were
changed, but updates it. As result, the update_priority_class was
removed as it is not relevant anymore.

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

controllers/operands/kubevirt.go

@@ -17,6 +17,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -839,8 +840,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
 	}
 
@@ -850,16 +851,38 @@ 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 {
+		patch, err := getLabelPatch(found.Labels, labels)
+		if err != nil {
+			return false, false, err
+		}
+
+		err = Client.Patch(req.Ctx, found, client.RawPatch(types.JSONPatchType, patch))
+		if err != nil {
+			return false, false, err
+		}
 	}
 
 	pc.DeepCopyInto(found)

controllers/operands/kubevirt_test.go

@@ -73,22 +73,28 @@ var _ = Describe("KubeVirt Operand", func() {
 			Expect(hco.Status.RelatedObjects).To(ContainElement(*objectRef))
 		})
 
-		DescribeTable("should update if something changed", func(modifiedResource *schedulingv1.PriorityClass) {
-			cl := commontestutils.InitClient([]client.Object{modifiedResource})
+		DescribeTable("should update if something changed", func(modifiedPC *schedulingv1.PriorityClass) {
+			expectedPC := NewKubeVirtPriorityClass(hco)
+			key := client.ObjectKeyFromObject(expectedPC)
+
+			cl := commontestutils.InitClient([]client.Object{modifiedPC})
+
+			origPC := &schedulingv1.PriorityClass{}
+			Expect(cl.Get(context.TODO(), key, origPC)).To(Succeed())
+
 			handler := (*genericOperand)(newKvPriorityClassHandler(cl, commontestutils.GetScheme()))
 			res := handler.ensure(req)
 			Expect(res.UpgradeDone).To(BeFalse())
 			Expect(res.Err).ToNot(HaveOccurred())
 
-			expectedResource := NewKubeVirtPriorityClass(hco)
-			key := client.ObjectKeyFromObject(expectedResource)
-			foundResource := &schedulingv1.PriorityClass{}
-			Expect(cl.Get(context.TODO(), key, foundResource)).To(Succeed())
-			Expect(foundResource.Name).To(Equal(expectedResource.Name))
-			Expect(foundResource.Value).To(Equal(expectedResource.Value))
-			Expect(foundResource.GlobalDefault).To(Equal(expectedResource.GlobalDefault))
+			foundPC := &schedulingv1.PriorityClass{}
+			Expect(cl.Get(context.TODO(), key, foundPC)).To(Succeed())
+			Expect(foundPC.Name).To(Equal(expectedPC.Name))
+			Expect(foundPC.Value).To(Equal(expectedPC.Value))
+			Expect(foundPC.GlobalDefault).To(Equal(expectedPC.GlobalDefault))
+			Expect(foundPC.UID).ToNot(Equal(origPC.UID))
 
-			newReference, err := reference.GetReference(cl.Scheme(), foundResource)
+			newReference, err := reference.GetReference(cl.Scheme(), foundPC)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(hco.Status.RelatedObjects).To(ContainElement(*newReference))
 		},
@@ -100,6 +106,7 @@ var _ = Describe("KubeVirt Operand", func() {
 					},
 					ObjectMeta: metav1.ObjectMeta{
 						Name: kvPriorityClass,
+						UID:  "origPC",
 					},
 					Value:         1,
 					GlobalDefault: false,
@@ -113,6 +120,7 @@ var _ = Describe("KubeVirt Operand", func() {
 					},
 					ObjectMeta: metav1.ObjectMeta{
 						Name: kvPriorityClass,
+						UID:  "origPC",
 					},
 					Value:         1000000000,
 					GlobalDefault: true,
@@ -122,7 +130,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)
@@ -165,7 +173,74 @@ var _ = Describe("KubeVirt Operand", func() {
 				return expectedError
 			}),
 		)
+		Context("check labels", func() {
+			const origUID = types.UID("origPC")
+			It("should add missing labels", func(ctx context.Context) {
+				expectedResource := NewKubeVirtPriorityClass(hco)
+				expectedResource.UID = origUID
+				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)))
+				Expect(foundPC.UID).To(Equal(origUID))
+			})
 
+			It("should fix wrong labels", func(ctx context.Context) {
+				expectedResource := NewKubeVirtPriorityClass(hco)
+				expectedResource.UID = "origPC"
+				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)))
+				Expect(foundPC.UID).To(Equal(origUID))
+			})
+
+			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)
+				expectedResource.UID = "origPC"
+
+				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)))
+				Expect(foundPC.UID).To(Equal(origUID))
+			})
+		})
 	})
 
 	Context("KubeVirt", func() {

controllers/operands/labels.go

@@ -0,0 +1,43 @@
+package operands
+
+import (
+	"encoding/json"
+
+	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
+	"github.com/kubevirt/hyperconverged-cluster-operator/pkg/patch"
+	hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
+)
+
+func getLabels(hc *hcov1beta1.HyperConverged, component hcoutil.AppComponent) map[string]string {
+	hcoName := hcov1beta1.HyperConvergedName
+
+	if hc.Name != "" {
+		hcoName = hc.Name
+	}
+
+	return hcoutil.GetLabels(hcoName, component)
+}
+
+func getLabelPatch(dest, src map[string]string) ([]byte, error) {
+	const labelPath = "/metadata/labels/"
+	var patches []patch.JSONPatchAction
+
+	for k, v := range src {
+		op := "replace"
+		lbl, ok := dest[k]
+
+		if !ok {
+			op = "add"
+		} else if lbl == v {
+			continue
+		}
+
+		patches = append(patches, patch.JSONPatchAction{
+			Op:    op,
+			Path:  labelPath + patch.EscapeJSONPointer(k),
+			Value: v,
+		})
+	}
+
+	return json.Marshal(patches)
+}

controllers/operands/operand.go

@@ -386,16 +386,6 @@ func getNamespace(defaultNamespace string, opts []string) string {
 	return defaultNamespace
 }
 
-func getLabels(hc *hcov1beta1.HyperConverged, component hcoutil.AppComponent) map[string]string {
-	hcoName := hcov1beta1.HyperConvergedName
-
-	if hc.Name != "" {
-		hcoName = hc.Name
-	}
-
-	return hcoutil.GetLabels(hcoName, component)
-}
-
 func applyAnnotationPatch(obj runtime.Object, annotation string) error {
 	patches, err := jsonpatch.DecodePatch([]byte(annotation))
 	if err != nil {

deploy/cluster_role.yaml

@@ -948,6 +948,7 @@ rules:
   - watch
   - create
   - delete
+  - patch
 - apiGroups:
   - admissionregistration.k8s.io
   resources:

deploy/index-image/community-kubevirt-hyperconverged/1.12.1/manifests/kubevirt-hyperconverged-operator.v1.12.1.clusterserviceversion.yaml

@@ -432,6 +432,7 @@ spec:
           - watch
           - create
           - delete
+          - patch
         - apiGroups:
           - admissionregistration.k8s.io
           resources:

deploy/olm-catalog/community-kubevirt-hyperconverged/1.12.1/manifests/kubevirt-hyperconverged-operator.v1.12.1.clusterserviceversion.yaml

@@ -9,7 +9,7 @@ metadata:
     certified: "false"
     console.openshift.io/disable-operand-delete: "true"
     containerImage: quay.io/kubevirt/hyperconverged-cluster-operator:1.12.1-unstable
-    createdAt: "2024-12-23 16:44:56"
+    createdAt: "2024-12-26 12:54:15"
     description: A unified operator deploying and controlling KubeVirt and its supporting
       operators with opinionated defaults
     features.operators.openshift.io/cnf: "false"
@@ -432,6 +432,7 @@ spec:
           - watch
           - create
           - delete
+          - patch
         - apiGroups:
           - admissionregistration.k8s.io
           resources:

pkg/components/components.go

@@ -538,7 +538,7 @@ func GetClusterPermissions() []rbacv1.PolicyRule {
 		{
 			APIGroups: stringListToSlice("scheduling.k8s.io"),
 			Resources: stringListToSlice("priorityclasses"),
-			Verbs:     stringListToSlice("get", "list", "watch", "create", "delete"),
+			Verbs:     stringListToSlice("get", "list", "watch", "create", "delete", "patch"),
 		},
 		{
 			APIGroups: stringListToSlice("admissionregistration.k8s.io"),

pkg/patch/patch.go

@@ -0,0 +1,14 @@
+package patch
+
+import "strings"
+
+type JSONPatchAction struct {
+	Op    string      `json:"op"`
+	Path  string      `json:"path"`
+	Value interface{} `json:"value,omitempty"`
+}
+
+func EscapeJSONPointer(ptr string) string {
+	s := strings.ReplaceAll(ptr, "~", "~0")
+	return strings.ReplaceAll(s, "/", "~1")
+}

tests/func-tests/dependency_objects_test.go

@@ -13,6 +13,8 @@ import (
 	tests "github.com/kubevirt/hyperconverged-cluster-operator/tests/func-tests"
 )
 
+const priorityClassName = "kubevirt-cluster-critical"
+
 var _ = Describe("[rfe_id:5672][crit:medium][vendor:[email protected]][level:system]Dependency objects", func() {
 	flag.Parse()