Merge pull request #6622 from XiShanYongYe-Chang/fix-6616

When using MCS and MCI simultaneously, prevent resource residuce caused by deleting MCS and MCI

Author: karmada-bot

pkg/controllers/ctrlutil/work.go

@@ -71,7 +71,7 @@ func CreateOrUpdateWork(ctx context.Context, c client.Client, workMeta metav1.Ob
 
 			runtimeObject.Labels = util.DedupeAndMergeLabels(runtimeObject.Labels, work.Labels)
 			runtimeObject.Annotations = util.DedupeAndMergeAnnotations(runtimeObject.Annotations, work.Annotations)
-			runtimeObject.Finalizers = work.Finalizers
+			runtimeObject.Finalizers = util.MergeFinalizers(runtimeObject.Finalizers, work.Finalizers)
 			runtimeObject.Spec = work.Spec
 
 			// Do the same thing as the mutating webhook does, add the permanent ID to workload if not exist,

pkg/controllers/mcs/service_export_controller.go

@@ -541,10 +541,14 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
 		return metav1.ObjectMeta{}, err
 	}
 
+	existFinalizers := existWork.GetFinalizers()
+	finalizersToAdd := []string{util.EndpointSliceControllerFinalizer}
+	newFinalizers := util.MergeFinalizers(existFinalizers, finalizersToAdd)
+
 	workMeta := metav1.ObjectMeta{
 		Name:       workName,
 		Namespace:  ns,
-		Finalizers: []string{util.EndpointSliceControllerFinalizer},
+		Finalizers: newFinalizers,
 		Labels: map[string]string{
 			util.ServiceNamespaceLabel:           endpointSlice.GetNamespace(),
 			util.ServiceNameLabel:                endpointSlice.GetLabels()[discoveryv1.LabelServiceName],

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -415,13 +415,22 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
 		return metav1.ObjectMeta{}, err
 	}
 
+	existFinalizers := existWork.GetFinalizers()
+	finalizersToAdd := []string{util.MCSEndpointSliceDispatchControllerFinalizer}
+	newFinalizers := util.MergeFinalizers(existFinalizers, finalizersToAdd)
+
 	ls := map[string]string{
 		util.MultiClusterServiceNamespaceLabel: endpointSlice.GetNamespace(),
 		util.MultiClusterServiceNameLabel:      endpointSlice.GetLabels()[discoveryv1.LabelServiceName],
 		util.EndpointSliceWorkManagedByLabel:   util.MultiClusterServiceKind,
 	}
 	if existWork.Labels == nil || (err != nil && apierrors.IsNotFound(err)) {
-		workMeta := metav1.ObjectMeta{Name: workName, Namespace: ns, Labels: ls}
+		workMeta := metav1.ObjectMeta{
+			Name:       workName,
+			Namespace:  ns,
+			Labels:     ls,
+			Finalizers: newFinalizers,
+		}
 		return workMeta, nil
 	}
 
@@ -436,7 +445,7 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
 		Name:       workName,
 		Namespace:  ns,
 		Labels:     ls,
-		Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
+		Finalizers: newFinalizers,
 	}, nil
 }
 

pkg/controllers/multiclusterservice/endpointslice_collect_controller_test.go

@@ -146,10 +146,11 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
 					util.MultiClusterServiceNameLabel:      "test-service",
 					util.EndpointSliceWorkManagedByLabel:   util.MultiClusterServiceKind,
 				},
+				Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
 			},
 		},
 		{
-			name:          "Existing work for EndpointSlice",
+			name:          "Existing work for EndpointSlice without finalizers",
 			existingWork:  createExistingWork("endpointslice-test-eps-default", "test-cluster", "ExistingController"),
 			endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
 			expectedMeta: metav1.ObjectMeta{
@@ -163,6 +164,51 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
 				Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
 			},
 		},
