From 76badb204ecb16df61681df1b04ce8333250fae4 Mon Sep 17 00:00:00 2001 From: whitewindmills Date: Wed, 2 Aug 2023 11:40:38 +0800 Subject: [PATCH] avoid updating directly cached resource template for clean up policy Signed-off-by: whitewindmills --- pkg/detector/detector.go | 12 ++++++------ pkg/detector/policy.go | 6 +++--- pkg/util/helper/binding.go | 5 ++++- pkg/util/label.go | 16 +++++++++++----- pkg/util/label_test.go | 2 +- 5 files changed, 25 insertions(+), 16 deletions(-) diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go index 1ec920ccf7ce..74f4252428e5 100644 --- a/pkg/detector/detector.go +++ b/pkg/detector/detector.go @@ -551,6 +551,9 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured, } // GetUnstructuredObject retrieves object by key and returned its unstructured. +// Any updates to this resource template are not recommended as it may come from the informer cache. +// We should abide by the principle of making a deep copy first and then modifying it. +// See issue: https://github.com/karmada-io/karmada/issues/3878. func (d *ResourceDetector) GetUnstructuredObject(objectKey keys.ClusterWideKey) (*unstructured.Unstructured, error) { objectGVR, err := restmapper.GetGroupVersionResource(d.RESTMapper, objectKey.GroupVersionKind()) if err != nil { @@ -1077,7 +1080,7 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyCreationOrUpdate(policy } // CleanupLabels removes labels from object referencing by objRef. -func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, labels ...string) error { +func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, labelKeys ...string) error { workload, err := helper.FetchWorkload(d.DynamicClient, d.InformerManager, d.RESTMapper, objRef) if err != nil { // do nothing if resource template not exist, it might has been removed. @@ -1088,11 +1091,8 @@ func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, la return err } - workloadLabels := workload.GetLabels() - for _, l := range labels { - delete(workloadLabels, l) - } - workload.SetLabels(workloadLabels) + workload = workload.DeepCopy() + util.RemoveLabels(workload, labelKeys...) gvr, err := restmapper.GetGroupVersionResource(d.RESTMapper, workload.GroupVersionKind()) if err != nil { diff --git a/pkg/detector/policy.go b/pkg/detector/policy.go index 72418adc707f..6ce673a3b6c5 100644 --- a/pkg/detector/policy.go +++ b/pkg/detector/policy.go @@ -263,9 +263,9 @@ func (d *ResourceDetector) removeResourceLabelsIfNotMatch(objectReference workv1 return false, nil } - for _, labelKey := range labelKeys { - util.RemoveLabel(object, labelKey) - } + object = object.DeepCopy() + util.RemoveLabels(object, labelKeys...) + err = d.Client.Update(context.TODO(), object) if err != nil { return false, err diff --git a/pkg/util/helper/binding.go b/pkg/util/helper/binding.go index e6a140ce82db..f890d646f3bd 100644 --- a/pkg/util/helper/binding.go +++ b/pkg/util/helper/binding.go @@ -215,7 +215,10 @@ func RemoveOrphanWorks(c client.Client, works []workv1alpha1.Work) error { return errors.NewAggregate(errs) } -// FetchWorkload fetches the kubernetes resource to be propagated. +// FetchWorkload fetches the workload to be propagated. +// Any updates to this workload are not recommended as it may come from the informer cache. +// We should abide by the principle of making a deep copy first and then modifying it. +// See issue: https://github.com/karmada-io/karmada/issues/3878. func FetchWorkload(dynamicClient dynamic.Interface, informerManager genericmanager.SingleClusterInformerManager, restMapper meta.RESTMapper, resource workv1alpha2.ObjectReference) (*unstructured.Unstructured, error) { dynamicResource, err := restmapper.GetGroupVersionResource(restMapper, diff --git a/pkg/util/label.go b/pkg/util/label.go index d9bee47eeee2..8e9c025ae644 100644 --- a/pkg/util/label.go +++ b/pkg/util/label.go @@ -57,11 +57,17 @@ func MergeLabel(obj *unstructured.Unstructured, labelKey string, labelValue stri obj.SetLabels(labels) } -// RemoveLabel removes the label from the given object. -func RemoveLabel(obj *unstructured.Unstructured, labelKey string) { - labels := obj.GetLabels() - delete(labels, labelKey) - obj.SetLabels(labels) +// RemoveLabels removes the labels from the given object. +func RemoveLabels(obj *unstructured.Unstructured, labelKeys ...string) { + if len(labelKeys) == 0 { + return + } + + objLabels := obj.GetLabels() + for _, labelKey := range labelKeys { + delete(objLabels, labelKey) + } + obj.SetLabels(objLabels) } // DedupeAndMergeLabels merges the new labels into exist labels. diff --git a/pkg/util/label_test.go b/pkg/util/label_test.go index 9740e834cf0e..e3ad3351763d 100644 --- a/pkg/util/label_test.go +++ b/pkg/util/label_test.go @@ -306,7 +306,7 @@ func TestRemoveLabel(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - RemoveLabel(tt.args.obj, tt.args.labelKey) + RemoveLabels(tt.args.obj, tt.args.labelKey) if !reflect.DeepEqual(tt.args.obj, tt.expected) { t.Errorf("RemoveLabel() = %v, want %v", tt.args.obj, tt.expected) }