From d3e6e107aad46bcdd248765ec4471d736e215c8d Mon Sep 17 00:00:00 2001 From: whitewindmills Date: Wed, 15 Nov 2023 11:34:20 +0800 Subject: [PATCH] ensure idempotence when deleting policy Signed-off-by: whitewindmills --- pkg/detector/detector.go | 56 +++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go index 62d40fd4f08e..b7c42ab158b4 100644 --- a/pkg/detector/detector.go +++ b/pkg/detector/detector.go @@ -1039,18 +1039,19 @@ func (d *ResourceDetector) HandlePropagationPolicyDeletion(policyNS string, poli } for index, binding := range rbs.Items { - // Cleanup the labels from the reference binding so that the karmada scheduler won't reschedule the binding. - if err := d.CleanupResourceBindingLabels(&rbs.Items[index], policyv1alpha1.PropagationPolicyNamespaceLabel, policyv1alpha1.PropagationPolicyNameLabel); err != nil { - klog.Errorf("Failed to cleanup label from resource binding(%s/%s) when propagation policy(%s/%s) removing, error: %v", - binding.Namespace, binding.Name, policyNS, policyName, err) + // Must remove the labels from the resource template ahead of ResourceBinding, otherwise might lose the chance + // to do that in a retry loop(in particular, the label was successfully removed from ResourceBinding, but + // resource template not), since the ResourceBinding will not be listed again. + if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.PropagationPolicyNamespaceLabel, policyv1alpha1.PropagationPolicyNameLabel); err != nil { + klog.Errorf("Failed to clean up label from resource(%s-%s/%s) when propagation policy(%s/%s) removing, error: %v", + binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, policyNS, policyName, err) return err } - // Cleanup the labels from the object referencing by binding. - // In addition, this will give the object a chance to match another policy. - if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.PropagationPolicyNamespaceLabel, policyv1alpha1.PropagationPolicyNameLabel); err != nil { - klog.Errorf("Failed to cleanup label from resource(%s-%s/%s) when propagation policy(%s/%s) removing, error: %v", - binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, policyNS, policyName, err) + // Clean up the labels from the reference binding so that the karmada scheduler won't reschedule the binding. + if err := d.CleanupResourceBindingLabels(&rbs.Items[index], policyv1alpha1.PropagationPolicyNamespaceLabel, policyv1alpha1.PropagationPolicyNameLabel); err != nil { + klog.Errorf("Failed to clean up label from resource binding(%s/%s) when propagation policy(%s/%s) removing, error: %v", + binding.Namespace, binding.Name, policyNS, policyName, err) return err } } @@ -1075,18 +1076,19 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyName str errs = append(errs, err) } else if len(crbs.Items) > 0 { for index, binding := range crbs.Items { - // Cleanup the labels from the reference binding so that the karmada scheduler won't reschedule the binding. - if err := d.CleanupClusterResourceBindingLabels(&crbs.Items[index], policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { - klog.Errorf("Failed to cleanup label from cluster resource binding(%s) when cluster propagation policy(%s) removing, error: %v", - binding.Name, policyName, err) + // Must remove the labels from the resource template ahead of ClusterResourceBinding, otherwise might lose the chance + // to do that in a retry loop(in particular, the label was successfully removed from ClusterResourceBinding, but + // resource template not), since the ClusterResourceBinding will not be listed again. + if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { + klog.Errorf("Failed to clean up label from resource(%s-%s) when cluster propagation policy(%s) removing, error: %v", + binding.Spec.Resource.Kind, binding.Spec.Resource.Name, policyName, err) errs = append(errs, err) } - // Cleanup the labels from the object referencing by binding. - // In addition, this will give the object a chance to match another policy. - if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { - klog.Errorf("Failed to cleanup label from resource(%s-%s/%s) when cluster resource binding(%s) removing, error: %v", - binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, binding.Name, err) + // Clean up the labels from the reference binding so that the karmada scheduler won't reschedule the binding. + if err := d.CleanupClusterResourceBindingLabels(&crbs.Items[index], policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { + klog.Errorf("Failed to clean up label from cluster resource binding(%s) when cluster propagation policy(%s) removing, error: %v", + binding.Name, policyName, err) errs = append(errs, err) } } @@ -1099,17 +1101,19 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyName str errs = append(errs, err) } else if len(rbs.Items) > 0 { for index, binding := range rbs.Items { - // Cleanup the labels from the reference binding so that the karmada scheduler won't reschedule the binding. - if err := d.CleanupResourceBindingLabels(&rbs.Items[index], policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { - klog.Errorf("Failed to cleanup label from resource binding(%s/%s) when cluster propagation policy(%s) removing, error: %v", - binding.Namespace, binding.Name, policyName, err) + // Must remove the labels from the resource template ahead of ResourceBinding, otherwise might lose the chance + // to do that in a retry loop(in particular, the label was successfully removed from ResourceBinding, but + // resource template not), since the ResourceBinding will not be listed again. + if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { + klog.Errorf("Failed to clean up label from resource(%s-%s/%s) when cluster propagation policy(%s) removing, error: %v", + binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, policyName, err) errs = append(errs, err) } - // Cleanup the labels from the object referencing by binding. - // In addition, this will give the object a chance to match another policy. - if err := d.CleanupLabels(binding.Spec.Resource, policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { - klog.Errorf("Failed to cleanup label from resource binding(%s/%s), error: %v", binding.Namespace, binding.Name, err) + // Clean up the labels from the reference binding so that the karmada scheduler won't reschedule the binding. + if err := d.CleanupResourceBindingLabels(&rbs.Items[index], policyv1alpha1.ClusterPropagationPolicyLabel); err != nil { + klog.Errorf("Failed to clean up label from resource binding(%s/%s) when cluster propagation policy(%s) removing, error: %v", + binding.Namespace, binding.Name, policyName, err) errs = append(errs, err) } }