fix: update condition before calling delete and decrementing counter

controllers/space/space_controller.go

@@ -148,13 +148,17 @@ func (r *Reconciler) ensureNSTemplateSet(ctx context.Context, space *toolchainv1
 
 	// deprovision from space.Status.TargetCluster if needed
 	if space.Status.TargetCluster != "" && space.Spec.TargetCluster != space.Status.TargetCluster {
+		if err := r.setStatusRetargeting(ctx, space); err != nil {
+			logger.Error(err, "error updating status")
+			return norequeue, err
+		}
 		logger.Info("retargeting space", "from_cluster", space.Status.TargetCluster, "to_cluster", space.Spec.TargetCluster)
 		// look-up and delete the NSTemplateSet on the current member cluster
 		if isBeingDeleted, err := r.deleteNSTemplateSetFromCluster(ctx, space, space.Status.TargetCluster); err != nil {
 			return norequeue, r.setStatusRetargetFailed(ctx, space, err)
 		} else if isBeingDeleted {
 			logger.Info("wait while NSTemplateSet is being deleted", "member_cluster", space.Status.TargetCluster)
-			return norequeue, r.setStatusRetargeting(ctx, space)
+			return norequeue, nil
 		} else {
 			logger.Info("resetting 'space.Status.TargetCluster' field")
 			// NSTemplateSet was removed: reset `space.Status.TargetCluster`
@@ -474,6 +478,10 @@ func (r *Reconciler) ensureSpaceDeletion(ctx context.Context, space *toolchainv1
 	logger := log.FromContext(ctx)
 
 	logger.Info("terminating Space")
+	if err := r.setStatusTerminating(ctx, space); err != nil {
+		logger.Error(err, "error updating status")
+		return err
+	}
 	if isBeingDeleted, err := r.deleteNSTemplateSet(ctx, space); err != nil {
 		// space was already provisioned to a cluster
 		// let's not proceed with deletion
@@ -483,11 +491,7 @@ func (r *Reconciler) ensureSpaceDeletion(ctx context.Context, space *toolchainv1
 		}
 		logger.Error(err, "error while deleting NSTemplateSet - ignored since the target cluster in the Status is empty")
 	} else if isBeingDeleted {
-		if err := r.setStatusTerminating(ctx, space); err != nil {
-			logger.Error(err, "error updating status")
-			return err
-		}
-		return nil
+		return nil // wait until the NSTemplateSet is deleted
 	}
 	// Remove finalizer from Space
 	util.RemoveFinalizer(space, toolchainv1alpha1.FinalizerName)

controllers/space/space_controller_test.go

@@ -890,6 +890,41 @@ func TestDeleteSpace(t *testing.T) {
 				HaveSpacesForCluster("member-1", 1).
 				HaveSpacesForCluster("member-2", 0) // space counter is not decremented when there NSTemplateSet deletion is stuck
 		})
+
+		t.Run("unable to update status, it should not change the counter", func(t *testing.T) {
+			// given
+			s := spacetest.NewSpace(test.HostOperatorNs, "oddity",
+				spacetest.WithDeletionTimestamp(),
+				spacetest.WithFinalizer(),
+				spacetest.WithSpecTargetCluster("member-1"),
+				spacetest.WithStatusTargetCluster("member-1"))
+
+			hostClient := test.NewFakeClient(t, s, base1nsTier)
+			hostClient.MockStatusUpdate = mockUpdateSpaceStatusFail(hostClient.Client)
+			nstmplSet := nstemplatetsettest.NewNSTemplateSet("oddity", nstemplatetsettest.WithReadyCondition())
+			member1Client := test.NewFakeClient(t, nstmplSet)
+			member1 := NewMemberClusterWithClient(member1Client, "member-1", corev1.ConditionTrue)
+			member2 := NewMemberClusterWithTenantRole(t, "member-2", corev1.ConditionTrue)
+			ctrl := newReconciler(hostClient, member1, member2)
+			InitializeCounters(t,
+				NewToolchainStatus(WithEmptyMetrics(), WithMember("member-1", WithSpaceCount(1))))
+
+			// when
+			res, err := ctrl.Reconcile(context.TODO(), requestFor(s))
+
+			// then
+			require.EqualError(t, err, "mock error")
+			assert.False(t, res.Requeue)
+			spacetest.AssertThatSpace(t, test.HostOperatorNs, s.Name, hostClient).
+				Exists().
+				HasFinalizer().
+				HasSpecTargetCluster("member-1").
+				HasStatusTargetCluster("member-1").
+				HasConditions()
+			AssertThatCountersAndMetrics(t).
+				HaveSpacesForCluster("member-1", 1). // counter is not decremented because of the failed status update
+				HaveSpacesForCluster("member-2", 0)  // space counter is unchanged
+		})
 	})
 }
 
@@ -1925,7 +1960,7 @@ func TestRetargetSpace(t *testing.T) {
 				HaveSpacesForCluster("member-2", 0) // space counter is unchanged when there is an error while deleting NSTemplateSet
 		})
 
-		t.Run("unable to update status", func(t *testing.T) {
+		t.Run("unable to update status, it should not change the counter", func(t *testing.T) {
 			// given
 			s := spacetest.NewSpace(test.HostOperatorNs, "oddity",
 				spacetest.WithFinalizer(),
@@ -1954,7 +1989,7 @@ func TestRetargetSpace(t *testing.T) {
 				HasNoConditions().
 				HasStatusTargetCluster("member-1") // NOT updated
 			AssertThatCountersAndMetrics(t).
-				HaveSpacesForCluster("member-1", 0). // counter is decremented according to the value in status
+				HaveSpacesForCluster("member-1", 1). // counter is not decremented because of the failed status update
 				HaveSpacesForCluster("member-2", 0)  // space counter is unchanged
 		})