Repository: karmada-io/karmada

Issue:
[Suspension] .spec.suspension of ResourceBinding not removed after .spec.suspension of PropagationPolicy has been removed
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
-->


**What happened**:
.spec.suspension of ResourceBinding not removed after .spec.suspension of PropagationPolicy has been removed

**What you expected to happen**:
.spec.suspension of ResourceBinding will be removed after .spec.suspension of PropagationPolicy has been removed

**How to reproduce it (as minimally and precisely as possible)**:
1. Add suspension into PropagationPolicy
2. Suspension is added automatically into ResourceBinding
3. Remove suspension of PropagationPolicy
4. Suspension of ResourceBinding  cannot be removed automatically, which makes Suspension still take effect.
5. After removed Suspension of ResourceBinding manually, the Suspension effect is gone.

**Anything else we need to know?**:

**Environment**: 
- Karmada version: v1.14.0
- kubectl-karmada or karmadactl version (the result of `kubectl-karmada version` or `karmadactl version`):
- Others:


Hints from issue comments:
@XiShanYongYe-Chang: ~Hi @ryanwuer the current behavior is as expected; even if the PropagationPolicy is deleted, the ResourceBinding will still remain~, and its configuration will be consistent with that before the PropagationPolicy was deleted (unless another PropagationPolicy with the same name as the current resource exists, thereby overriding the configuration of the ResourceBinding).

Could you help explain why you need to delete the PropagationPolicy and the use cases where you do not set suspension to true before deleting it?@ryanwuer: @XiShanYongYe-Chang I'm saying the behavior of deletion of 'Suspension', not the PropagationPolicy itself.

Here is the reference doc: https://karmada.io/zh/docs/userguide/scheduling/resource-propagating/#%E6%9A%82%E5%81%9C%E5%90%91%E7%89%B9%E5%AE%9A%E9%9B%86%E7%BE%A4%E5%88%86%E5%8F%91%E8%B5%84%E6%BA%90@XiShanYongYe-Chang: ~Sorry, I misread it. This issue may be consistent with the problem reported by #5999.~

This issue description addresses the same problem: https://github.com/karmada-io/karmada/issues/5996@ryanwuer: The question is the real logic is not consistent with the doc: https://karmada.io/docs/userguide/scheduling/resource-propagating/#resume-resource-propagation, which says dispatching work can be resumed by removing the `.spec.suspension` of PropagationPolicy.

`.spec.suspension` of ResourceBinding need to be removed manually to make that happen. 

@RainbowMango Is this a problem waiting to be fixed?@RainbowMango: >  Is this a problem waiting to be fixed?

Yes. 
This issue was reported by @CharlesQQ now @zhzhuang-zju is looking at it. But it's not easy to fix~@ryanwuer: > > Is this a problem waiting to be fixed?
> 
> Yes. This issue was reported by [@CharlesQQ](https://github.com/CharlesQQ) now [@zhzhuang-zju](https://github.com/zhzhuang-zju) is looking at it. But it's not easy to fix~

Could this be resolved in the next release? We're counting on Suspension function to handle real case.@RainbowMango: Yeah, we are trying to fix it in the coming release (by the end of this month). 
@zhzhuang-zju Please leave a message here once you send the PR.
Maybe @CharlesQQ and @ryanwuer can also help to review and test.@zhzhuang-zju: Sure, the work is in progress, and welcome to review it once it's completed.@zhzhuang-zju: cc @CharlesQQ @ryanwuer 
https://github.com/karmada-io/karmada/pull/6674 @zhzhuang-zju: @ryanwuer Sorry, we previously gave an incorrect assessment. The actual cause of this issue is that when the policy's suspension is set to nil, the detector does not make corresponding corrections to the existing resource binding. The specific code is as follows:
https://github.com/karmada-io/karmada/blob/3b4768350e09ae7ebfed2d4e4c8ea56177106fdf/pkg/detector/detector.go#L451-L486

And this issue was first introduced by https://github.com/karmada-io/karmada/pull/6062

I'll fix this issue ASAP, and I think we need to add protection for this scenario in the e2e tests.

Solution patch:
diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go
index 1f3b4cacc..1d8b315bb 100644
--- a/pkg/detector/detector.go
+++ b/pkg/detector/detector.go
@@ -491,12 +491,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
 		})
@@ -587,12 +582,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
@@ -640,12 +630,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
index 6811984f0..267f07890 100644
--- 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
+}



Test patch:
diff --git a/pkg/util/binding_test.go b/pkg/util/binding_test.go
index 79355978b..236254ec3 100644
--- 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
index 707d692e0..bc5b024d9 100644
--- 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
+				})
+		})
+	})
 })