From 7ac1a8ed03fd705be212ec7cb5bc75ba13e520d7 Mon Sep 17 00:00:00 2001 From: Rafaela Soares Date: Mon, 3 Nov 2025 10:15:16 +0000 Subject: [PATCH 1/3] requested changes --- .../nstemplateset/nstemplateset_controller.go | 17 +++++++++++------ controllers/nstemplateset/status.go | 9 +++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/controllers/nstemplateset/nstemplateset_controller.go b/controllers/nstemplateset/nstemplateset_controller.go index 888423df..783afd17 100644 --- a/controllers/nstemplateset/nstemplateset_controller.go +++ b/controllers/nstemplateset/nstemplateset_controller.go @@ -200,6 +200,17 @@ func (r *Reconciler) deleteNSTemplateSet(ctx context.Context, nsTmplSet *toolcha } spacename := nsTmplSet.GetName() + // delete cluster resources first + err := r.clusterResources.delete(ctx, nsTmplSet) + if err != nil { + return reconcile.Result{}, err + } + + // clear the ClusterResources status after successful deletion + if err := r.status.clearStatusClusterResources(ctx, nsTmplSet); err != nil { + return reconcile.Result{}, r.status.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.status.setStatusTerminatingFailed, err, "failed to clear ClusterResources status") + } + // delete all namespace one by one allDeleted, err := r.namespaces.ensureDeleted(ctx, nsTmplSet) // when err, status Update will not trigger reconcile, sending returning error. @@ -216,12 +227,6 @@ func (r *Reconciler) deleteNSTemplateSet(ctx context.Context, nsTmplSet *toolcha }, nil } - // if no namespace was to be deleted, then we can proceed with the cluster resources associated with the user - err = r.clusterResources.delete(ctx, nsTmplSet) - if err != nil { - return reconcile.Result{}, err - } - // if nothing was to be deleted, then we can remove the finalizer and we're done logger.Info("NSTemplateSet resource is ready to be terminated: all related user namespaces have been marked for deletion") util.RemoveFinalizer(nsTmplSet, toolchainv1alpha1.FinalizerName) diff --git a/controllers/nstemplateset/status.go b/controllers/nstemplateset/status.go index c4f5c2fd..76dbdfa1 100644 --- a/controllers/nstemplateset/status.go +++ b/controllers/nstemplateset/status.go @@ -290,3 +290,12 @@ func (r *statusManager) setStatusTerminatingFailed(ctx context.Context, nsTmplSe Message: message, }) } + +// clearStatusClusterResources sets the ClusterResources status to nil +func (r *statusManager) clearStatusClusterResources(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) error { + if nsTmplSet.Status.ClusterResources != nil { + nsTmplSet.Status.ClusterResources = nil + return r.Client.Status().Update(ctx, nsTmplSet) + } + return nil +} From 60f0271674a86a3d6a40f86c5d85c64f8ea09cc5 Mon Sep 17 00:00:00 2001 From: Rafaela Soares Date: Mon, 3 Nov 2025 16:48:48 +0000 Subject: [PATCH 2/3] add unit tests --- .../nstemplateset_controller_test.go | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index 7f9bccba..e3e42364 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -1215,6 +1215,69 @@ func TestDeleteNSTemplateSet(t *testing.T) { require.Equal(t, controllerruntime.Result{}, result) }) + t.Run("cluster resources status is cleared after successful deletion", func(t *testing.T) { + t.Run("with namespaces - status cleared and NSTemplateSet remains", func(t *testing.T) { + // given an NSTemplateSet resource with cluster resources and namespaces + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) + crq := newClusterResourceQuota(spacename, "advanced") + devNS := newNamespace("advanced", spacename, "dev", withTemplateRefUsingRevision("abcde11")) + r, _ := prepareController(t, nsTmplSet, crq, devNS) + req := newReconcileRequest(namespaceName, spacename) + + // when reconcile is triggered + _, err := r.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + // verify that the cluster resources status has been cleared but NSTemplateSet still exists + AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). + HasStatusClusterResourcesNil(). + HasConditions(Terminating()) + }) + + t.Run("without namespaces - status cleared and NSTemplateSet deleted", func(t *testing.T) { + // given an NSTemplateSet resource with cluster resources but no namespaces + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) + crq := newClusterResourceQuota(spacename, "advanced") + r, _ := prepareController(t, nsTmplSet, crq) + req := newReconcileRequest(namespaceName, spacename) + + // when reconcile is triggered + _, err := r.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + // verify that the NSTemplateSet is completely deleted (including finalizer removal) + AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). + DoesNotExist() + // verify cluster resources are also deleted + AssertThatCluster(t, r.Client).HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) + }) + }) + + t.Run("failed to clear cluster resources status", func(t *testing.T) { + // given an NSTemplateSet resource with cluster resources + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) + crq := newClusterResourceQuota(spacename, "advanced") + r, fakeClient := prepareController(t, nsTmplSet, crq) + req := newReconcileRequest(namespaceName, spacename) + + // mock status update failure for clearing cluster resources + fakeClient.MockStatusUpdate = func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if nsTmplSet, ok := obj.(*toolchainv1alpha1.NSTemplateSet); ok && nsTmplSet.Status.ClusterResources == nil { + return fmt.Errorf("mock status update error") + } + return fakeClient.Client.Status().Update(ctx, obj, opts...) + } + + // when reconcile is triggered + result, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "failed to clear ClusterResources status: mock status update error") + require.Equal(t, controllerruntime.Result{}, result) + }) + t.Run("NSTemplateSet deletion errors when namespace is not deleted in 1 min", func(t *testing.T) { // given an NSTemplateSet resource and 1 active user namespaces ("dev") nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "stage"), withDeletionTs(), withClusterResources("abcde11")) From d1b542b3d0fd0d3cc9cecf78de7d01884e82e239 Mon Sep 17 00:00:00 2001 From: Rafaela Soares Date: Tue, 4 Nov 2025 10:40:43 +0000 Subject: [PATCH 3/3] requested changes --- .../nstemplateset_controller_test.go | 44 +------------------ 1 file changed, 2 insertions(+), 42 deletions(-) diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index e3e42364..a5730cbd 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -1162,7 +1162,8 @@ func TestDeleteNSTemplateSet(t *testing.T) { AssertThatNamespace(t, firstNSName, r.Client).DoesNotExist() // get the NSTemplateSet resource again and check its status AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). - HasFinalizer(). // the finalizer should NOT have been removed yet + HasFinalizer(). // the finalizer should NOT have been removed yet + HasStatusClusterResourcesNil(). // the cluster resources status should be cleared HasConditions(Terminating()) t.Run("reconcile after first user namespace deletion triggers deletion of the second namespace", func(t *testing.T) { @@ -1214,47 +1215,6 @@ func TestDeleteNSTemplateSet(t *testing.T) { require.EqualError(t, err, "failed to delete cluster resource 'for-johnsmith': mock error") require.Equal(t, controllerruntime.Result{}, result) }) - - t.Run("cluster resources status is cleared after successful deletion", func(t *testing.T) { - t.Run("with namespaces - status cleared and NSTemplateSet remains", func(t *testing.T) { - // given an NSTemplateSet resource with cluster resources and namespaces - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) - crq := newClusterResourceQuota(spacename, "advanced") - devNS := newNamespace("advanced", spacename, "dev", withTemplateRefUsingRevision("abcde11")) - r, _ := prepareController(t, nsTmplSet, crq, devNS) - req := newReconcileRequest(namespaceName, spacename) - - // when reconcile is triggered - _, err := r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // verify that the cluster resources status has been cleared but NSTemplateSet still exists - AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). - HasStatusClusterResourcesNil(). - HasConditions(Terminating()) - }) - - t.Run("without namespaces - status cleared and NSTemplateSet deleted", func(t *testing.T) { - // given an NSTemplateSet resource with cluster resources but no namespaces - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) - crq := newClusterResourceQuota(spacename, "advanced") - r, _ := prepareController(t, nsTmplSet, crq) - req := newReconcileRequest(namespaceName, spacename) - - // when reconcile is triggered - _, err := r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // verify that the NSTemplateSet is completely deleted (including finalizer removal) - AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). - DoesNotExist() - // verify cluster resources are also deleted - AssertThatCluster(t, r.Client).HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) - }) - }) - t.Run("failed to clear cluster resources status", func(t *testing.T) { // given an NSTemplateSet resource with cluster resources nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11"))