From 6d4561ca9acffacd33c08d458dcbef747b9aec6c Mon Sep 17 00:00:00 2001 From: Yang Le Date: Thu, 11 Sep 2025 15:54:39 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20addon=20namespace=20deleti?= =?UTF-8?q?on=20in=20multi-hub=20scenarios=20(#1153)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yang Le --- .../klusterlet_controller_test.go | 268 ++++++++++++++++++ .../klusterlet_managed_reconcile.go | 31 +- 2 files changed, 298 insertions(+), 1 deletion(-) diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go index 520b0aff1..5335413e1 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go +++ b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go @@ -1621,3 +1621,271 @@ func (f *fakeManagedClusterBuilder) build(_ context.Context) (*managedClusterCli kubeconfigSecretCreationTime: creationTime, }, nil } + +// TestHasActiveKlusterletAgentNamespaces tests the hasActiveKlusterletAgentNamespaces helper function +func TestHasActiveKlusterletAgentNamespaces(t *testing.T) { + tests := []struct { + name string + currentAgentNamespace string + existingNamespaces []runtime.Object + expectedResult bool + expectError bool + }{ + { + name: "no other klusterlet agent namespaces exist", + currentAgentNamespace: "open-cluster-management-agent", + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + }, + expectedResult: false, + expectError: false, + }, + { + name: "other klusterlet agent namespaces exist", + currentAgentNamespace: "open-cluster-management-agent", + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent-hosted", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet-hosted", + }, + }, + }, + }, + expectedResult: true, + expectError: false, + }, + { + name: "multiple other klusterlet agent namespaces exist", + currentAgentNamespace: "open-cluster-management-agent-hosted1", + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent-hosted1", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet-hosted1", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent-hosted2", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet-hosted2", + }, + }, + }, + }, + expectedResult: true, + expectError: false, + }, + { + name: "no klusterlet namespaces with labels exist", + currentAgentNamespace: "open-cluster-management-agent", + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-other-namespace", + }, + }, + }, + expectedResult: false, + expectError: false, + }, + { + name: "namespace without klusterlet label should be ignored", + currentAgentNamespace: "open-cluster-management-agent", + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-namespace-without-label", + }, + }, + }, + expectedResult: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + fakeKubeClient := fakekube.NewSimpleClientset(tt.existingNamespaces...) + + managedClusterClients := &managedClusterClients{ + kubeClient: fakeKubeClient, + } + + managedReconciler := &managedReconcile{ + managedClusterClients: managedClusterClients, + } + + hasActive, err := managedReconciler.hasActiveKlusterletAgentNamespaces(ctx, tt.currentAgentNamespace) + + if tt.expectError { + if err == nil { + t.Errorf("Expected an error but got none") + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + if hasActive != tt.expectedResult { + t.Errorf("Expected hasActiveKlusterletAgentNamespaces to return %t, but got %t", tt.expectedResult, hasActive) + } + } + }) + } +} + +// TestCleanWithMultipleKlusterletAgentNamespaces tests the clean function behavior when multiple klusterlet agent namespaces exist +func TestCleanWithMultipleKlusterletAgentNamespaces(t *testing.T) { + tests := []struct { + name string + klusterlet *operatorapiv1.Klusterlet + existingNamespaces []runtime.Object + shouldDeleteAddonNamespace bool + description string + }{ + { + name: "should not delete addon namespace when other klusterlet agents exist", + klusterlet: newKlusterlet("klusterlet", "open-cluster-management-agent", "cluster1"), + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent-hosted", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet-hosted", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: helpers.DefaultAddonNamespace, + }, + }, + }, + shouldDeleteAddonNamespace: false, + description: "Other klusterlet agents still exist, so addon namespace should be preserved", + }, + { + name: "should delete addon namespace when no other klusterlet agents exist", + klusterlet: newKlusterlet("klusterlet", "open-cluster-management-agent", "cluster1"), + existingNamespaces: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "open-cluster-management-agent", + Labels: map[string]string{ + klusterletNamespaceLabelKey: "klusterlet", + }, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: helpers.DefaultAddonNamespace, + }, + }, + }, + shouldDeleteAddonNamespace: true, + description: "No other klusterlet agents exist, so addon namespace should be deleted", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + syncContext := testingcommon.NewFakeSyncContext(t, "klusterlet") + + // Mark klusterlet for deletion + now := metav1.Now() + tt.klusterlet.DeletionTimestamp = &now + + controller := newTestController(t, tt.klusterlet, syncContext.Recorder(), nil, false, tt.existingNamespaces...) + + // Call the clean function through the cleanup controller + err := controller.cleanupController.sync(ctx, syncContext) + if err != nil { + t.Errorf("Expected no error from cleanup sync, but got: %v", err) + } + + // Check the namespace deletion actions + var deletedNamespaces []string + for _, action := range controller.kubeClient.Actions() { + if action.GetVerb() == deleteVerb && action.GetResource().Resource == "namespaces" { + deletedNamespaces = append(deletedNamespaces, action.(clienttesting.DeleteActionImpl).Name) + } + } + + // Verify addon namespace deletion behavior + addonNamespaceDeleted := false + for _, ns := range deletedNamespaces { + if ns == helpers.DefaultAddonNamespace { + addonNamespaceDeleted = true + break + } + } + + if tt.shouldDeleteAddonNamespace { + if !addonNamespaceDeleted { + t.Errorf("Expected addon namespace to be deleted but it wasn't. %s", tt.description) + } + } else { + if addonNamespaceDeleted { + t.Errorf("Expected addon namespace NOT to be deleted but it was. %s", tt.description) + } + } + + // The klusterlet's own agent namespace should always be deleted + agentNamespaceDeleted := false + for _, ns := range deletedNamespaces { + if ns == tt.klusterlet.Spec.Namespace { + agentNamespaceDeleted = true + break + } + } + if !agentNamespaceDeleted { + t.Errorf("Expected klusterlet agent namespace %s to be deleted but it wasn't", tt.klusterlet.Spec.Namespace) + } + }) + } +} diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go index 8df6105d6..41efad713 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go +++ b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go @@ -185,7 +185,16 @@ func (r *managedReconcile) clean(ctx context.Context, klusterlet *operatorapiv1. // For now, whether in Default or Hosted mode, the addons could be deployed on the managed cluster. namespaces := []string{config.KlusterletNamespace} if !config.DisableAddonNamespace { - namespaces = append(namespaces, helpers.DefaultAddonNamespace) + // check if other klusterlet agent namespaces still exist before deleting addon namespace + hasOtherAgents, err := r.hasActiveKlusterletAgentNamespaces(ctx, config.KlusterletNamespace) + if err != nil { + return klusterlet, reconcileStop, fmt.Errorf("failed to check for active klusterlet agent namespaces: %v", err) + } + + // only delete addon namespace if no other klusterlet agents exist + if !hasOtherAgents { + namespaces = append(namespaces, helpers.DefaultAddonNamespace) + } } for _, namespace := range namespaces { if err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().Delete( @@ -230,3 +239,23 @@ func (r *managedReconcile) cleanUpAppliedManifestWorks(ctx context.Context, klus } return utilerrors.NewAggregate(errs) } + +// hasActiveKlusterletAgentNamespaces checks if there are any other klusterlet agent namespaces +// on the managed cluster besides the one being deleted. This prevents premature deletion of +// the shared addon namespace when multiple klusterlets (from different hubs) are using it. +func (r *managedReconcile) hasActiveKlusterletAgentNamespaces(ctx context.Context, currentAgentNamespace string) (bool, error) { + // Look for namespaces with klusterlet labels + namespaces, err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{ + LabelSelector: klusterletNamespaceLabelKey, + }) + if err != nil { + return false, fmt.Errorf("failed to list klusterlet agent namespaces: %v", err) + } + + // check if there exist namespaces other than the one being deleted + if len(namespaces.Items) > 1 || len(namespaces.Items) == 1 && namespaces.Items[0].Name != currentAgentNamespace { + return true, nil + } + + return false, nil +}