diff --git a/internal/operands/common-instancetypes/reconcile.go b/internal/operands/common-instancetypes/reconcile.go index bc8f44f4e..e37189be4 100644 --- a/internal/operands/common-instancetypes/reconcile.go +++ b/internal/operands/common-instancetypes/reconcile.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + k8serrors "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" @@ -260,7 +262,11 @@ func (c *CommonInstancetypes) reconcileFromURL(request *common.Request) ([]commo // Generate the normal set of reconcile funcs to create or update the provided resources c.virtualMachineClusterInstancetypes = clusterInstancetypesFromURL c.virtualMachineClusterPreferences = clusterPreferencesFromURL - return common.CollectResourceStatus(request, c.reconcileFuncs()...) + reconcileFuncs, err := c.reconcileFuncs(request) + if err != nil { + return nil, err + } + return common.CollectResourceStatus(request, reconcileFuncs...) } func (c *CommonInstancetypes) reconcileFromBundle(request *common.Request) ([]common.ReconcileResult, error) { @@ -277,7 +283,11 @@ func (c *CommonInstancetypes) reconcileFromBundle(request *common.Request) ([]co c.virtualMachineClusterInstancetypes = clusterInstancetypesFromBundle c.virtualMachineClusterPreferences = clusterPreferencesFromBundle - return common.CollectResourceStatus(request, c.reconcileFuncs()...) + reconcileFuncs, err := c.reconcileFuncs(request) + if err != nil { + return nil, err + } + return common.CollectResourceStatus(request, reconcileFuncs...) } func (c *CommonInstancetypes) Reconcile(request *common.Request) ([]common.ReconcileResult, error) { @@ -309,17 +319,31 @@ func (c *CommonInstancetypes) Cleanup(request *common.Request) ([]common.Cleanup return nil, nil } -func (c *CommonInstancetypes) reconcileFuncs() []common.ReconcileFunc { +func (c *CommonInstancetypes) reconcileFuncs(request *common.Request) ([]common.ReconcileFunc, error) { + instancetypeFuncs, err := c.reconcileVirtualMachineClusterInstancetypesFuncs(request) + if err != nil { + return nil, err + } + preferenceFuncs, err := c.reconcileVirtualMachineClusterPreferencesFuncs(request) + if err != nil { + return nil, err + } + funcs := []common.ReconcileFunc{} - funcs = append(funcs, c.reconcileVirtualMachineClusterInstancetypesFuncs()...) - funcs = append(funcs, c.reconcileVirtualMachineClusterPreferencesFuncs()...) - return funcs + funcs = append(funcs, instancetypeFuncs...) + funcs = append(funcs, preferenceFuncs...) + return funcs, nil } -func (c *CommonInstancetypes) reconcileVirtualMachineClusterInstancetypesFuncs() []common.ReconcileFunc { +func (c *CommonInstancetypes) reconcileVirtualMachineClusterInstancetypesFuncs(request *common.Request) ([]common.ReconcileFunc, error) { funcs := make([]common.ReconcileFunc, 0, len(c.virtualMachineClusterInstancetypes)) for i := range c.virtualMachineClusterInstancetypes { clusterInstancetype := &c.virtualMachineClusterInstancetypes[i] + if ignore, err := ignoreObjectOwnedByVirtOperator(request, clusterInstancetype); err != nil { + return nil, err + } else if ignore { + continue + } funcs = append(funcs, func(request *common.Request) (common.ReconcileResult, error) { return common.CreateOrUpdate(request). ClusterResource(clusterInstancetype). @@ -327,13 +351,18 @@ func (c *CommonInstancetypes) reconcileVirtualMachineClusterInstancetypesFuncs() Reconcile() }) } - return funcs + return funcs, nil } -func (c *CommonInstancetypes) reconcileVirtualMachineClusterPreferencesFuncs() []common.ReconcileFunc { +func (c *CommonInstancetypes) reconcileVirtualMachineClusterPreferencesFuncs(request *common.Request) ([]common.ReconcileFunc, error) { funcs := make([]common.ReconcileFunc, 0, len(c.virtualMachineClusterPreferences)) for i := range c.virtualMachineClusterPreferences { clusterPreference := &c.virtualMachineClusterPreferences[i] + if ignore, err := ignoreObjectOwnedByVirtOperator(request, clusterPreference); err != nil { + return nil, err + } else if ignore { + continue + } funcs = append(funcs, func(request *common.Request) (common.ReconcileResult, error) { return common.CreateOrUpdate(request). ClusterResource(clusterPreference). @@ -341,5 +370,28 @@ func (c *CommonInstancetypes) reconcileVirtualMachineClusterPreferencesFuncs() [ Reconcile() }) } - return funcs + return funcs, nil +} + +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. + existing := obj.DeepCopyObject().(client.Object) + if err := request.Client.Get(request.Context, client.ObjectKeyFromObject(existing), existing); err != nil { + if k8serrors.IsNotFound(err) { + return false, nil + } + return true, err + } + + return isOwnedByVirtOperator(existing), 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..573fb82c6 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,55 @@ 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 { + Expect(res.Resource.GetName()).ToNot(Equal(instancetypeToUpdate.Name)) + Expect(res.Resource.GetName()).ToNot(Equal(preferenceToUpdate.Name)) + } + + // 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) {