From 434d4c5c9b854ff7863dd810a0d88238cd8fe2d6 Mon Sep 17 00:00:00 2001 From: Felix Matouschek Date: Tue, 24 Oct 2023 13:23:12 +0200 Subject: [PATCH] common-instancetypes: Ignore objects owned by virt-operator With this change the common-instancetypes operand ignores resources that are labelled as being owned by KubeVirt's virt-operator during reconciliation. Co-authored-by: Lee Yarwood Signed-off-by: Felix Matouschek --- internal/common/resource.go | 1 + .../common-instancetypes/reconcile.go | 36 +++++++++++++ .../common-instancetypes/reconcile_test.go | 51 +++++++++++++++++++ tests/common_instancetypes_test.go | 41 +++++++++++++++ 4 files changed, 129 insertions(+) diff --git a/internal/common/resource.go b/internal/common/resource.go index e8a336365..49dd3b240 100644 --- a/internal/common/resource.go +++ b/internal/common/resource.go @@ -28,6 +28,7 @@ const ( OperationResultCreated OperationResult = "created" OperationResultUpdated OperationResult = "updated" OperationResultDeleted OperationResult = "deleted" + OperationResultIgnored OperationResult = "ignored" ) type StatusMessage = *string diff --git a/internal/operands/common-instancetypes/reconcile.go b/internal/operands/common-instancetypes/reconcile.go index bc8f44f4e..55de8c2c1 100644 --- a/internal/operands/common-instancetypes/reconcile.go +++ b/internal/operands/common-instancetypes/reconcile.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/yaml" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kustomize/api/krusty" @@ -15,6 +16,7 @@ import ( "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/kustomize/kyaml/filesys" + virtv1 "kubevirt.io/api/core/v1" instancetypeapi "kubevirt.io/api/instancetype" instancetypev1beta1 "kubevirt.io/api/instancetype/v1beta1" "kubevirt.io/ssp-operator/internal/common" @@ -321,6 +323,12 @@ func (c *CommonInstancetypes) reconcileVirtualMachineClusterInstancetypesFuncs() for i := range c.virtualMachineClusterInstancetypes { clusterInstancetype := &c.virtualMachineClusterInstancetypes[i] funcs = append(funcs, func(request *common.Request) (common.ReconcileResult, error) { + if ignore, err := ignoreObjectOwnedByVirtOperator(request, clusterInstancetype.DeepCopy()); err != nil { + return common.ReconcileResult{}, err + } else if ignore { + return common.ReconcileResult{Resource: clusterInstancetype, OperationResult: common.OperationResultIgnored}, nil + } + return common.CreateOrUpdate(request). ClusterResource(clusterInstancetype). WithAppLabels(operandName, operandComponent). @@ -335,6 +343,12 @@ func (c *CommonInstancetypes) reconcileVirtualMachineClusterPreferencesFuncs() [ for i := range c.virtualMachineClusterPreferences { clusterPreference := &c.virtualMachineClusterPreferences[i] funcs = append(funcs, func(request *common.Request) (common.ReconcileResult, error) { + if ignore, err := ignoreObjectOwnedByVirtOperator(request, clusterPreference.DeepCopy()); err != nil { + return common.ReconcileResult{}, err + } else if ignore { + return common.ReconcileResult{Resource: clusterPreference, OperationResult: common.OperationResultIgnored}, nil + } + return common.CreateOrUpdate(request). ClusterResource(clusterPreference). WithAppLabels(operandName, operandComponent). @@ -343,3 +357,25 @@ func (c *CommonInstancetypes) reconcileVirtualMachineClusterPreferencesFuncs() [ } return funcs } + +func ignoreObjectOwnedByVirtOperator(request *common.Request, obj client.Object) (bool, error) { + // During an upgrade to v0.19.0 we might encounter virt-operator attempting + // to reconcile the same set of common-instancetype resources. Ignore these + // requests to reconcile such objects once owned by virt-operator. + if err := request.Client.Get(request.Context, client.ObjectKeyFromObject(obj), obj); err != nil { + if errors.IsNotFound(err) { + return false, nil + } + return true, err + } + + return isOwnedByVirtOperator(obj), nil +} + +func isOwnedByVirtOperator(obj client.Object) bool { + if obj.GetLabels() == nil { + return false + } + managedValue, managedOk := obj.GetLabels()[virtv1.ManagedByLabel] + return managedOk && managedValue == virtv1.ManagedByLabelOperatorValue +} diff --git a/internal/operands/common-instancetypes/reconcile_test.go b/internal/operands/common-instancetypes/reconcile_test.go index be145a67e..115937160 100644 --- a/internal/operands/common-instancetypes/reconcile_test.go +++ b/internal/operands/common-instancetypes/reconcile_test.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/kio" + virtv1 "kubevirt.io/api/core/v1" instancetypeapi "kubevirt.io/api/instancetype" instancetypev1beta1 "kubevirt.io/api/instancetype/v1beta1" ssp "kubevirt.io/ssp-operator/api/v1beta2" @@ -251,6 +252,56 @@ var _ = Describe("Common-Instancetypes operand", func() { ExpectResourceExists(preference, request) }) + It("should ignore virt-operator owned objects during reconcile when also provided by bundle", func() { + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + instancetypeList := &instancetypev1beta1.VirtualMachineClusterInstancetypeList{} + Expect(request.Client.List(request.Context, instancetypeList, &client.ListOptions{})).To(Succeed()) + Expect(len(instancetypeList.Items) > 0).To(BeTrue()) + + preferenceList := &instancetypev1beta1.VirtualMachineClusterPreferenceList{} + Expect(request.Client.List(request.Context, preferenceList, &client.ListOptions{})).To(Succeed()) + Expect(len(preferenceList.Items) > 0).To(BeTrue()) + + // Mutate the instance type while also adding the labels for virt-operator + instancetypeToUpdate := instancetypeList.Items[0] + updatedCPUGuestCount := instancetypeToUpdate.Spec.CPU.Guest + 1 + instancetypeToUpdate.Spec.CPU.Guest = updatedCPUGuestCount + instancetypeToUpdate.Labels = map[string]string{ + virtv1.ManagedByLabel: virtv1.ManagedByLabelOperatorValue, + } + Expect(request.Client.Update(request.Context, &instancetypeToUpdate, &client.UpdateOptions{})).To(Succeed()) + + // Mutate the preference while also adding the labels for virt-operator + preferenceToUpdate := preferenceList.Items[0] + updatedPreferredCPUTopology := instancetypev1beta1.PreferCores + updatedPreferenceCPU := &instancetypev1beta1.CPUPreferences{ + PreferredCPUTopology: &updatedPreferredCPUTopology, + } + preferenceToUpdate.Spec.CPU = updatedPreferenceCPU + preferenceToUpdate.Labels = map[string]string{ + virtv1.ManagedByLabel: virtv1.ManagedByLabelOperatorValue, + } + Expect(request.Client.Update(request.Context, &preferenceToUpdate, &client.UpdateOptions{})).To(Succeed()) + + results, err := operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + // Assert that we have reported ignoring the attempt to reconcile the objects owned by virt-operator + for _, res := range results { + if res.Resource.GetName() == instancetypeToUpdate.Name || res.Resource.GetName() == preferenceToUpdate.Name { + Expect(res.OperationResult).To(Equal(common.OperationResultIgnored)) + } + } + + // Assert that the mutations made above persist as the reconcile is being ignored + Expect(request.Client.Get(request.Context, client.ObjectKeyFromObject(&instancetypeToUpdate), &instancetypeToUpdate, &client.GetOptions{})).To(Succeed()) + Expect(instancetypeToUpdate.Spec.CPU.Guest).To(Equal(updatedCPUGuestCount)) + Expect(request.Client.Get(request.Context, client.ObjectKeyFromObject(&preferenceToUpdate), &preferenceToUpdate, &client.GetOptions{})).To(Succeed()) + Expect(preferenceToUpdate.Spec.CPU).To(Equal(updatedPreferenceCPU)) + }) + It("should create and cleanup resources from an external URL", func() { // Generate a mock ResMap and resources for the test mockResMap, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences, err := newMockResources(10, 10) diff --git a/tests/common_instancetypes_test.go b/tests/common_instancetypes_test.go index dcd796824..fa2972799 100644 --- a/tests/common_instancetypes_test.go +++ b/tests/common_instancetypes_test.go @@ -4,6 +4,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/utils/pointer" + virtv1 "kubevirt.io/api/core/v1" instancetypev1beta1 "kubevirt.io/api/instancetype/v1beta1" ssp "kubevirt.io/ssp-operator/api/v1beta2" common_instancetypes "kubevirt.io/ssp-operator/internal/operands/common-instancetypes" @@ -60,6 +61,46 @@ var _ = Describe("Common Instance Types", func() { Expect(apiClient.Get(ctx, client.ObjectKey{Name: preference.Name}, &instancetypev1beta1.VirtualMachineClusterPreference{})).To(Succeed()) } }) + It("should ignore resources owned by virt-operator", func() { + virtualMachineClusterInstancetypes, err := common_instancetypes.FetchBundleResource[instancetypev1beta1.VirtualMachineClusterInstancetype]("../" + common_instancetypes.BundleDir + common_instancetypes.ClusterInstancetypesBundle) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterInstancetypes).ToNot(BeEmpty()) + + virtualMachineClusterPreferences, err := common_instancetypes.FetchBundleResource[instancetypev1beta1.VirtualMachineClusterPreference]("../" + common_instancetypes.BundleDir + common_instancetypes.ClusterPreferencesBundle) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterPreferences).ToNot(BeEmpty()) + + // Mutate the preference while also adding the labels for virt-operator + instancetypeToUpdate := &virtualMachineClusterInstancetypes[0] + Expect(apiClient.Get(ctx, client.ObjectKey{Name: instancetypeToUpdate.Name}, instancetypeToUpdate)).To(Succeed()) + updatedCPUGuestCount := instancetypeToUpdate.Spec.CPU.Guest + 1 + instancetypeToUpdate.Spec.CPU.Guest = updatedCPUGuestCount + instancetypeToUpdate.Labels = map[string]string{ + virtv1.ManagedByLabel: virtv1.ManagedByLabelOperatorValue, + } + Expect(apiClient.Update(ctx, instancetypeToUpdate)).To(Succeed()) + + // Mutate the preference while also adding the labels for virt-operator + preferenceToUpdate := &virtualMachineClusterPreferences[0] + Expect(apiClient.Get(ctx, client.ObjectKey{Name: preferenceToUpdate.Name}, preferenceToUpdate)).To(Succeed()) + updatedPreferredCPUTopology := instancetypev1beta1.PreferCores + updatedPreferenceCPU := &instancetypev1beta1.CPUPreferences{ + PreferredCPUTopology: &updatedPreferredCPUTopology, + } + preferenceToUpdate.Spec.CPU = updatedPreferenceCPU + preferenceToUpdate.Labels = map[string]string{ + virtv1.ManagedByLabel: virtv1.ManagedByLabelOperatorValue, + } + Expect(apiClient.Update(ctx, preferenceToUpdate)).To(Succeed()) + + triggerReconciliation() + + // Assert that the mutations made above persist as the reconcile is being ignored + Expect(apiClient.Get(ctx, client.ObjectKey{Name: instancetypeToUpdate.Name}, instancetypeToUpdate)).To(Succeed()) + Expect(instancetypeToUpdate.Spec.CPU.Guest).To(Equal(updatedCPUGuestCount)) + Expect(apiClient.Get(ctx, client.ObjectKey{Name: preferenceToUpdate.Name}, preferenceToUpdate)).To(Succeed()) + Expect(preferenceToUpdate.Spec.CPU).To(Equal(updatedPreferenceCPU)) + }) }) Context("webhook", func() { DescribeTable("should reject URL", func(URL string) {