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

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"k8s.io/apimachinery/pkg/types"
 	"maps"
 	"os"
 	"path"
@@ -24,6 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	kubevirtcorev1 "kubevirt.io/api/core/v1"
+	"kubevirt.io/kubevirt/pkg/apimachinery/patch"
 
 	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
 	"github.com/kubevirt/hyperconverged-cluster-operator/controllers/common"
@@ -888,8 +890,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 +901,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)
@@ -986,3 +1010,18 @@ func hcoCertConfig2KvCertificateRotateStrategy(hcoCertConfig hcov1beta1.HyperCon
 		},
 	}
 }
+
+func getLabelPatch(dest, src map[string]string) ([]byte, error) {
+	const labelPath = "/metadata/labels/"
+	patches := patch.New()
+
+	for k, v := range src {
+		lbl, ok := dest[k]
+		if !ok {
+			patches.AddOption(patch.WithAdd(labelPath+patch.EscapeJSONPointer(k), v))
+		} else if lbl != v {
+			patches.AddOption(patch.WithReplace(labelPath+patch.EscapeJSONPointer(k), v))
+		}
+	}
+	return patches.GeneratePayload()
+}

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

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-08-31 05:04:17"
+    createdAt: "2024-09-02 10:50:10"
     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:

go.mod

@@ -36,12 +36,13 @@ require (
 	k8s.io/apimachinery v0.30.4
 	k8s.io/apiserver v0.30.4
 	k8s.io/client-go v12.0.0+incompatible
-	k8s.io/kube-openapi v0.0.0-20240521193020-835d969ad83a
+	k8s.io/kube-openapi v0.30.0
 	k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0
 	kubevirt.io/api v1.3.1
 	kubevirt.io/application-aware-quota v1.3.0
 	kubevirt.io/containerized-data-importer-api v1.60.1
 	kubevirt.io/controller-lifecycle-operator-sdk/api v0.2.4
+	kubevirt.io/kubevirt v1.3.1
 	kubevirt.io/ssp-operator/api v0.21.1
 	sigs.k8s.io/controller-runtime v0.18.4
 	sigs.k8s.io/controller-tools v0.15.0

go.sum

@@ -44,6 +44,12 @@ github.com/gertd/go-pluralize v0.2.1 h1:M3uASbVjMnTsPb0PNqg+E/24Vwigyo/tvyMTtAlL
 github.com/gertd/go-pluralize v0.2.1/go.mod h1:rbYaKDbsXxmRfr8uygAEKhOWsjyrrqrkHVpZvoOp8zk=
 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 h1:Mn26/9ZMNWSw9C9ERFA1PUxfmGpolnw2v0bKOREu5ew=
 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32/go.mod h1:GIjDIg/heH5DOkXY3YJ/wNhfHsQHoXGjl8G8amsYQ1I=
+github.com/go-kit/kit v0.12.0 h1:e4o3o3IsBfAKQh5Qbbiqyfu97Ku7jrO/JbohvztANh4=
+github.com/go-kit/kit v0.12.0/go.mod h1:lHd+EkCZPIwYItmGDDRdhinkzX2A1sj+M9biaEaizzs=
+github.com/go-kit/log v0.2.1 h1:MRVx0/zhvdseW+Gza6N9rVzU/IVzaeE1SFI4raAhmBU=
+github.com/go-kit/log v0.2.1/go.mod h1:NwTd00d/i8cPZ3xOwwiv2PO5MOcx78fFErGNcVmBjv0=
+github.com/go-logfmt/logfmt v0.6.0 h1:wGYYu3uicYdqXVgoYbvnkrPVXkuLM1p1ifugDMEdRi4=
+github.com/go-logfmt/logfmt v0.6.0/go.mod h1:WYhtIu8zTZfxdn5+rREduYbwxfcBr/Vr6KEVveWlfTs=
 github.com/go-logr/logr v0.2.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU=
 github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
@@ -80,6 +86,8 @@ github.com/gobwas/pool v0.2.1/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6Wezm
 github.com/gobwas/ws v1.2.1/go.mod h1:hRKAFb8wOxFROYNsT1bqfWnhX+b5MFeJM9r2ZSwg/KY=
 github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
 github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
+github.com/golang/glog v1.2.0 h1:uCdmnmatrKCgMBlM4rMuJZWOkPDqdbZPnrMXDY4gI68=
+github.com/golang/glog v1.2.0/go.mod h1:6AhwSGph0fcJtXVM/PEHPqZlFeoLxhs7/t5UDAwmO+w=
 github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE=
 github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
 github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
@@ -481,10 +489,14 @@ kubevirt.io/api v1.3.1 h1:MoTNo/zvDlZ44c2ocXLPln8XTaQOeUodiYbEKrTCqv4=
 kubevirt.io/api v1.3.1/go.mod h1:tCn7VAZktEvymk490iPSMPCmKM9UjbbfH2OsFR/IOLU=
 kubevirt.io/application-aware-quota v1.3.0 h1:19wWg9bWsGGnY5NxIXgsC3MOFS28PK89zE8iLwEb5PU=
 kubevirt.io/application-aware-quota v1.3.0/go.mod h1:DCiwU/Y9ZhzOSz8Enio7+rxxHYLfjW9xP/B4YhhEXiE=
+kubevirt.io/client-go v1.0.0 h1:MMn41j/lFd+lJ7gWn7yuIZYW/aT9fI3bUimAuxAQ+Xk=
+kubevirt.io/client-go v1.0.0/go.mod h1:zvYQ/L5OmTP2Pn7XcawtoR07p8VhgviADWtgdGCniWA=
 kubevirt.io/containerized-data-importer-api v1.60.1 h1:chmxuINvA7TPmIe8LpShCoKPxoegcKjkG9tYboFBs/U=
 kubevirt.io/containerized-data-importer-api v1.60.1/go.mod h1:8mwrkZIdy8j/LmCyKt2wFXbiMavLUIqDaegaIF67CZs=
 kubevirt.io/controller-lifecycle-operator-sdk/api v0.2.4 h1:fZYvD3/Vnitfkx6IJxjLAk8ugnZQ7CXVYcRfkSKmuZY=
 kubevirt.io/controller-lifecycle-operator-sdk/api v0.2.4/go.mod h1:018lASpFYBsYN6XwmA2TIrPCx6e0gviTd/ZNtSitKgc=
+kubevirt.io/kubevirt v1.3.1 h1:wWBfXqaLokfGkn4Gr4BiOCd5rHo3bNfj9YlN27nTZGo=
+kubevirt.io/kubevirt v1.3.1/go.mod h1:5CmvdK0pIwSdTMN5kb4JDMMn2yARqQhuW9fSqZAOA/I=
 kubevirt.io/ssp-operator/api v0.21.1 h1:SFhwep5ISGsfFvgjKnoTC+No/kQGDg6qffzT+sU0FJ4=
 kubevirt.io/ssp-operator/api v0.21.1/go.mod h1:5oWbTQBAnKa7JDqooinhI+SwaVqurHicYtHV7aBgzmY=
 sigs.k8s.io/controller-runtime v0.18.4 h1:87+guW1zhvuPLh1PHybKdYFLU0YJp4FhJRmiHvm5BZw=

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"),

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