Fix error tracking - btpoperator cleanup (#2892)

* Fix error tracking and adjust fake provider

* Fix retryOnError and add test in case of error

* Linter check

Author: piotrek1459

internal/process/deprovisioning/btp_operator_cleanup.go

@@ -55,7 +55,8 @@ func (s *BTPOperatorCleanupStep) Name() string {
 func (s *BTPOperatorCleanupStep) softDelete(operation internal.Operation, k8sClient client.Client, log *slog.Logger) (internal.Operation, time.Duration, error) {
 	namespaces := corev1.NamespaceList{}
 	if err := k8sClient.List(context.Background(), &namespaces); err != nil {
-		return s.retryOnError(operation, nil, err, log, "failed to list namespaces")
+		err = kebError.AsTemporaryError(err, "failed to list namespaces")
+		return s.retryOnError(operation, k8sClient, err, log, "failed to list namespaces")
 	}
 
 	var errors []string
@@ -65,7 +66,7 @@ func (s *BTPOperatorCleanupStep) softDelete(operation internal.Operation, k8sCli
 		return operation, 0, err
 	}
 	if SBCrdExists {
-		s.removeResources(k8sClient, gvk, namespaces, errors)
+		errors = s.removeResources(k8sClient, gvk, namespaces, errors)
 	}
 
 	gvk.Kind = btpOperatorServiceInstance
@@ -74,11 +75,13 @@ func (s *BTPOperatorCleanupStep) softDelete(operation internal.Operation, k8sCli
 		return operation, 0, err
 	}
 	if SICrdExists {
-		s.removeResources(k8sClient, gvk, namespaces, errors)
+		errors = s.removeResources(k8sClient, gvk, namespaces, errors)
 	}
 
 	if len(errors) != 0 {
-		return s.retryOnError(operation, nil, fmt.Errorf("%s", strings.Join(errors, ";")), log, "failed to cleanup")
+		err := fmt.Errorf("%s", strings.Join(errors, ";"))
+		err = kebError.AsTemporaryError(err, "failed to cleanup BTP operator resources")
+		return s.retryOnError(operation, k8sClient, err, log, "failed to cleanup")
 	}
 	return operation, 0, nil
 }
@@ -235,7 +238,7 @@ func (s *BTPOperatorCleanupStep) checkCRDExistence(k8sClient client.Client, gvk
 	return true, nil
 }
 
-func (s *BTPOperatorCleanupStep) removeResources(k8sClient client.Client, gvk schema.GroupVersionKind, namespaces corev1.NamespaceList, errors []string) {
+func (s *BTPOperatorCleanupStep) removeResources(k8sClient client.Client, gvk schema.GroupVersionKind, namespaces corev1.NamespaceList, errors []string) []string {
 	for _, ns := range namespaces.Items {
 		obj := &unstructured.Unstructured{}
 		obj.SetGroupVersionKind(gvk)
@@ -246,4 +249,5 @@ func (s *BTPOperatorCleanupStep) removeResources(k8sClient client.Client, gvk sc
 	if err := s.removeFinalizers(k8sClient, namespaces, gvk); err != nil {
 		errors = append(errors, err.Error())
 	}
+	return errors
 }

internal/process/deprovisioning/btp_operator_cleanup_test.go

@@ -362,10 +362,84 @@ func TestBTPOperatorCleanupStep_NoRuntimeID(t *testing.T) {
 	assert.Zero(t, backoff)
 }
 
+func TestBTPOperatorCleanupStep_ErrorCollection(t *testing.T) {
+	log := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{
+		Level: slog.LevelInfo,
+	})).With("step", "TEST")
+
+	t.Run("should collect errors from DeleteAllOf", func(t *testing.T) {
+		// given
+		ms := storage.NewMemoryStorage()
+		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "kyma-system"}}
+
+		scheme := internal.NewSchemeForTests(t)
+		err := apiextensionsv1.AddToScheme(scheme)
+		require.NoError(t, err)
+		decoder := serializer.NewCodecFactory(scheme).UniversalDeserializer()
+		obj, _, err := decoder.Decode(siCRD, nil, nil)
+		require.NoError(t, err)
+
+		// fake client that returns error on DeleteAllOf
+		k8sCli := &fakeK8sClientWrapper{
+			fake:             fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(obj, ns).Build(),
+			deleteAllOfError: fmt.Errorf("simulated DeleteAllOf error"),
+		}
+
+		op := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID)
+		op.Temporary = true
+		op.ProvisioningParameters.PlanID = broker.TrialPlanID
+		op.UserAgent = broker.AccountCleanupJob
+		op.State = operationStateInProgress
+		step := NewBTPOperatorCleanupStep(ms, kubeconfig.NewFakeK8sClientProvider(k8sCli))
+
+		// when
+		_, backoff, err := step.Run(op.Operation, log)
+
+		// then
+		// handleError consumes the error and returns backoff for retry instead
+		assert.NoError(t, err, "handleError returns nil error, only backoff")
+		assert.NotZero(t, backoff, "should return non-zero backoff for retry on temporary errors")
+		assert.True(t, k8sCli.cleanupInstances, "DeleteAllOf should have been called for ServiceInstances")
+	})
+
+	t.Run("should collect errors from List operation", func(t *testing.T) {
+		// given
+		ms := storage.NewMemoryStorage()
+		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "kyma-system"}}
+
+		scheme := internal.NewSchemeForTests(t)
+		err := apiextensionsv1.AddToScheme(scheme)
+		require.NoError(t, err)
+
+		// fake client that returns error on List
+		k8sCli := &fakeK8sClientWrapper{
+			fake:      fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(ns).Build(),
+			listError: fmt.Errorf("simulated List error"),
+		}
+
+		op := fixture.FixDeprovisioningOperation(fixOperationID, fixInstanceID)
+		op.Temporary = true
+		op.ProvisioningParameters.PlanID = broker.TrialPlanID
+		op.UserAgent = broker.AccountCleanupJob
+		op.State = operationStateInProgress
+		step := NewBTPOperatorCleanupStep(ms, kubeconfig.NewFakeK8sClientProvider(k8sCli))
+
+		// when
+		_, backoff, err := step.Run(op.Operation, log)
+
+		// then
+		// handleError consumes the error and returns backoff for retry instead
+		assert.NoError(t, err, "handleError returns nil error, only backoff")
+		assert.NotZero(t, backoff, "should return non-zero backoff for retry on temporary errors")
+	})
+}
+
 type fakeK8sClientWrapper struct {
 	fake             client.Client
 	cleanupInstances bool
 	cleanupBindings  bool
+	deleteAllOfError error
+	listError        error
 }
 
 func (f *fakeK8sClientWrapper) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error {
@@ -377,6 +451,9 @@ func (f *fakeK8sClientWrapper) Get(ctx context.Context, key client.ObjectKey, ob
 }
 
 func (f *fakeK8sClientWrapper) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	if f.listError != nil {
+		return f.listError
+	}
 	if u, ok := list.(*unstructured.UnstructuredList); ok {
 		switch u.Object["kind"] {
 		case "ServiceBindingList":
@@ -409,8 +486,16 @@ func (f *fakeK8sClientWrapper) DeleteAllOf(ctx context.Context, obj client.Objec
 		switch u.Object["kind"] {
 		case "ServiceBinding":
 			f.cleanupBindings = true
+			if f.deleteAllOfError != nil {
+				return f.deleteAllOfError
+			}
+			return nil
 		case "ServiceInstance":
 			f.cleanupInstances = true
+			if f.deleteAllOfError != nil {
+				return f.deleteAllOfError
+			}
+			return nil
 		}
 	}
 	return f.fake.DeleteAllOf(ctx, obj, opts...)