Merge pull request #661 from lyarwood/common-instancetypes-ignore-vir…

…t-operator-resources

common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle

internal/operands/common-instancetypes/reconcile.go

@@ -8,13 +8,15 @@ 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"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"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,37 +319,79 @@ 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).
 				WithAppLabels(operandName, operandComponent).
 				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).
 				WithAppLabels(operandName, operandComponent).
 				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
 }

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)

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