diff --git a/controllers/operands/kubevirt.go b/controllers/operands/kubevirt.go index c7aa3d510..da47232d4 100644 --- a/controllers/operands/kubevirt.go +++ b/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) diff --git a/controllers/operands/kubevirt_test.go b/controllers/operands/kubevirt_test.go index 3cb004c6d..6aa963bc1 100644 --- 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() { diff --git a/controllers/operands/labels.go b/controllers/operands/labels.go new file mode 100644 index 000000000..031c0d4aa --- /dev/null +++ b/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) +} diff --git a/controllers/operands/operand.go b/controllers/operands/operand.go index ec3d16bfc..69d2878ce 100644 --- a/controllers/operands/operand.go +++ b/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 { diff --git a/deploy/cluster_role.yaml b/deploy/cluster_role.yaml index d1be1e55d..c43e3e5d4 100644 --- a/deploy/cluster_role.yaml +++ b/deploy/cluster_role.yaml @@ -944,6 +944,7 @@ rules: - watch - create - delete + - patch - apiGroups: - admissionregistration.k8s.io resources: diff --git a/deploy/index-image/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml b/deploy/index-image/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml index bb640cb8b..504adfeb4 100644 --- a/deploy/index-image/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml +++ b/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: diff --git a/deploy/olm-catalog/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml b/deploy/olm-catalog/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml index cb4e83426..c645f0db0 100644 --- a/deploy/olm-catalog/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml +++ b/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: diff --git a/pkg/components/components.go b/pkg/components/components.go index 89c96a447..d1fce7c92 100644 --- a/pkg/components/components.go +++ b/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"), diff --git a/pkg/patch/patch.go b/pkg/patch/patch.go new file mode 100644 index 000000000..2b0d466c0 --- /dev/null +++ b/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") +} diff --git a/tests/func-tests/dependency_objects_test.go b/tests/func-tests/dependency_objects_test.go index a6560b05b..ce857b454 100644 --- a/tests/func-tests/dependency_objects_test.go +++ b/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() diff --git a/tests/func-tests/update_priority_class_test.go b/tests/func-tests/update_priority_class_test.go deleted file mode 100644 index 1cc5414ab..000000000 --- a/tests/func-tests/update_priority_class_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package tests_test - -import ( - "context" - "time" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes" - "sigs.k8s.io/controller-runtime/pkg/client" - - tests "github.com/kubevirt/hyperconverged-cluster-operator/tests/func-tests" -) - -const priorityClassName = "kubevirt-cluster-critical" - -var _ = Describe("check update priorityClass", Ordered, Serial, func() { - var ( - cli client.Client - cliSet *kubernetes.Clientset - oldPriorityClassUID types.UID - ) - - tests.FlagParse() - - BeforeAll(func(ctx context.Context) { - var err error - cli = tests.GetControllerRuntimeClient() - cliSet = tests.GetK8sClientSet() - - pc, err := cliSet.SchedulingV1().PriorityClasses().Get(ctx, priorityClassName, metav1.GetOptions{}) - Expect(err).ToNot(HaveOccurred()) - - Expect(pc.UID).ToNot(BeEmpty()) - oldPriorityClassUID = pc.UID - }) - - It("should have the right reference for the priorityClass in the HyperConverged CR", func(ctx context.Context) { - uid := getPriorityClassHCORef(ctx, cli) - Expect(uid).To(Equal(oldPriorityClassUID)) - }) - - It("should recreate the priorityClass on update", func(ctx context.Context) { - GinkgoWriter.Printf("oldPriorityClassUID: %q\n", oldPriorityClassUID) - // `~1` is the jsonpatch escapoe sequence for `\` - patch := []byte(`[{"op": "replace", "path": "/metadata/labels/app.kubernetes.io~1managed-by", "value": "test"}]`) - - Eventually(func(ctx context.Context) error { - _, err := cliSet.SchedulingV1().PriorityClasses().Patch(ctx, priorityClassName, types.JSONPatchType, patch, metav1.PatchOptions{}) - return err - }).WithTimeout(time.Second * 5).WithPolling(time.Millisecond * 100).WithContext(ctx).Should(Succeed()) - - var newUID types.UID - Eventually(func(g Gomega, ctx context.Context) { - By("make sure a new priority class was created, by checking its UID") - pc, err := cliSet.SchedulingV1().PriorityClasses().Get(ctx, priorityClassName, metav1.GetOptions{}) - g.Expect(err).ToNot(HaveOccurred()) - - newUID = pc.UID - g.Expect(newUID).ToNot(Or(Equal(types.UID("")), Equal(oldPriorityClassUID))) - g.Expect(pc.GetLabels()).ToNot(HaveKey("test")) - }).WithTimeout(30 * time.Second). - WithPolling(100 * time.Millisecond). - WithContext(ctx). - Should(Succeed()) - - GinkgoWriter.Printf("oldPriorityClassUID: %q; newUID: %q\n", oldPriorityClassUID, newUID) - Eventually(func(ctx context.Context) types.UID { - return getPriorityClassHCORef(ctx, cli) - }). - WithTimeout(5 * time.Minute). - WithPolling(time.Second). - WithContext(ctx). - Should(And(Not(BeEmpty()), Equal(newUID))) - }) -}) - -func getPriorityClassHCORef(ctx context.Context, cli client.Client) types.UID { - hc := tests.GetHCO(ctx, cli) - - for _, obj := range hc.Status.RelatedObjects { - if obj.Kind == "PriorityClass" && obj.Name == priorityClassName { - return obj.UID - } - } - return "" -}