From 2f3368b20abfe917010d41319ad30b0f1c1971af Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 11 Oct 2023 18:07:55 +0100 Subject: [PATCH 1/2] common-instancetypes: Remove args from reconcileRemovedResources Co-authored-by: Felix Matouschek Signed-off-by: Lee Yarwood --- .../common-instancetypes/reconcile.go | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/internal/operands/common-instancetypes/reconcile.go b/internal/operands/common-instancetypes/reconcile.go index e37189be4..a938e8f25 100644 --- a/internal/operands/common-instancetypes/reconcile.go +++ b/internal/operands/common-instancetypes/reconcile.go @@ -223,17 +223,17 @@ func reconcileRemovedPreferences(request *common.Request, existingResources []in return nil } -func (c *CommonInstancetypes) reconcileRemovedResources(request *common.Request, newInstancetypes []instancetypev1beta1.VirtualMachineClusterInstancetype, newPreferences []instancetypev1beta1.VirtualMachineClusterPreference) error { +func (c *CommonInstancetypes) reconcileRemovedResources(request *common.Request) error { existingClusterInstancetypes, existingClusterPreferences, err := c.fetchExistingResources(request) if err != nil { return err } - if err = reconcileRemovedInstancetypes(request, existingClusterInstancetypes, newInstancetypes); err != nil { + if err = reconcileRemovedInstancetypes(request, existingClusterInstancetypes, c.virtualMachineClusterInstancetypes); err != nil { return err } - if err = reconcileRemovedPreferences(request, existingClusterPreferences, newPreferences); err != nil { + if err = reconcileRemovedPreferences(request, existingClusterPreferences, c.virtualMachineClusterPreferences); err != nil { return err } return nil @@ -249,19 +249,18 @@ func (c *CommonInstancetypes) reconcileFromURL(request *common.Request) ([]commo // Cache the URL so we can check if it changes with future reconcile attempts above c.resourceURL = *request.Instance.Spec.CommonInstancetypes.URL request.Logger.Info(fmt.Sprintf("Reconciling common-instancetypes from URL %s", c.resourceURL)) - clusterInstancetypesFromURL, clusterPreferencesFromURL, err := c.FetchResourcesFromURL(c.resourceURL) + var err error + c.virtualMachineClusterInstancetypes, c.virtualMachineClusterPreferences, err = c.FetchResourcesFromURL(c.resourceURL) if err != nil { return nil, err } // Remove any resources no longer provided by the URL, this should only happen when switching from the internal bundle to external URL for now. - if err = c.reconcileRemovedResources(request, clusterInstancetypesFromURL, clusterPreferencesFromURL); err != nil { + if err = c.reconcileRemovedResources(request); err != nil { return nil, err } // Generate the normal set of reconcile funcs to create or update the provided resources - c.virtualMachineClusterInstancetypes = clusterInstancetypesFromURL - c.virtualMachineClusterPreferences = clusterPreferencesFromURL reconcileFuncs, err := c.reconcileFuncs(request) if err != nil { return nil, err @@ -271,18 +270,17 @@ func (c *CommonInstancetypes) reconcileFromURL(request *common.Request) ([]commo func (c *CommonInstancetypes) reconcileFromBundle(request *common.Request) ([]common.ReconcileResult, error) { request.Logger.Info("Reconciling common-instancetypes from internal bundle") - clusterInstancetypesFromBundle, clusterPreferencesFromBundle, err := c.fetchResourcesFromBundle() + var err error + c.virtualMachineClusterInstancetypes, c.virtualMachineClusterPreferences, err = c.fetchResourcesFromBundle() if err != nil { return nil, err } // Remove any resources no longer provided by the bundle - if err = c.reconcileRemovedResources(request, clusterInstancetypesFromBundle, clusterPreferencesFromBundle); err != nil { + if err = c.reconcileRemovedResources(request); err != nil { return nil, err } - c.virtualMachineClusterInstancetypes = clusterInstancetypesFromBundle - c.virtualMachineClusterPreferences = clusterPreferencesFromBundle reconcileFuncs, err := c.reconcileFuncs(request) if err != nil { return nil, err From 7648d2986cafd2c685de9f32194d1439f3513336 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 11 Oct 2023 16:09:32 +0100 Subject: [PATCH 2/2] common-instancetypes: Add DeployCommonInstancetypes featureGate This change adds a simple DeployCommonInstancetypes feature gate to the SSP CR. This feature gate defaults to true to keep the original behaviour of the common-instancetypes operand. When disabled, any previously deployed resources are removed. Co-authored-by: Felix Matouschek Signed-off-by: Lee Yarwood --- api/v1beta2/ssp_types.go | 7 +- api/v1beta2/zz_generated.deepcopy.go | 7 +- config/crd/bases/ssp.kubevirt.io_ssps.yaml | 6 +- data/crd/ssp.kubevirt.io_ssps.yaml | 4 +- .../common-instancetypes/reconcile.go | 31 +++++ .../common-instancetypes/reconcile_test.go | 108 ++++++++++++++++++ tests/common_instancetypes_test.go | 22 ++++ .../ssp-operator/api/v1beta2/ssp_types.go | 7 +- .../api/v1beta2/zz_generated.deepcopy.go | 7 +- 9 files changed, 191 insertions(+), 8 deletions(-) diff --git a/api/v1beta2/ssp_types.go b/api/v1beta2/ssp_types.go index 63ca358dc..e46d120b2 100644 --- a/api/v1beta2/ssp_types.go +++ b/api/v1beta2/ssp_types.go @@ -90,7 +90,7 @@ type SSPSpec struct { // TektonTasks is the configuration of the tekton-tasks operand TektonTasks *TektonTasks `json:"tektonTasks,omitempty"` - // FeatureGates is the configuration of the tekton operands + // FeatureGates for SSP FeatureGates *FeatureGates `json:"featureGates,omitempty"` } @@ -104,11 +104,14 @@ type TektonTasks struct { Namespace string `json:"namespace,omitempty"` } -// FeatureGates defines feature gate for tto operator +// FeatureGates for SSP type FeatureGates struct { DeployTektonTaskResources bool `json:"deployTektonTaskResources,omitempty"` DeployVmConsoleProxy bool `json:"deployVmConsoleProxy,omitempty"` + + // Enables deployment of the common-instancetypes bundles, defaults to true. + DeployCommonInstancetypes *bool `json:"deployCommonInstancetypes,omitempty"` } // DataImportCronTemplate defines the template type for DataImportCrons. diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index caef6d6a5..4522a6ad3 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -88,6 +88,11 @@ func (in *DataImportCronTemplate) DeepCopy() *DataImportCronTemplate { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *FeatureGates) DeepCopyInto(out *FeatureGates) { *out = *in + if in.DeployCommonInstancetypes != nil { + in, out := &in.DeployCommonInstancetypes, &out.DeployCommonInstancetypes + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FeatureGates. @@ -191,7 +196,7 @@ func (in *SSPSpec) DeepCopyInto(out *SSPSpec) { if in.FeatureGates != nil { in, out := &in.FeatureGates, &out.FeatureGates *out = new(FeatureGates) - **out = **in + (*in).DeepCopyInto(*out) } } diff --git a/config/crd/bases/ssp.kubevirt.io_ssps.yaml b/config/crd/bases/ssp.kubevirt.io_ssps.yaml index 123287d14..e9066ce25 100644 --- a/config/crd/bases/ssp.kubevirt.io_ssps.yaml +++ b/config/crd/bases/ssp.kubevirt.io_ssps.yaml @@ -3911,8 +3911,12 @@ spec: - namespace type: object featureGates: - description: FeatureGates is the configuration of the tekton operands + description: FeatureGates for SSP properties: + deployCommonInstancetypes: + description: Enables deployment of the common-instancetypes bundles, + defaults to true. + type: boolean deployTektonTaskResources: type: boolean deployVmConsoleProxy: diff --git a/data/crd/ssp.kubevirt.io_ssps.yaml b/data/crd/ssp.kubevirt.io_ssps.yaml index 5a6daf207..39b81b5ec 100644 --- a/data/crd/ssp.kubevirt.io_ssps.yaml +++ b/data/crd/ssp.kubevirt.io_ssps.yaml @@ -3912,8 +3912,10 @@ spec: - namespace type: object featureGates: - description: FeatureGates is the configuration of the tekton operands + description: FeatureGates for SSP properties: + deployCommonInstancetypes: + type: boolean deployTektonTaskResources: type: boolean deployVmConsoleProxy: diff --git a/internal/operands/common-instancetypes/reconcile.go b/internal/operands/common-instancetypes/reconcile.go index a938e8f25..c312852d6 100644 --- a/internal/operands/common-instancetypes/reconcile.go +++ b/internal/operands/common-instancetypes/reconcile.go @@ -240,6 +240,11 @@ func (c *CommonInstancetypes) reconcileRemovedResources(request *common.Request) } func (c *CommonInstancetypes) reconcileFromURL(request *common.Request) ([]common.ReconcileResult, error) { + // Handle the featureGate being disabled by ensuring any resources previously reconciled are cleaned up + if !isFeatureGateEnabled(request) { + return c.cleanupReconcile(request) + } + // TODO - In the future we should handle cases where the URL remains the same but the provided resources change. if c.resourceURL != "" && c.resourceURL == *request.Instance.Spec.CommonInstancetypes.URL { request.Logger.Info(fmt.Sprintf("Skipping reconcile of common-instancetypes from URL %s, force with a restart of the service.", *request.Instance.Spec.CommonInstancetypes.URL)) @@ -269,6 +274,11 @@ func (c *CommonInstancetypes) reconcileFromURL(request *common.Request) ([]commo } func (c *CommonInstancetypes) reconcileFromBundle(request *common.Request) ([]common.ReconcileResult, error) { + // Handle the featureGate being disabled by ensuring any resources previously reconciled are cleaned up + if !isFeatureGateEnabled(request) { + return c.cleanupReconcile(request) + } + request.Logger.Info("Reconciling common-instancetypes from internal bundle") var err error c.virtualMachineClusterInstancetypes, c.virtualMachineClusterPreferences, err = c.fetchResourcesFromBundle() @@ -288,6 +298,13 @@ func (c *CommonInstancetypes) reconcileFromBundle(request *common.Request) ([]co return common.CollectResourceStatus(request, reconcileFuncs...) } +func isFeatureGateEnabled(request *common.Request) bool { + if request.Instance.Spec.FeatureGates == nil || request.Instance.Spec.FeatureGates.DeployCommonInstancetypes == nil { + return true + } + return *request.Instance.Spec.FeatureGates.DeployCommonInstancetypes +} + func (c *CommonInstancetypes) Reconcile(request *common.Request) ([]common.ReconcileResult, error) { if request.Instance.Spec.CommonInstancetypes != nil && request.Instance.Spec.CommonInstancetypes.URL != nil { return c.reconcileFromURL(request) @@ -295,6 +312,20 @@ func (c *CommonInstancetypes) Reconcile(request *common.Request) ([]common.Recon return c.reconcileFromBundle(request) } +func (c *CommonInstancetypes) cleanupReconcile(request *common.Request) ([]common.ReconcileResult, error) { + cleanupResults, err := c.Cleanup(request) + if err != nil { + return nil, err + } + var results []common.ReconcileResult + for _, cleanupResult := range cleanupResults { + if !cleanupResult.Deleted { + results = append(results, common.ResourceDeletedResult(cleanupResult.Resource, common.OperationResultDeleted)) + } + } + return results, nil +} + func (c *CommonInstancetypes) Cleanup(request *common.Request) ([]common.CleanupResult, error) { var objects []client.Object diff --git a/internal/operands/common-instancetypes/reconcile_test.go b/internal/operands/common-instancetypes/reconcile_test.go index 573fb82c6..841977ddb 100644 --- a/internal/operands/common-instancetypes/reconcile_test.go +++ b/internal/operands/common-instancetypes/reconcile_test.go @@ -400,6 +400,114 @@ var _ = Describe("Common-Instancetypes operand", func() { ExpectResourceExists(instancetype, request) ExpectResourceExists(preference, request) }) + + It("should not deploy internal bundle resources when featureGate is disabled", func() { + request.Instance.Spec.FeatureGates = &ssp.FeatureGates{ + DeployCommonInstancetypes: pointer.Bool(false), + } + + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + virtualMachineClusterInstancetypes, err := FetchBundleResource[instancetypev1beta1.VirtualMachineClusterInstancetype](instancetypePath) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterInstancetypes).ToNot(BeEmpty()) + + virtualMachineClusterPreferences, err := FetchBundleResource[instancetypev1beta1.VirtualMachineClusterPreference](preferencePath) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterPreferences).ToNot(BeEmpty()) + + assertResoucesDoNotExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + }) + + It("should cleanup internal bundle resources from when featureGate is disabled after being enabled", func() { + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + virtualMachineClusterInstancetypes, err := FetchBundleResource[instancetypev1beta1.VirtualMachineClusterInstancetype](instancetypePath) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterInstancetypes).ToNot(BeEmpty()) + + virtualMachineClusterPreferences, err := FetchBundleResource[instancetypev1beta1.VirtualMachineClusterPreference](preferencePath) + Expect(err).ToNot(HaveOccurred()) + Expect(virtualMachineClusterPreferences).ToNot(BeEmpty()) + + assertResoucesExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + + // Assert that CrdList can see the required CRDs before we Reconcile and Cleanup + Expect(request.CrdList.CrdExists(virtualMachineClusterInstancetypeCrd)).To(BeTrue()) + Expect(request.CrdList.CrdExists(virtualMachineClusterPreferenceCrd)).To(BeTrue()) + + request.Instance.Spec.FeatureGates = &ssp.FeatureGates{ + DeployCommonInstancetypes: pointer.Bool(false), + } + + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + assertResoucesDoNotExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + }) + + It("should not deploy external URI resources resources when featureGate is disabled", func() { + // Generate a mock ResMap and resources for the test + mockResMap, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences, err := newMockResources(10, 10) + Expect(err).ToNot(HaveOccurred()) + + // Use a mock Run function to return our fake ResMap + operand.KustomizeRunFunc = func(_ filesys.FileSystem, _ string) (resmap.ResMap, error) { + return mockResMap, nil + } + + request.Instance.Spec.FeatureGates = &ssp.FeatureGates{ + DeployCommonInstancetypes: pointer.Bool(false), + } + + // Update the SSP CR to use a URL so that it calls our mock KustomizeRunFunc + request.Instance.Spec.CommonInstancetypes = &ssp.CommonInstancetypes{ + URL: pointer.String("https://foo.com/bar?ref=1"), + } + + // Run Reconcile and assert the results + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + assertResoucesDoNotExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + }) + + It("should cleanup external URI resources from when featureGate is disabled after being enabled", func() { + // Generate a mock ResMap and resources for the test + mockResMap, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences, err := newMockResources(10, 10) + Expect(err).ToNot(HaveOccurred()) + + // Use a mock Run function to return our fake ResMap + operand.KustomizeRunFunc = func(_ filesys.FileSystem, _ string) (resmap.ResMap, error) { + return mockResMap, nil + } + + // Update the SSP CR to use a URL so that it calls our mock KustomizeRunFunc + request.Instance.Spec.CommonInstancetypes = &ssp.CommonInstancetypes{ + URL: pointer.String("https://foo.com/bar?ref=1"), + } + + // Run Reconcile and assert the results + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + assertResoucesExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + + // Assert that CrdList can see the required CRDs before we Reconcile and Cleanup + Expect(request.CrdList.CrdExists(virtualMachineClusterInstancetypeCrd)).To(BeTrue()) + Expect(request.CrdList.CrdExists(virtualMachineClusterPreferenceCrd)).To(BeTrue()) + + request.Instance.Spec.FeatureGates = &ssp.FeatureGates{ + DeployCommonInstancetypes: pointer.Bool(false), + } + + _, err = operand.Reconcile(&request) + Expect(err).ToNot(HaveOccurred()) + + assertResoucesDoNotExist(request, virtualMachineClusterInstancetypes, virtualMachineClusterPreferences) + }) }) func addConversionFunctions(s *runtime.Scheme) error { diff --git a/tests/common_instancetypes_test.go b/tests/common_instancetypes_test.go index fa2972799..abe416017 100644 --- a/tests/common_instancetypes_test.go +++ b/tests/common_instancetypes_test.go @@ -101,6 +101,28 @@ var _ = Describe("Common Instance Types", func() { Expect(apiClient.Get(ctx, client.ObjectKey{Name: preferenceToUpdate.Name}, preferenceToUpdate)).To(Succeed()) Expect(preferenceToUpdate.Spec.CPU).To(Equal(updatedPreferenceCPU)) }) + It("should cleanup resources when feature gate is disabled", func() { + sspObj := getSsp() + sspObj.Spec.FeatureGates = &ssp.FeatureGates{ + DeployCommonInstancetypes: pointer.Bool(false), + } + createOrUpdateSsp(sspObj) + waitUntilDeployed() + + virtualMachineClusterInstancetypes, err := common_instancetypes.FetchBundleResource[instancetypev1beta1.VirtualMachineClusterInstancetype]("../" + common_instancetypes.BundleDir + common_instancetypes.ClusterInstancetypesBundle) + Expect(err).ToNot(HaveOccurred()) + + virtualMachineClusterPreferences, err := common_instancetypes.FetchBundleResource[instancetypev1beta1.VirtualMachineClusterPreference]("../" + common_instancetypes.BundleDir + common_instancetypes.ClusterPreferencesBundle) + Expect(err).ToNot(HaveOccurred()) + + for _, instancetype := range virtualMachineClusterInstancetypes { + Expect(apiClient.Get(ctx, client.ObjectKey{Name: instancetype.Name}, &instancetypev1beta1.VirtualMachineClusterInstancetype{})).ToNot(Succeed()) + } + + for _, preference := range virtualMachineClusterPreferences { + Expect(apiClient.Get(ctx, client.ObjectKey{Name: preference.Name}, &instancetypev1beta1.VirtualMachineClusterPreference{})).ToNot(Succeed()) + } + }) }) Context("webhook", func() { DescribeTable("should reject URL", func(URL string) { diff --git a/vendor/kubevirt.io/ssp-operator/api/v1beta2/ssp_types.go b/vendor/kubevirt.io/ssp-operator/api/v1beta2/ssp_types.go index 63ca358dc..e46d120b2 100644 --- a/vendor/kubevirt.io/ssp-operator/api/v1beta2/ssp_types.go +++ b/vendor/kubevirt.io/ssp-operator/api/v1beta2/ssp_types.go @@ -90,7 +90,7 @@ type SSPSpec struct { // TektonTasks is the configuration of the tekton-tasks operand TektonTasks *TektonTasks `json:"tektonTasks,omitempty"` - // FeatureGates is the configuration of the tekton operands + // FeatureGates for SSP FeatureGates *FeatureGates `json:"featureGates,omitempty"` } @@ -104,11 +104,14 @@ type TektonTasks struct { Namespace string `json:"namespace,omitempty"` } -// FeatureGates defines feature gate for tto operator +// FeatureGates for SSP type FeatureGates struct { DeployTektonTaskResources bool `json:"deployTektonTaskResources,omitempty"` DeployVmConsoleProxy bool `json:"deployVmConsoleProxy,omitempty"` + + // Enables deployment of the common-instancetypes bundles, defaults to true. + DeployCommonInstancetypes *bool `json:"deployCommonInstancetypes,omitempty"` } // DataImportCronTemplate defines the template type for DataImportCrons. diff --git a/vendor/kubevirt.io/ssp-operator/api/v1beta2/zz_generated.deepcopy.go b/vendor/kubevirt.io/ssp-operator/api/v1beta2/zz_generated.deepcopy.go index caef6d6a5..4522a6ad3 100644 --- a/vendor/kubevirt.io/ssp-operator/api/v1beta2/zz_generated.deepcopy.go +++ b/vendor/kubevirt.io/ssp-operator/api/v1beta2/zz_generated.deepcopy.go @@ -88,6 +88,11 @@ func (in *DataImportCronTemplate) DeepCopy() *DataImportCronTemplate { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *FeatureGates) DeepCopyInto(out *FeatureGates) { *out = *in + if in.DeployCommonInstancetypes != nil { + in, out := &in.DeployCommonInstancetypes, &out.DeployCommonInstancetypes + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FeatureGates. @@ -191,7 +196,7 @@ func (in *SSPSpec) DeepCopyInto(out *SSPSpec) { if in.FeatureGates != nil { in, out := &in.FeatureGates, &out.FeatureGates *out = new(FeatureGates) - **out = **in + (*in).DeepCopyInto(*out) } }