+		{
+			name:          "Existing work with existing finalizers",
+			existingWork:  createExistingWorkWithFinalizers("endpointslice-test-eps-default", "test-cluster", "ExistingController", []string{"existing.finalizer", "another.finalizer"}),
+			endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
+			expectedMeta: metav1.ObjectMeta{
+				Name:      "endpointslice-test-eps-default",
+				Namespace: "test-cluster",
+				Labels: map[string]string{
+					util.MultiClusterServiceNamespaceLabel: "default",
+					util.MultiClusterServiceNameLabel:      "test-service",
+					util.EndpointSliceWorkManagedByLabel:   "ExistingController.MultiClusterService",
+				},
+				Finalizers: []string{"another.finalizer", "existing.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
+			},
+		},
+		{
+			name:          "Existing work with duplicate finalizer",
+			existingWork:  createExistingWorkWithFinalizers("endpointslice-test-eps-default", "test-cluster", "ExistingController", []string{util.MCSEndpointSliceDispatchControllerFinalizer, "another.finalizer"}),
+			endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
+			expectedMeta: metav1.ObjectMeta{
+				Name:      "endpointslice-test-eps-default",
+				Namespace: "test-cluster",
+				Labels: map[string]string{
+					util.MultiClusterServiceNamespaceLabel: "default",
+					util.MultiClusterServiceNameLabel:      "test-service",
+					util.EndpointSliceWorkManagedByLabel:   "ExistingController.MultiClusterService",
+				},
+				Finalizers: []string{"another.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
+			},
+		},
+		{
+			name:          "Existing work without labels",
+			existingWork:  createExistingWorkWithoutLabels("endpointslice-test-eps-default", "test-cluster", []string{"existing.finalizer"}),
+			endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
+			expectedMeta: metav1.ObjectMeta{
+				Name:      "endpointslice-test-eps-default",
+				Namespace: "test-cluster",
+				Labels: map[string]string{
+					util.MultiClusterServiceNamespaceLabel: "default",
+					util.MultiClusterServiceNameLabel:      "test-service",
+					util.EndpointSliceWorkManagedByLabel:   util.MultiClusterServiceKind,
+				},
+				Finalizers: []string{"existing.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
+			},
+		},
 		{
 			name:          "Nil EndpointSlice",
 			endpointSlice: nil,
@@ -186,7 +232,10 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
 				require.NoError(t, err)
 				assert.Equal(t, tc.expectedMeta.Name, meta.Name)
 				assert.Equal(t, tc.expectedMeta.Namespace, meta.Namespace)
-				assert.Equal(t, tc.expectedMeta.Finalizers, meta.Finalizers)
+
+				assert.Equal(t, tc.expectedMeta.Finalizers, meta.Finalizers,
+					"Finalizers do not match. Expected: %v, Got: %v", tc.expectedMeta.Finalizers, meta.Finalizers)
+
 				assert.True(t, compareLabels(meta.Labels, tc.expectedMeta.Labels),
 					"Labels do not match. Expected: %v, Got: %v", tc.expectedMeta.Labels, meta.Labels)
 			}
@@ -325,6 +374,31 @@ func createExistingWork(name, namespace, managedBy string) *workv1alpha1.Work {
 	}
 }
 
+// Helper function to create an existing Work resource for testing with specific finalizers
+func createExistingWorkWithFinalizers(name, namespace, managedBy string, finalizers []string) *workv1alpha1.Work {
+	return &workv1alpha1.Work{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+			Labels: map[string]string{
+				util.EndpointSliceWorkManagedByLabel: managedBy,
+			},
+			Finalizers: finalizers,
+		},
+	}
+}
+
+// Helper function to create an existing Work resource for testing without labels
+func createExistingWorkWithoutLabels(name, namespace string, finalizers []string) *workv1alpha1.Work {
+	return &workv1alpha1.Work{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       name,
+			Namespace:  namespace,
+			Finalizers: finalizers,
+		},
+	}
+}
+
 // Helper function to create a fake client with an optional existing Work
 func createFakeClient(existingWork *workv1alpha1.Work) client.Client {
 	scheme := setupSchemeEndpointCollect()

pkg/util/finalizer.go

@@ -0,0 +1,30 @@
+/*
+Copyright 2025 The Karmada Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import "k8s.io/apimachinery/pkg/util/sets"
+
+// MergeFinalizers merges the new finalizers into exist finalizers, and deduplicates the finalizers.
+// The result is sorted.
+func MergeFinalizers(existFinalizers, newFinalizers []string) []string {
+	if existFinalizers == nil && newFinalizers == nil {
+		return nil
+	}
+
+	finalizers := sets.New[string](existFinalizers...).Insert(newFinalizers...)
+	return sets.List[string](finalizers)
+}

pkg/util/finalizer_test.go

@@ -0,0 +1,131 @@
+/*
+Copyright 2025 The Karmada Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestMergeFinalizers(t *testing.T) {
+	tests := []struct {
+		name            string
+		existFinalizers []string
+		newFinalizers   []string
+		expectedResult  []string
+	}{
+		{
+			name:            "both nil",
+			existFinalizers: nil,
+			newFinalizers:   nil,
+			expectedResult:  nil,
+		},
+		{
+			name:            "exist finalizers is nil",
+			existFinalizers: nil,
+			newFinalizers:   []string{"finalizer1", "finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "new finalizers is nil",
+			existFinalizers: []string{"finalizer1", "finalizer2"},
+			newFinalizers:   nil,
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "both empty",
+			existFinalizers: []string{},
+			newFinalizers:   []string{},
+			expectedResult:  []string{},
+		},
+		{
+			name:            "exist finalizers is empty",
+			existFinalizers: []string{},
+			newFinalizers:   []string{"finalizer1", "finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "new finalizers is empty",
+			existFinalizers: []string{"finalizer1", "finalizer2"},
+			newFinalizers:   []string{},
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "no duplicates",
+			existFinalizers: []string{"finalizer1", "finalizer2"},
+			newFinalizers:   []string{"finalizer3", "finalizer4"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3", "finalizer4"},
+		},
+		{
+			name:            "with duplicates",
+			existFinalizers: []string{"finalizer1", "finalizer2"},
+			newFinalizers:   []string{"finalizer2", "finalizer3"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3"},
+		},
+		{
+			name:            "all duplicates",
+			existFinalizers: []string{"finalizer1", "finalizer2"},
+			newFinalizers:   []string{"finalizer1", "finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "duplicates in exist finalizers",
+			existFinalizers: []string{"finalizer1", "finalizer2", "finalizer1"},
+			newFinalizers:   []string{"finalizer3"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3"},
+		},
+		{
+			name:            "duplicates in new finalizers",
+			existFinalizers: []string{"finalizer1"},
+			newFinalizers:   []string{"finalizer2", "finalizer3", "finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3"},
+		},
+		{
+			name:            "duplicates in both",
+			existFinalizers: []string{"finalizer1", "finalizer2", "finalizer1"},
+			newFinalizers:   []string{"finalizer2", "finalizer3", "finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3"},
+		},
+		{
+			name:            "single finalizer in exist",
+			existFinalizers: []string{"finalizer1"},
+			newFinalizers:   []string{"finalizer2"},
+			expectedResult:  []string{"finalizer1", "finalizer2"},
+		},
+		{
+			name:            "single duplicate finalizer",
+			existFinalizers: []string{"finalizer1"},
+			newFinalizers:   []string{"finalizer1"},
+			expectedResult:  []string{"finalizer1"},
+		},
+		{
+			name:            "sort with result",
+			existFinalizers: []string{"finalizer3", "finalizer1", "finalizer2"},
+			newFinalizers:   []string{"finalizer4", "finalizer5"},
+			expectedResult:  []string{"finalizer1", "finalizer2", "finalizer3", "finalizer4", "finalizer5"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := MergeFinalizers(tt.existFinalizers, tt.newFinalizers)
+			if !reflect.DeepEqual(result, tt.expectedResult) {
+				t.Errorf("MergeFinalizers() = %v, want %v", result, tt.expectedResult)
+			}
+		})
+	}
+}