Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common-instancetypes: Add DeployCommonInstancetypes featureGate #702

Merged
merged 2 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions api/v1beta2/ssp_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}

Expand All @@ -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"`
0xFelix marked this conversation as resolved.
Show resolved Hide resolved
}

// DataImportCronTemplate defines the template type for DataImportCrons.
Expand Down
7 changes: 6 additions & 1 deletion api/v1beta2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion config/crd/bases/ssp.kubevirt.io_ssps.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 3 additions & 1 deletion data/crd/ssp.kubevirt.io_ssps.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
51 changes: 40 additions & 11 deletions internal/operands/common-instancetypes/reconcile.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,23 +223,28 @@ 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
}

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))
Expand All @@ -249,19 +254,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
Expand All @@ -270,33 +274,58 @@ 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")
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
}
return common.CollectResourceStatus(request, reconcileFuncs...)
}

func isFeatureGateEnabled(request *common.Request) bool {
if request.Instance.Spec.FeatureGates == nil || request.Instance.Spec.FeatureGates.DeployCommonInstancetypes == nil {
0xFelix marked this conversation as resolved.
Show resolved Hide resolved
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)
}
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

Expand Down
108 changes: 108 additions & 0 deletions internal/operands/common-instancetypes/reconcile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you access operand.virtualMachineClusterInstancetypes and operand.virtualMachineClusterPreferences here instead of creating a separate list of resources?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd just be asserting that the resources the operand knows about have been reconciled. Appreciate it's the same code but I wanted to be ensure that the expected resources are reconciled here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

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 {
Expand Down
22 changes: 22 additions & 0 deletions tests/common_instancetypes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
})
0xFelix marked this conversation as resolved.
Show resolved Hide resolved
})
Context("webhook", func() {
DescribeTable("should reject URL", func(URL string) {
Expand Down
7 changes: 5 additions & 2 deletions vendor/kubevirt.io/ssp-operator/api/v1beta2/ssp_types.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading