fix the issue that the binding's suspension persists when the policy deletes the suspension configuration

zhzhuang-zju · zhzhuang-zju · commit b9a85d4e35c4 · 2025-08-30T09:43:27.000+08:00
Signed-off-by: zhzhuang-zju &lt;m17799853869@163.com&gt;
diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go
@@ -478,12 +478,7 @@ func (d *ResourceDetector) ApplyPolicy(object *unstructured.Unstructured, object
 			bindingCopy.Spec.ConflictResolution = binding.Spec.ConflictResolution
 			bindingCopy.Spec.PreserveResourcesOnDeletion = binding.Spec.PreserveResourcesOnDeletion
 			bindingCopy.Spec.SchedulePriority = binding.Spec.SchedulePriority
-			if binding.Spec.Suspension != nil {
-				if bindingCopy.Spec.Suspension == nil {
-					bindingCopy.Spec.Suspension = &workv1alpha2.Suspension{}
-				}
-				bindingCopy.Spec.Suspension.Suspension = binding.Spec.Suspension.Suspension
-			}
+			bindingCopy.Spec.Suspension = util.MergePolicySuspension(bindingCopy.Spec.Suspension, policy.Spec.Suspension)
 			excludeClusterPolicy(bindingCopy)
 			return nil
 		})
@@ -574,12 +569,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 				bindingCopy.Spec.ConflictResolution = binding.Spec.ConflictResolution
 				bindingCopy.Spec.PreserveResourcesOnDeletion = binding.Spec.PreserveResourcesOnDeletion
 				bindingCopy.Spec.SchedulePriority = binding.Spec.SchedulePriority
-				if binding.Spec.Suspension != nil {
-					if bindingCopy.Spec.Suspension == nil {
-						bindingCopy.Spec.Suspension = &workv1alpha2.Suspension{}
-					}
-					bindingCopy.Spec.Suspension.Suspension = binding.Spec.Suspension.Suspension
-				}
+				bindingCopy.Spec.Suspension = util.MergePolicySuspension(bindingCopy.Spec.Suspension, policy.Spec.Suspension)
 				return nil
 			})
 			return err
@@ -627,12 +617,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 				bindingCopy.Spec.Failover = binding.Spec.Failover
 				bindingCopy.Spec.ConflictResolution = binding.Spec.ConflictResolution
 				bindingCopy.Spec.PreserveResourcesOnDeletion = binding.Spec.PreserveResourcesOnDeletion
-				if binding.Spec.Suspension != nil {
-					if bindingCopy.Spec.Suspension == nil {
-						bindingCopy.Spec.Suspension = &workv1alpha2.Suspension{}
-					}
-					bindingCopy.Spec.Suspension.Suspension = binding.Spec.Suspension.Suspension
-				}
+				bindingCopy.Spec.Suspension = util.MergePolicySuspension(bindingCopy.Spec.Suspension, policy.Spec.Suspension)
 				return nil
 			})
 			return err
diff --git a/pkg/util/binding.go b/pkg/util/binding.go
@@ -110,3 +110,23 @@ func RescheduleRequired(rescheduleTriggeredAt, lastScheduledTime *metav1.Time) b
 	}
 	return rescheduleTriggeredAt.After(lastScheduledTime.Time)
 }
+
+// MergePolicySuspension merges the suspension configuration from policy to binding suspension.
+func MergePolicySuspension(bindingSuspension *workv1alpha2.Suspension, policySuspension *policyv1alpha1.Suspension) *workv1alpha2.Suspension {
+	if bindingSuspension == nil && policySuspension == nil {
+		return nil
+	}
+
+	if policySuspension != nil { // have to sync policy suspension to binding
+		if bindingSuspension == nil {
+			bindingSuspension = &workv1alpha2.Suspension{}
+		}
+		bindingSuspension.Suspension = *policySuspension
+	} else { // have to clean suspension previous synced to binding if any
+		bindingSuspension.Suspension = policyv1alpha1.Suspension{}
+		if bindingSuspension.Scheduling == nil { // if the scheduling not set, no need to keep an empty struct
+			bindingSuspension = nil
+		}
+	}
+	return bindingSuspension
+}
diff --git a/pkg/util/binding_test.go b/pkg/util/binding_test.go
@@ -23,6 +23,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/utils/ptr"
 
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
 	workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
@@ -419,3 +420,87 @@ func TestRescheduleRequired(t *testing.T) {
 		})
 	}
 }
+
+func TestMergePolicySuspension(t *testing.T) {
+	tests := []struct {
+		name              string
+		bindingSuspension *workv1alpha2.Suspension
+		policySuspension  *policyv1alpha1.Suspension
+		want              *workv1alpha2.Suspension
+	}{
+		{
+			name:              "both nil returns nil",
+			bindingSuspension: nil,
+			policySuspension:  nil,
+			want:              nil,
+		},
+		{
+			name: "binding suspension only preserves scheduling when policy suspension nil",
+			bindingSuspension: &workv1alpha2.Suspension{
+				Scheduling: ptr.To(true),
+			},
+			policySuspension: nil,
+			want: &workv1alpha2.Suspension{
+				Scheduling: ptr.To(true),
+			},
+		},
+		{
+			name: "binding suspension only preserves scheduling when policy suspension nil",
+			bindingSuspension: &workv1alpha2.Suspension{
+				Suspension: policyv1alpha1.Suspension{
+					Dispatching: ptr.To(true),
+				},
+				Scheduling: ptr.To(true),
+			},
+			policySuspension: nil,
+			want: &workv1alpha2.Suspension{
+				Scheduling: ptr.To(true),
+			},
+		},
+		{
+			name: "if the scheduling not set and policy suspension nil, will return nil",
+			bindingSuspension: &workv1alpha2.Suspension{
+				Suspension: policyv1alpha1.Suspension{
+					Dispatching: ptr.To(true),
+				},
+			},
+			policySuspension: nil,
+			want:             nil,
+		},
+		{
+			name:              "policy suspension set and no existing binding creates new suspension from policy",
+			bindingSuspension: nil,
+			policySuspension: &policyv1alpha1.Suspension{
+				Dispatching: ptr.To(true),
+			},
+			want: &workv1alpha2.Suspension{
+				Suspension: policyv1alpha1.Suspension{
+					Dispatching: ptr.To(true),
+				},
+			},
+		},
+		{
+			name: "should merge policy suspension and binding suspension",
+			bindingSuspension: &workv1alpha2.Suspension{
+				Scheduling: ptr.To(true),
+			},
+			policySuspension: &policyv1alpha1.Suspension{
+				Dispatching: ptr.To(true),
+			},
+			want: &workv1alpha2.Suspension{
+				Suspension: policyv1alpha1.Suspension{
+					Dispatching: ptr.To(true),
+				},
+				Scheduling: ptr.To(true),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := MergePolicySuspension(tt.bindingSuspension, tt.policySuspension)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("UpdateBindingSuspension() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/test/e2e/suites/base/propagationpolicy_test.go b/test/e2e/suites/base/propagationpolicy_test.go
@@ -1161,7 +1161,7 @@ var _ = ginkgo.Describe("[AdvancedCase] PropagationPolicy testing", func() {
 	})
 })
 
-var _ = ginkgo.Describe("[Suspension] PropagationPolicy testing", func() {
+var _ = ginkgo.Describe("Suspension: PropagationPolicy testing", func() {
 	var policy *policyv1alpha1.PropagationPolicy
 	var deployment *appsv1.Deployment
 	var targetMember string
@@ -1190,20 +1190,19 @@ var _ = ginkgo.Describe("[Suspension] PropagationPolicy testing", func() {
 			func(*appsv1.Deployment) bool {
 				return true
 			})
-		ginkgo.DeferCleanup(func() {
-			framework.RemovePropagationPolicy(karmadaClient, policy.Namespace, policy.Name)
-			framework.RemoveDeployment(kubeClient, deployment.Namespace, deployment.Name)
-		})
-	})
-
-	ginkgo.It("suspend the PP dispatching", func() {
 		ginkgo.By("update the pp suspension dispatching to true", func() {
 			policy.Spec.Suspension = &policyv1alpha1.Suspension{
 				Dispatching: ptr.To(true),
 			}
 			framework.UpdatePropagationPolicyWithSpec(karmadaClient, policy.Namespace, policy.Name, policy.Spec)
 		})
+		ginkgo.DeferCleanup(func() {
+			framework.RemovePropagationPolicy(karmadaClient, policy.Namespace, policy.Name)
+			framework.RemoveDeployment(kubeClient, deployment.Namespace, deployment.Name)
+		})
+	})
 
+	ginkgo.It("suspend the PP dispatching", func() {
 		ginkgo.By("check RB suspension spec", func() {
 			framework.WaitResourceBindingFitWith(karmadaClient, deployment.Namespace, names.GenerateBindingName(deployment.Kind, deployment.Name),
 				func(binding *workv1alpha2.ResourceBinding) bool {
@@ -1246,7 +1245,7 @@ var _ = ginkgo.Describe("[Suspension] PropagationPolicy testing", func() {
 		})
 	})
 
-	ginkgo.It("suspension resume", func() {
+	ginkgo.It("suspension resume by setting policy suspension to a empty structure", func() {
 		ginkgo.By("update deployment replicas", func() {
 			framework.UpdateDeploymentReplicas(kubeClient, deployment, updateDeploymentReplicas)
 		})
@@ -1263,4 +1262,22 @@ var _ = ginkgo.Describe("[Suspension] PropagationPolicy testing", func() {
 				})
 		})
 	})
+
+	ginkgo.It("suspension resume by setting policy suspension to nil", func() {
+		ginkgo.By("update deployment replicas", func() {
+			framework.UpdateDeploymentReplicas(kubeClient, deployment, updateDeploymentReplicas)
+		})
+
+		ginkgo.By("resume the propagationPolicy", func() {
+			policy.Spec.Suspension = nil
+			framework.UpdatePropagationPolicyWithSpec(karmadaClient, policy.Namespace, policy.Name, policy.Spec)
+		})
+
+		ginkgo.By("check deployment replicas", func() {
+			framework.WaitDeploymentPresentOnClusterFitWith(targetMember, deployment.Namespace, deployment.Name,
+				func(d *appsv1.Deployment) bool {
+					return *d.Spec.Replicas == updateDeploymentReplicas
+				})
+		})
+	})
 })
