From 750bb86730703b5512aed2732cd2495ae3a7ce0f Mon Sep 17 00:00:00 2001 From: Pieszka Date: Tue, 17 Sep 2024 11:59:05 +0200 Subject: [PATCH] simplifing label handling and reshuffle steps logic but doubts remain tests finished field added to model, remove runtime uses field from operations Test done --- cmd/broker/deprovisioning.go | 2 +- internal/model.go | 3 + .../delete_runtime_resource_step.go | 20 ++++--- .../delete_runtime_resource_step_test.go | 59 +++++++++++++++++-- .../process/deprovisioning/remove_runtime.go | 9 ++- .../deprovisioning/remove_runtime_test.go | 50 +++++++++++++--- .../create_runtime_resource_step.go | 7 +-- internal/process/update/upgrade_shoot_step.go | 2 +- 8 files changed, 121 insertions(+), 31 deletions(-) diff --git a/cmd/broker/deprovisioning.go b/cmd/broker/deprovisioning.go index 496f895468..351b618b5b 100644 --- a/cmd/broker/deprovisioning.go +++ b/cmd/broker/deprovisioning.go @@ -62,7 +62,7 @@ func NewDeprovisioningProcessingQueue(ctx context.Context, workersAmount int, de step: deprovisioning.NewCheckGardenerClusterDeletedStep(db.Operations(), cli), }, { - step: deprovisioning.NewRemoveRuntimeStep(db.Operations(), db.Instances(), provisionerClient, cfg.Provisioner.DeprovisioningTimeout, cfg.Broker.KimConfig), + step: deprovisioning.NewRemoveRuntimeStep(db.Operations(), db.Instances(), provisionerClient, cfg.Provisioner.DeprovisioningTimeout), }, { step: deprovisioning.NewCheckRuntimeRemovalStep(db.Operations(), db.Instances(), provisionerClient, cfg.Provisioner.DeprovisioningTimeout), diff --git a/internal/model.go b/internal/model.go index 7339f7dbfe..4f496cc63d 100644 --- a/internal/model.go +++ b/internal/model.go @@ -165,6 +165,9 @@ type Operation struct { LastError kebError.LastError `json:"last_error"` + // Used during KIM integration while deprovisioning - to be removed later on when provisioner not used anymore + KimDeprovisionsOnly bool `json:"kim_deprovisions_only"` + // following fields are not stored in the storage and should be added to the Merge function InputCreator ProvisionerInputCreator `json:"-"` } diff --git a/internal/process/deprovisioning/delete_runtime_resource_step.go b/internal/process/deprovisioning/delete_runtime_resource_step.go index a7dea9479f..8f6bf3554e 100644 --- a/internal/process/deprovisioning/delete_runtime_resource_step.go +++ b/internal/process/deprovisioning/delete_runtime_resource_step.go @@ -12,7 +12,6 @@ import ( "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" @@ -55,14 +54,21 @@ func (step *DeleteRuntimeResourceStep) Run(operation internal.Operation, logger return operation, 0, nil } - runtime := &imv1.Runtime{ - ObjectMeta: v1.ObjectMeta{ - Name: resourceName, - Namespace: resourceNamespace, - }, + var runtime = imv1.Runtime{} + err := step.kcpClient.Get(context.Background(), client.ObjectKey{Name: resourceName, Namespace: resourceNamespace}, &runtime) + if err != nil && !errors.IsNotFound(err) { + logger.Warnf("Unable to read runtime: %s", err) + return step.operationManager.RetryOperation(operation, err.Error(), err, 5*time.Second, 1*time.Minute, logger) + } + controlledByKimOnly := !runtime.IsControlledByProvisioner() + operation, backoff, _ := step.operationManager.UpdateOperation(operation, func(operation *internal.Operation) { + operation.KimDeprovisionsOnly = controlledByKimOnly + }, logger) + if backoff > 0 { + return operation, backoff, nil } - err := step.kcpClient.Delete(context.Background(), runtime) + err = step.kcpClient.Delete(context.Background(), &runtime) // check the error if err != nil { diff --git a/internal/process/deprovisioning/delete_runtime_resource_step_test.go b/internal/process/deprovisioning/delete_runtime_resource_step_test.go index 2fde7db179..b0ab597273 100644 --- a/internal/process/deprovisioning/delete_runtime_resource_step_test.go +++ b/internal/process/deprovisioning/delete_runtime_resource_step_test.go @@ -2,6 +2,7 @@ package deprovisioning import ( "context" + "strconv" "testing" "github.com/kyma-project/kyma-environment-broker/internal/fixture" @@ -23,42 +24,78 @@ func TestDeleteRuntimeResourceStep_RuntimeResourceDoesNotExists(t *testing.T) { err := imv1.AddToScheme(scheme.Scheme) assert.NoError(t, err) kcpClient := fake.NewClientBuilder().Build() - op := fixture.FixDeprovisioningOperationAsOperation(fixOperationID, fixInstanceID) + op := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID) op.RuntimeResourceName = "runtime-name" op.KymaResourceNamespace = "kyma-ns" memoryStorage := storage.NewMemoryStorage() + err = memoryStorage.Operations().InsertDeprovisioningOperation(op) + assert.NoError(t, err) + log := logger.NewLogDummy() // when step := NewDeleteRuntimeResourceStep(memoryStorage.Operations(), kcpClient) - _, backoff, err := step.Run(op, log) + postOperation, backoff, err := step.Run(op.Operation, log) // then assert.NoError(t, err) assert.Zero(t, backoff) assertRuntimeDoesNotExists(t, kcpClient, "kyma-ns", "runtime-name") + + // till provisioner may be involved + assert.False(t, postOperation.KimDeprovisionsOnly) } func TestDeleteRuntimeResourceStep_RuntimeResourceExists(t *testing.T) { // given err := imv1.AddToScheme(scheme.Scheme) assert.NoError(t, err) - op := fixture.FixDeprovisioningOperationAsOperation(fixOperationID, fixInstanceID) + op := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID) op.RuntimeResourceName = "runtime-name" op.KymaResourceNamespace = "kyma-ns" memoryStorage := storage.NewMemoryStorage() - kcpClient := fake.NewClientBuilder().WithRuntimeObjects(fixRuntimeResource("kyma-ns", "runtime-name")).Build() + err = memoryStorage.Operations().InsertDeprovisioningOperation(op) + kcpClient := fake.NewClientBuilder().WithRuntimeObjects(fixRuntimeResourceControlledByProvisioner("kyma-ns", "runtime-name", false)).Build() log := logger.NewLogDummy() // when step := NewDeleteRuntimeResourceStep(memoryStorage.Operations(), kcpClient) - _, backoff, err := step.Run(op, log) + postOperation, backoff, err := step.Run(op.Operation, log) // then assert.NoError(t, err) assert.Zero(t, backoff) assertRuntimeDoesNotExists(t, kcpClient, "kyma-ns", "runtime-name") + + // till provisioner may be involved + assert.True(t, postOperation.KimDeprovisionsOnly) +} + +func TestDeleteRuntimeResourceStep_RuntimeResourceExistsControlledByProvisioner(t *testing.T) { + // given + err := imv1.AddToScheme(scheme.Scheme) + assert.NoError(t, err) + op := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID) + op.RuntimeResourceName = "runtime-name" + op.KymaResourceNamespace = "kyma-ns" + memoryStorage := storage.NewMemoryStorage() + err = memoryStorage.Operations().InsertDeprovisioningOperation(op) + + kcpClient := fake.NewClientBuilder().WithRuntimeObjects(fixRuntimeResourceControlledByProvisioner("kyma-ns", "runtime-name", true)).Build() + log := logger.NewLogDummy() + + // when + step := NewDeleteRuntimeResourceStep(memoryStorage.Operations(), kcpClient) + postOperation, backoff, err := step.Run(op.Operation, log) + + // then + assert.NoError(t, err) + assert.Zero(t, backoff) + assertRuntimeDoesNotExists(t, kcpClient, "kyma-ns", "runtime-name") + + // till provisioner may be involved + assert.False(t, postOperation.KimDeprovisionsOnly) } func assertRuntimeDoesNotExists(t *testing.T, kcpClient client.WithWatch, namespace string, name string) { @@ -75,3 +112,15 @@ func fixRuntimeResource(namespace string, name string) runtime.Object { }, } } + +func fixRuntimeResourceControlledByProvisioner(namespace string, name string, controlledByProvisioner bool) runtime.Object { + return &imv1.Runtime{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{ + imv1.LabelControlledByProvisioner: strconv.FormatBool(controlledByProvisioner), + }, + }, + } +} diff --git a/internal/process/deprovisioning/remove_runtime.go b/internal/process/deprovisioning/remove_runtime.go index 7057a755d1..396fee2581 100644 --- a/internal/process/deprovisioning/remove_runtime.go +++ b/internal/process/deprovisioning/remove_runtime.go @@ -21,16 +21,14 @@ type RemoveRuntimeStep struct { instanceStorage storage.Instances provisionerClient provisioner.Client provisionerTimeout time.Duration - kimConfig broker.KimConfig } -func NewRemoveRuntimeStep(os storage.Operations, is storage.Instances, cli provisioner.Client, provisionerTimeout time.Duration, kimConfig broker.KimConfig) *RemoveRuntimeStep { +func NewRemoveRuntimeStep(os storage.Operations, is storage.Instances, cli provisioner.Client, provisionerTimeout time.Duration) *RemoveRuntimeStep { return &RemoveRuntimeStep{ operationManager: process.NewOperationManager(os), instanceStorage: is, provisionerClient: cli, provisionerTimeout: provisionerTimeout, - kimConfig: kimConfig, } } @@ -39,8 +37,9 @@ func (s *RemoveRuntimeStep) Name() string { } func (s *RemoveRuntimeStep) Run(operation internal.Operation, log logrus.FieldLogger) (internal.Operation, time.Duration, error) { - if s.kimConfig.IsDrivenByKimOnly(broker.PlanNamesMapping[operation.ProvisioningParameters.PlanID]) { - log.Infof("Only KIM is driving the process for plan %s, skipping", broker.PlanNamesMapping[operation.ProvisioningParameters.PlanID]) + + if operation.KimDeprovisionsOnly { + log.Infof("Skipping the step because the runtime %s/%s is not controlled by the provisioner", operation.GetRuntimeResourceName(), operation.GetRuntimeResourceName()) return operation, 0, nil } diff --git a/internal/process/deprovisioning/remove_runtime_test.go b/internal/process/deprovisioning/remove_runtime_test.go index 13bdff3cc5..ff6a84157f 100644 --- a/internal/process/deprovisioning/remove_runtime_test.go +++ b/internal/process/deprovisioning/remove_runtime_test.go @@ -4,8 +4,6 @@ import ( "testing" "time" - "github.com/kyma-project/kyma-environment-broker/internal/broker" - "github.com/kyma-project/kyma-environment-broker/internal/fixture" provisionerAutomock "github.com/kyma-project/kyma-environment-broker/internal/provisioner/automock" "github.com/kyma-project/kyma-environment-broker/internal/storage" @@ -14,17 +12,16 @@ import ( ) func TestRemoveRuntimeStep_Run(t *testing.T) { - t.Run("Should repeat process when deprovisioning call to provisioner succeeded", func(t *testing.T) { + t.Run("Should not repeat process when deprovisioning call to provisioner succeeded", func(t *testing.T) { // given log := logrus.New() memoryStorage := storage.NewMemoryStorage() - kimConfig := broker.KimConfig{ - Enabled: false, - } operation := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID) operation.GlobalAccountID = fixGlobalAccountID operation.RuntimeID = fixRuntimeID + operation.KymaResourceNamespace = "kcp-system" + operation.KimDeprovisionsOnly = false err := memoryStorage.Operations().InsertDeprovisioningOperation(operation) assert.NoError(t, err) @@ -34,7 +31,7 @@ func TestRemoveRuntimeStep_Run(t *testing.T) { provisionerClient := &provisionerAutomock.Client{} provisionerClient.On("DeprovisionRuntime", fixGlobalAccountID, fixRuntimeID).Return(fixProvisionerOperationID, nil) - step := NewRemoveRuntimeStep(memoryStorage.Operations(), memoryStorage.Instances(), provisionerClient, time.Minute, kimConfig) + step := NewRemoveRuntimeStep(memoryStorage.Operations(), memoryStorage.Instances(), provisionerClient, time.Minute) // when entry := log.WithFields(logrus.Fields{"step": "TEST"}) @@ -49,5 +46,44 @@ func TestRemoveRuntimeStep_Run(t *testing.T) { assert.NoError(t, err) assert.Equal(t, instance.RuntimeID, fixRuntimeID) + provisionerClient.AssertNumberOfCalls(t, "DeprovisionRuntime", 1) + + }) + + t.Run("Should not call provisioner", func(t *testing.T) { + // given + log := logrus.New() + memoryStorage := storage.NewMemoryStorage() + + operation := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID) + operation.GlobalAccountID = fixGlobalAccountID + operation.RuntimeID = fixRuntimeID + operation.KymaResourceNamespace = "kcp-system" + operation.KimDeprovisionsOnly = true + err := memoryStorage.Operations().InsertDeprovisioningOperation(operation) + assert.NoError(t, err) + + err = memoryStorage.Instances().Insert(fixInstanceRuntimeStatus()) + assert.NoError(t, err) + + provisionerClient := &provisionerAutomock.Client{} + provisionerClient.On("DeprovisionRuntime", fixGlobalAccountID, fixRuntimeID).Return(fixProvisionerOperationID, nil) + + step := NewRemoveRuntimeStep(memoryStorage.Operations(), memoryStorage.Instances(), provisionerClient, time.Minute) + + // when + entry := log.WithFields(logrus.Fields{"step": "TEST"}) + result, repeat, err := step.Run(operation.Operation, entry) + + // then + assert.NoError(t, err) + assert.Equal(t, 0*time.Second, repeat) + + instance, err := memoryStorage.Instances().GetByID(result.InstanceID) + assert.NoError(t, err) + assert.Equal(t, instance.RuntimeID, fixRuntimeID) + + provisionerClient.AssertNumberOfCalls(t, "DeprovisionRuntime", 0) + }) } diff --git a/internal/process/provisioning/create_runtime_resource_step.go b/internal/process/provisioning/create_runtime_resource_step.go index c08c59b2da..448bde7790 100644 --- a/internal/process/provisioning/create_runtime_resource_step.go +++ b/internal/process/provisioning/create_runtime_resource_step.go @@ -171,11 +171,8 @@ func (s *CreateRuntimeResourceStep) createLabelsForRuntime(operation internal.Op "kyma-project.io/region": region, "operator.kyma-project.io/kyma-name": kymaName, } - if s.kimConfig.ViewOnly && !s.kimConfig.IsDrivenByKimOnly(broker.PlanNamesMapping[operation.ProvisioningParameters.PlanID]) { - labels[imv1.LabelControlledByProvisioner] = "true" - } else { - labels[imv1.LabelControlledByProvisioner] = "false" - } + controlledByProvisioner := s.kimConfig.ViewOnly && !s.kimConfig.IsDrivenByKimOnly(broker.PlanNamesMapping[operation.ProvisioningParameters.PlanID]) + labels[imv1.LabelControlledByProvisioner] = strconv.FormatBool(controlledByProvisioner) return labels } diff --git a/internal/process/update/upgrade_shoot_step.go b/internal/process/update/upgrade_shoot_step.go index 505b11e4d2..124a169541 100644 --- a/internal/process/update/upgrade_shoot_step.go +++ b/internal/process/update/upgrade_shoot_step.go @@ -62,7 +62,7 @@ func (s *UpgradeShootStep) Run(operation internal.Operation, log logrus.FieldLog return s.operationManager.RetryOperation(operation, err.Error(), err, 5*time.Second, 1*time.Minute, log) } if !runtime.IsControlledByProvisioner() { - log.Infof("Skipping provisioning because the runtime is not controlled by the provisioner") + log.Infof("Skipping because the runtime is not controlled by the provisioner") return operation, 0, nil }