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. 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 <nunnatsa@redhat.com>

controllers/operands/kubevirt.go

@@ -19,15 +19,15 @@ 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"
 
-	kubevirtcorev1 "kubevirt.io/api/core/v1"
-
 	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
 	"github.com/kubevirt/hyperconverged-cluster-operator/controllers/common"
 	hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
+	kubevirtcorev1 "kubevirt.io/api/core/v1"
 )
 
 // env vars
@@ -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,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

@@ -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() {

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

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

deploy/index-image/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml

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

deploy/olm-catalog/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.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.13.0-unstable
-    createdAt: "2024-09-02 05:04:33"
+    createdAt: "2024-09-02 12:54:25"
     description: A unified operator deploying and controlling KubeVirt and its supporting
       operators with opinionated defaults
     features.operators.openshift.io/cnf: "false"
@@ -414,6 +414,7 @@ spec:
           - watch
           - create
           - delete
+          - patch
         - apiGroups:
           - admissionregistration.k8s.io
           resources:

pkg/components/components.go

@@ -537,7 +537,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

@@ -11,6 +11,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:cnv-qe@redhat.com][level:system]Dependency objects", Label("PriorityClass"), func() {
 	flag.Parse()