diff --git a/pkg/detector/claim.go b/pkg/detector/claim.go new file mode 100644 index 000000000000..f5715bbf98ba --- /dev/null +++ b/pkg/detector/claim.go @@ -0,0 +1,77 @@ +/* +Copyright 2024 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 detector + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" + "github.com/karmada-io/karmada/pkg/util" +) + +var ( + propagationPolicyClaimLabels = []string{ + policyv1alpha1.PropagationPolicyPermanentIDLabel, + } + propagationPolicyClaimAnnotations = []string{ + policyv1alpha1.PropagationPolicyNamespaceAnnotation, + policyv1alpha1.PropagationPolicyNameAnnotation, + } + clusterPropagationPolicyClaimLabels = []string{ + policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel, + } + clusterPropagationPolicyClaimAnnotations = []string{ + policyv1alpha1.ClusterPropagationPolicyAnnotation, + } +) + +// AddPPClaimMetadata adds PropagationPolicy claim metadata, such as labels and annotations +func AddPPClaimMetadata(obj metav1.Object, policyID string, policyMeta metav1.ObjectMeta) { + util.MergeLabel(obj, policyv1alpha1.PropagationPolicyPermanentIDLabel, policyID) + + objectAnnotations := obj.GetAnnotations() + if objectAnnotations == nil { + objectAnnotations = make(map[string]string) + } + objectAnnotations[policyv1alpha1.PropagationPolicyNamespaceAnnotation] = policyMeta.GetNamespace() + objectAnnotations[policyv1alpha1.PropagationPolicyNameAnnotation] = policyMeta.GetName() + obj.SetAnnotations(objectAnnotations) +} + +// AddCPPClaimMetadata adds ClusterPropagationPolicy claim metadata, such as labels and annotations +func AddCPPClaimMetadata(obj metav1.Object, policyID string, policyMeta metav1.ObjectMeta) { + util.MergeLabel(obj, policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel, policyID) + + objectAnnotations := obj.GetAnnotations() + if objectAnnotations == nil { + objectAnnotations = make(map[string]string) + } + objectAnnotations[policyv1alpha1.ClusterPropagationPolicyAnnotation] = policyMeta.GetName() + obj.SetAnnotations(objectAnnotations) +} + +// CleanupPPClaimMetadata removes PropagationPolicy claim metadata, such as labels and annotations +func CleanupPPClaimMetadata(obj metav1.Object) { + util.RemoveLabels(obj, propagationPolicyClaimLabels...) + util.RemoveAnnotations(obj, propagationPolicyClaimAnnotations...) +} + +// CleanupCPPClaimMetadata removes ClusterPropagationPolicy claim metadata, such as labels and annotations +func CleanupCPPClaimMetadata(obj metav1.Object) { + util.RemoveLabels(obj, clusterPropagationPolicyClaimLabels...) + util.RemoveAnnotations(obj, clusterPropagationPolicyClaimAnnotations...) +} diff --git a/pkg/detector/claim_test.go b/pkg/detector/claim_test.go new file mode 100644 index 000000000000..2b27692d0ac1 --- /dev/null +++ b/pkg/detector/claim_test.go @@ -0,0 +1,169 @@ +/* +Copyright 2024 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 detector + +import ( + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" +) + +func TestAddPPClaimMetadata(t *testing.T) { + tests := []struct { + name string + policyID string + policyMeta metav1.ObjectMeta + obj metav1.Object + result metav1.Object + }{ + { + name: "add policy claim metadata", + policyID: "f2507cgb-f3f3-4a4b-b289-5691a4fef979", + policyMeta: metav1.ObjectMeta{Name: "pp-example", Namespace: "test"}, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{policyv1alpha1.PropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979"}, + "annotations": map[string]interface{}{policyv1alpha1.PropagationPolicyNamespaceAnnotation: "test", policyv1alpha1.PropagationPolicyNameAnnotation: "pp-example"}, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + AddPPClaimMetadata(tt.obj, tt.policyID, tt.policyMeta) + assert.Equal(t, tt.obj, tt.result) + }) + } +} + +func TestAddCPPClaimMetadata(t *testing.T) { + tests := []struct { + name string + policyID string + policyMeta metav1.ObjectMeta + obj metav1.Object + result metav1.Object + }{ + { + name: "add cluster policy claim metadata", + policyID: "f2507cgb-f3f3-4a4b-b289-5691a4fef979", + policyMeta: metav1.ObjectMeta{Name: "cpp-example"}, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979"}, + "annotations": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyAnnotation: "cpp-example"}, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + AddCPPClaimMetadata(tt.obj, tt.policyID, tt.policyMeta) + assert.Equal(t, tt.obj, tt.result) + }) + } +} + +func TestCleanupPPClaimMetadata(t *testing.T) { + tests := []struct { + name string + obj metav1.Object + result metav1.Object + }{ + { + name: "clean up policy claim metadata", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{policyv1alpha1.PropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979"}, + "annotations": map[string]interface{}{policyv1alpha1.PropagationPolicyNamespaceAnnotation: "default", policyv1alpha1.PropagationPolicyNameAnnotation: "pp-example"}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + "annotations": map[string]interface{}{}, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + CleanupPPClaimMetadata(tt.obj) + assert.Equal(t, tt.obj, tt.result) + }) + } +} + +func TestCleanupCPPClaimMetadata(t *testing.T) { + tests := []struct { + name string + obj metav1.Object + result metav1.Object + }{ + { + name: "clean up policy claim metadata", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979"}, + "annotations": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyAnnotation: "cpp-example"}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + "annotations": map[string]interface{}{}, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + CleanupCPPClaimMetadata(tt.obj) + assert.Equal(t, tt.obj, tt.result) + }) + } +} diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go index eaf682901bfa..c806d931e45e 100644 --- a/pkg/detector/detector.go +++ b/pkg/detector/detector.go @@ -62,22 +62,6 @@ import ( "github.com/karmada-io/karmada/pkg/util/restmapper" ) -var ( - propagationPolicyMarkedLabels = []string{ - policyv1alpha1.PropagationPolicyPermanentIDLabel, - } - propagationPolicyMarkedAnnotations = []string{ - policyv1alpha1.PropagationPolicyNamespaceAnnotation, - policyv1alpha1.PropagationPolicyNameAnnotation, - } - clusterPropagationPolicyMarkedLabels = []string{ - policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel, - } - clusterPropagationPolicyMarkedAnnotations = []string{ - policyv1alpha1.ClusterPropagationPolicyAnnotation, - } -) - // ResourceDetector is a resource watcher which watches all resources and reconcile the events. type ResourceDetector struct { // DiscoveryClientSet is used to resource discovery. @@ -464,15 +448,7 @@ func (d *ResourceDetector) ApplyPolicy(object *unstructured.Unstructured, object return nil } - policyLabels := map[string]string{ - policyv1alpha1.PropagationPolicyPermanentIDLabel: policyID, - } - policyAnnotations := map[string]string{ - policyv1alpha1.PropagationPolicyNamespaceAnnotation: policy.GetNamespace(), - policyv1alpha1.PropagationPolicyNameAnnotation: policy.GetName(), - } - - binding, err := d.BuildResourceBinding(object, policyLabels, policyAnnotations, &policy.Spec) + binding, err := d.BuildResourceBinding(object, &policy.Spec, policyID, policy.ObjectMeta, AddPPClaimMetadata) if err != nil { klog.Errorf("Failed to build resourceBinding for object: %s. error: %v", objectKey, err) return err @@ -501,7 +477,7 @@ func (d *ResourceDetector) ApplyPolicy(object *unstructured.Unstructured, object bindingCopy.Spec.Failover = binding.Spec.Failover bindingCopy.Spec.ConflictResolution = binding.Spec.ConflictResolution bindingCopy.Spec.Suspension = binding.Spec.Suspension - excludeClusterPolicy(bindingCopy.Labels) + excludeClusterPolicy(bindingCopy) return nil }) if err != nil { @@ -556,18 +532,11 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured, return nil } - policyLabels := map[string]string{ - policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: policyID, - } - policyAnnotations := map[string]string{ - policyv1alpha1.ClusterPropagationPolicyAnnotation: policy.GetName(), - } - // Build `ResourceBinding` or `ClusterResourceBinding` according to the resource template's scope. // For namespace-scoped resources, which namespace is not empty, building `ResourceBinding`. // For cluster-scoped resources, which namespace is empty, building `ClusterResourceBinding`. if object.GetNamespace() != "" { - binding, err := d.BuildResourceBinding(object, policyLabels, policyAnnotations, &policy.Spec) + binding, err := d.BuildResourceBinding(object, &policy.Spec, policyID, policy.ObjectMeta, AddCPPClaimMetadata) if err != nil { klog.Errorf("Failed to build resourceBinding for object: %s. error: %v", objectKey, err) return err @@ -614,7 +583,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured, klog.V(2).Infof("ResourceBinding(%s) is up to date.", binding.GetName()) } } else { - binding, err := d.BuildClusterResourceBinding(object, policyLabels, policyAnnotations, &policy.Spec) + binding, err := d.BuildClusterResourceBinding(object, &policy.Spec, policyID, policy.ObjectMeta) if err != nil { klog.Errorf("Failed to build clusterResourceBinding for object: %s. error: %v", objectKey, err) return err @@ -705,28 +674,16 @@ func (d *ResourceDetector) ClaimPolicyForObject(object *unstructured.Unstructure policyID := policy.Labels[policyv1alpha1.PropagationPolicyPermanentIDLabel] objLabels := object.GetLabels() - if objLabels == nil { - objLabels = make(map[string]string) - } else if len(objLabels) > 0 { + if len(objLabels) > 0 { // object has been claimed, don't need to claim again - if !excludeClusterPolicy(objLabels) && + if !excludeClusterPolicy(object) && objLabels[policyv1alpha1.PropagationPolicyPermanentIDLabel] == policyID { return policyID, nil } } - objLabels[policyv1alpha1.PropagationPolicyPermanentIDLabel] = policyID - - objectAnnotations := object.GetAnnotations() - if objectAnnotations == nil { - objectAnnotations = make(map[string]string) - } - objectAnnotations[policyv1alpha1.PropagationPolicyNamespaceAnnotation] = policy.Namespace - objectAnnotations[policyv1alpha1.PropagationPolicyNameAnnotation] = policy.Name - objectCopy := object.DeepCopy() - objectCopy.SetLabels(objLabels) - objectCopy.SetAnnotations(objectAnnotations) + AddPPClaimMetadata(objectCopy, policyID, policy.ObjectMeta) return policyID, d.Client.Update(context.TODO(), objectCopy) } @@ -741,15 +698,13 @@ func (d *ResourceDetector) ClaimClusterPolicyForObject(object *unstructured.Unst } objectCopy := object.DeepCopy() - util.MergeLabel(objectCopy, policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel, policyID) + AddCPPClaimMetadata(objectCopy, policyID, policy.ObjectMeta) - util.MergeAnnotation(objectCopy, policyv1alpha1.ClusterPropagationPolicyAnnotation, policy.Name) return policyID, d.Client.Update(context.TODO(), objectCopy) } // BuildResourceBinding builds a desired ResourceBinding for object. -func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructured, - labels, annotations map[string]string, policySpec *policyv1alpha1.PropagationSpec) (*workv1alpha2.ResourceBinding, error) { +func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructured, policySpec *policyv1alpha1.PropagationSpec, policyID string, policyMeta metav1.ObjectMeta, claimFunc func(object metav1.Object, policyId string, objectMeta metav1.ObjectMeta)) (*workv1alpha2.ResourceBinding, error) { bindingName := names.GenerateBindingName(object.GetKind(), object.GetName()) propagationBinding := &workv1alpha2.ResourceBinding{ ObjectMeta: metav1.ObjectMeta{ @@ -758,9 +713,7 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure OwnerReferences: []metav1.OwnerReference{ *metav1.NewControllerRef(object, object.GroupVersionKind()), }, - Annotations: annotations, - Labels: labels, - Finalizers: []string{util.BindingControllerFinalizer}, + Finalizers: []string{util.BindingControllerFinalizer}, }, Spec: workv1alpha2.ResourceBindingSpec{ PropagateDeps: policySpec.PropagateDeps, @@ -779,6 +732,7 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure }, }, } + claimFunc(propagationBinding, policyID, policyMeta) if d.ResourceInterpreter.HookEnabled(object.GroupVersionKind(), configv1alpha1.InterpreterOperationInterpretReplica) { replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object) @@ -795,7 +749,7 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure // BuildClusterResourceBinding builds a desired ClusterResourceBinding for object. func (d *ResourceDetector) BuildClusterResourceBinding(object *unstructured.Unstructured, - labels, annotations map[string]string, policySpec *policyv1alpha1.PropagationSpec) (*workv1alpha2.ClusterResourceBinding, error) { + policySpec *policyv1alpha1.PropagationSpec, policyID string, policyMeta metav1.ObjectMeta) (*workv1alpha2.ClusterResourceBinding, error) { bindingName := names.GenerateBindingName(object.GetKind(), object.GetName()) binding := &workv1alpha2.ClusterResourceBinding{ ObjectMeta: metav1.ObjectMeta{ @@ -803,9 +757,7 @@ func (d *ResourceDetector) BuildClusterResourceBinding(object *unstructured.Unst OwnerReferences: []metav1.OwnerReference{ *metav1.NewControllerRef(object, object.GroupVersionKind()), }, - Annotations: annotations, - Labels: labels, - Finalizers: []string{util.ClusterResourceBindingControllerFinalizer}, + Finalizers: []string{util.ClusterResourceBindingControllerFinalizer}, }, Spec: workv1alpha2.ResourceBindingSpec{ PropagateDeps: policySpec.PropagateDeps, @@ -824,6 +776,8 @@ func (d *ResourceDetector) BuildClusterResourceBinding(object *unstructured.Unst }, } + AddCPPClaimMetadata(binding, policyID, policyMeta) + if d.ResourceInterpreter.HookEnabled(object.GroupVersionKind(), configv1alpha1.InterpreterOperationInterpretReplica) { replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object) if err != nil { @@ -1092,7 +1046,7 @@ func (d *ResourceDetector) ReconcileClusterPropagationPolicy(key util.QueueKey) } // HandlePropagationPolicyDeletion handles PropagationPolicy delete event. -// After a policy is removed, the label and annotations marked on relevant resource template will be removed (which gives +// After a policy is removed, the label and annotations claimed on relevant resource template will be removed (which gives // the resource template a change to match another policy). // // Note: The relevant ResourceBinding will continue to exist until the resource template is gone. @@ -1103,26 +1057,22 @@ func (d *ResourceDetector) HandlePropagationPolicyDeletion(policyID string) erro return err } - cleanupMarksFunc := func(obj metav1.Object) { - util.RemoveLabels(obj, propagationPolicyMarkedLabels...) - util.RemoveAnnotations(obj, propagationPolicyMarkedAnnotations...) - } var errs []error for index, binding := range rbs.Items { - // Must remove the marks, such as labels and annotations, from the resource template ahead of ResourceBinding, - // otherwise might lose the chance to do that in a retry loop (in particular, the marks was successfully removed + // Must remove the claim metadata, such as labels and annotations, from the resource template ahead of ResourceBinding, + // otherwise might lose the chance to do that in a retry loop (in particular, the claim metadata was successfully removed // from ResourceBinding, but resource template not), since the ResourceBinding will not be listed again. - if err := d.CleanupResourceTemplateMarks(binding.Spec.Resource, cleanupMarksFunc); err != nil { - klog.Errorf("Failed to clean up marks from resource(%s-%s/%s) when propagationPolicy removed, error: %v", + if err := d.CleanupResourceTemplateClaimMetadata(binding.Spec.Resource, CleanupPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from resource(%s-%s/%s) when propagationPolicy removed, error: %v", binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, err) errs = append(errs, err) // Skip cleaning up policy labels and annotations from ResourceBinding, give a chance to do that in a retry loop. continue } - // Clean up the marks from the reference binding so that the karmada scheduler won't reschedule the binding. - if err := d.CleanupResourceBindingMarks(&rbs.Items[index], cleanupMarksFunc); err != nil { - klog.Errorf("Failed to clean up marks from resource binding(%s/%s) when propagationPolicy removed, error: %v", + // Clean up the claim metadata from the reference binding so that the karmada scheduler won't reschedule the binding. + if err := d.CleanupResourceBindingClaimMetadata(&rbs.Items[index], CleanupPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from resource binding(%s/%s) when propagationPolicy removed, error: %v", binding.Namespace, binding.Name, err) errs = append(errs, err) } @@ -1131,7 +1081,7 @@ func (d *ResourceDetector) HandlePropagationPolicyDeletion(policyID string) erro } // HandleClusterPropagationPolicyDeletion handles ClusterPropagationPolicy delete event. -// After a policy is removed, the label and annotation marked on relevant resource template will be removed (which gives +// After a policy is removed, the label and annotation claimed on relevant resource template will be removed (which gives // the resource template a change to match another policy). // // Note: The relevant ClusterResourceBinding or ResourceBinding will continue to exist until the resource template is gone. @@ -1141,11 +1091,6 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyID strin policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: policyID, } - cleanupMarksFun := func(obj metav1.Object) { - util.RemoveLabels(obj, clusterPropagationPolicyMarkedLabels...) - util.RemoveAnnotations(obj, clusterPropagationPolicyMarkedAnnotations...) - } - // load the ClusterResourceBindings which labeled with current policy crbs, err := helper.GetClusterResourceBindings(d.Client, labelSet) if err != nil { @@ -1153,20 +1098,20 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyID strin errs = append(errs, err) } else if len(crbs.Items) > 0 { for index, binding := range crbs.Items { - // Must remove the marks, such as labels and annotations, from the resource template ahead of + // Must remove the claim metadata, such as labels and annotations, from the resource template ahead of // ClusterResourceBinding, otherwise might lose the chance to do that in a retry loop (in particular, the - // marks was successfully removed from ClusterResourceBinding, but resource template not), since the + // claim metadata was successfully removed from ClusterResourceBinding, but resource template not), since the // ClusterResourceBinding will not be listed again. - if err := d.CleanupResourceTemplateMarks(binding.Spec.Resource, cleanupMarksFun); err != nil { - klog.Errorf("Failed to clean up marks from resource(%s-%s) when clusterPropagationPolicy removed, error: %v", + if err := d.CleanupResourceTemplateClaimMetadata(binding.Spec.Resource, CleanupCPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from resource(%s-%s) when clusterPropagationPolicy removed, error: %v", binding.Spec.Resource.Kind, binding.Spec.Resource.Name, err) // Skip cleaning up policy labels and annotations from ClusterResourceBinding, give a chance to do that in a retry loop. continue } - // Clean up the marks from the reference binding so that the Karmada scheduler won't reschedule the binding. - if err := d.CleanupClusterResourceBindingMarks(&crbs.Items[index], cleanupMarksFun); err != nil { - klog.Errorf("Failed to clean up marks from clusterResourceBinding(%s) when clusterPropagationPolicy removed, error: %v", + // Clean up the claim metadata from the reference binding so that the Karmada scheduler won't reschedule the binding. + if err := d.CleanupClusterResourceBindingClaimMetadata(&crbs.Items[index], CleanupCPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from clusterResourceBinding(%s) when clusterPropagationPolicy removed, error: %v", binding.Name, err) errs = append(errs, err) } @@ -1180,20 +1125,20 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyID strin errs = append(errs, err) } else if len(rbs.Items) > 0 { for index, binding := range rbs.Items { - // Must remove the marks, such as labels and annotations, from the resource template ahead of ResourceBinding, + // Must remove the claim metadata, such as labels and annotations, 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.CleanupResourceTemplateMarks(binding.Spec.Resource, cleanupMarksFun); err != nil { - klog.Errorf("Failed to clean up marks from resource(%s-%s/%s) when clusterPropagationPolicy removed, error: %v", + if err := d.CleanupResourceTemplateClaimMetadata(binding.Spec.Resource, CleanupCPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from resource(%s-%s/%s) when clusterPropagationPolicy removed, error: %v", binding.Spec.Resource.Kind, binding.Spec.Resource.Namespace, binding.Spec.Resource.Name, err) errs = append(errs, err) // Skip cleaning up policy labels and annotations from ResourceBinding, give a chance to do that in a retry loop. continue } - // Clean up the marks from the reference binding so that the Karmada scheduler won't reschedule the binding. - if err := d.CleanupResourceBindingMarks(&rbs.Items[index], cleanupMarksFun); err != nil { - klog.Errorf("Failed to clean up marks from resourceBinding(%s/%s) when clusterPropagationPolicy removed, error: %v", + // Clean up the claim metadata from the reference binding so that the Karmada scheduler won't reschedule the binding. + if err := d.CleanupResourceBindingClaimMetadata(&rbs.Items[index], CleanupCPPClaimMetadata); err != nil { + klog.Errorf("Failed to clean up claim metadata from resourceBinding(%s/%s) when clusterPropagationPolicy removed, error: %v", binding.Namespace, binding.Name, err) errs = append(errs, err) } @@ -1209,7 +1154,7 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyID strin // from waiting list and throw the object to it's reconcile queue. If not, do nothing. // Finally, handle the propagation policy preemption process if preemption is enabled. func (d *ResourceDetector) HandlePropagationPolicyCreationOrUpdate(policy *policyv1alpha1.PropagationPolicy) error { - // If the Policy's ResourceSelectors change, causing certain resources to no longer match the Policy, the label marked + // If the Policy's ResourceSelectors change, causing certain resources to no longer match the Policy, the label claimed // on relevant resource template will be removed (which gives the resource template a change to match another policy). policyID := policy.Labels[policyv1alpha1.PropagationPolicyPermanentIDLabel] err := d.cleanPPUnmatchedRBs(policyID, policy.Namespace, policy.Name, policy.Spec.ResourceSelectors) @@ -1251,7 +1196,7 @@ func (d *ResourceDetector) HandlePropagationPolicyCreationOrUpdate(policy *polic } // If preemption is enabled, handle the preemption process. - // If this policy succeeds in preempting resource managed by other policy, the label marked on relevant resource + // If this policy succeeds in preempting resource managed by other policy, the label claimed on relevant resource // will be replaced, which gives the resource template a change to match to this policy. if preemptionEnabled(policy.Spec.Preemption) { return d.handlePropagationPolicyPreemption(policy) @@ -1267,7 +1212,7 @@ func (d *ResourceDetector) HandlePropagationPolicyCreationOrUpdate(policy *polic // from waiting list and throw the object to it's reconcile queue. If not, do nothing. // Finally, handle the cluster propagation policy preemption process if preemption is enabled. func (d *ResourceDetector) HandleClusterPropagationPolicyCreationOrUpdate(policy *policyv1alpha1.ClusterPropagationPolicy) error { - // If the Policy's ResourceSelectors change, causing certain resources to no longer match the Policy, the label marked + // If the Policy's ResourceSelectors change, causing certain resources to no longer match the Policy, the label claimed // on relevant resource template will be removed (which gives the resource template a change to match another policy). policyID := policy.Labels[policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel] err := d.cleanCPPUnmatchedRBs(policyID, policy.Name, policy.Spec.ResourceSelectors) @@ -1324,7 +1269,7 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyCreationOrUpdate(policy } // If preemption is enabled, handle the preemption process. - // If this policy succeeds in preempting resource managed by other policy, the label marked on relevant resource + // If this policy succeeds in preempting resource managed by other policy, the label claimed on relevant resource // will be replaced, which gives the resource template a change to match to this policy. if preemptionEnabled(policy.Spec.Preemption) { return d.handleClusterPropagationPolicyPreemption(policy) @@ -1333,8 +1278,8 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyCreationOrUpdate(policy return nil } -// CleanupResourceTemplateMarks removes marks, such as labels and annotations, from object referencing by objRef. -func (d *ResourceDetector) CleanupResourceTemplateMarks(objRef workv1alpha2.ObjectReference, cleanupFunc func(obj metav1.Object)) error { +// CleanupResourceTemplateClaimMetadata removes claim metadata, such as labels and annotations, from object referencing by objRef. +func (d *ResourceDetector) CleanupResourceTemplateClaimMetadata(objRef workv1alpha2.ObjectReference, cleanupFunc func(obj metav1.Object)) error { gvr, err := restmapper.GetGroupVersionResource(d.RESTMapper, schema.FromAPIVersionAndKind(objRef.APIVersion, objRef.Kind)) if err != nil { klog.Errorf("Failed to convert GVR from GVK(%s/%s), err: %v", objRef.APIVersion, objRef.Kind, err) @@ -1364,8 +1309,8 @@ func (d *ResourceDetector) CleanupResourceTemplateMarks(objRef workv1alpha2.Obje }) } -// CleanupResourceBindingMarks removes marks, such as labels and annotations, from resource binding. -func (d *ResourceDetector) CleanupResourceBindingMarks(rb *workv1alpha2.ResourceBinding, cleanupFunc func(obj metav1.Object)) error { +// CleanupResourceBindingClaimMetadata removes claim metadata, such as labels and annotations, from resource binding. +func (d *ResourceDetector) CleanupResourceBindingClaimMetadata(rb *workv1alpha2.ResourceBinding, cleanupFunc func(obj metav1.Object)) error { return retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { cleanupFunc(rb) updateErr := d.Client.Update(context.TODO(), rb) @@ -1383,8 +1328,8 @@ func (d *ResourceDetector) CleanupResourceBindingMarks(rb *workv1alpha2.Resource }) } -// CleanupClusterResourceBindingMarks removes marks, such as labels and annotations, from cluster resource binding. -func (d *ResourceDetector) CleanupClusterResourceBindingMarks(crb *workv1alpha2.ClusterResourceBinding, cleanupFunc func(obj metav1.Object)) error { +// CleanupClusterResourceBindingClaimMetadata removes claim metadata, such as labels and annotations, from cluster resource binding. +func (d *ResourceDetector) CleanupClusterResourceBindingClaimMetadata(crb *workv1alpha2.ClusterResourceBinding, cleanupFunc func(obj metav1.Object)) error { return retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { cleanupFunc(crb) updateErr := d.Client.Update(context.TODO(), crb) diff --git a/pkg/detector/policy.go b/pkg/detector/policy.go index 47b1077e62ec..1a1b7c69c3fe 100644 --- a/pkg/detector/policy.go +++ b/pkg/detector/policy.go @@ -22,6 +22,7 @@ import ( "time" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/errors" @@ -187,7 +188,7 @@ func (d *ResourceDetector) cleanPPUnmatchedRBs(policyID, policyNamespace, policy return err } - return d.removeRBsMarks(bindings, selectors, propagationPolicyMarkedLabels, propagationPolicyMarkedAnnotations) + return d.removeRBsClaimMetadata(bindings, selectors, propagationPolicyClaimLabels, propagationPolicyClaimAnnotations) } func (d *ResourceDetector) cleanCPPUnmatchedRBs(policyID, policyName string, selectors []policyv1alpha1.ResourceSelector) error { @@ -196,7 +197,7 @@ func (d *ResourceDetector) cleanCPPUnmatchedRBs(policyID, policyName string, sel return err } - return d.removeRBsMarks(bindings, selectors, clusterPropagationPolicyMarkedLabels, clusterPropagationPolicyMarkedAnnotations) + return d.removeRBsClaimMetadata(bindings, selectors, clusterPropagationPolicyClaimLabels, clusterPropagationPolicyClaimAnnotations) } func (d *ResourceDetector) cleanUnmatchedCRBs(policyID, policyName string, selectors []policyv1alpha1.ResourceSelector) error { @@ -205,13 +206,13 @@ func (d *ResourceDetector) cleanUnmatchedCRBs(policyID, policyName string, selec return err } - return d.removeCRBsMarks(bindings, selectors, clusterPropagationPolicyMarkedLabels, clusterPropagationPolicyMarkedAnnotations) + return d.removeCRBsClaimMetadata(bindings, selectors, clusterPropagationPolicyClaimLabels, clusterPropagationPolicyClaimAnnotations) } -func (d *ResourceDetector) removeRBsMarks(bindings *workv1alpha2.ResourceBindingList, selectors []policyv1alpha1.ResourceSelector, labels, annotations []string) error { +func (d *ResourceDetector) removeRBsClaimMetadata(bindings *workv1alpha2.ResourceBindingList, selectors []policyv1alpha1.ResourceSelector, labels, annotations []string) error { var errs []error for _, binding := range bindings.Items { - removed, err := d.removeResourceMarksIfNotMatched(binding.Spec.Resource, selectors, labels, annotations) + removed, err := d.removeResourceClaimMetadataIfNotMatched(binding.Spec.Resource, selectors, labels, annotations) if err != nil { klog.Errorf("Failed to remove resource labels and annotations when resource not match with policy selectors, err: %v", err) errs = append(errs, err) @@ -234,11 +235,11 @@ func (d *ResourceDetector) removeRBsMarks(bindings *workv1alpha2.ResourceBinding return errors.NewAggregate(errs) } -func (d *ResourceDetector) removeCRBsMarks(bindings *workv1alpha2.ClusterResourceBindingList, +func (d *ResourceDetector) removeCRBsClaimMetadata(bindings *workv1alpha2.ClusterResourceBindingList, selectors []policyv1alpha1.ResourceSelector, removeLabels, removeAnnotations []string) error { var errs []error for _, binding := range bindings.Items { - removed, err := d.removeResourceMarksIfNotMatched(binding.Spec.Resource, selectors, removeLabels, removeAnnotations) + removed, err := d.removeResourceClaimMetadataIfNotMatched(binding.Spec.Resource, selectors, removeLabels, removeAnnotations) if err != nil { klog.Errorf("Failed to remove resource labels and annotations when resource not match with policy selectors, err: %v", err) errs = append(errs, err) @@ -261,7 +262,7 @@ func (d *ResourceDetector) removeCRBsMarks(bindings *workv1alpha2.ClusterResourc return errors.NewAggregate(errs) } -func (d *ResourceDetector) removeResourceMarksIfNotMatched(objectReference workv1alpha2.ObjectReference, +func (d *ResourceDetector) removeResourceClaimMetadataIfNotMatched(objectReference workv1alpha2.ObjectReference, selectors []policyv1alpha1.ResourceSelector, labels, annotations []string) (bool, error) { objectKey, err := helper.ConstructClusterWideKey(objectReference) if err != nil { @@ -340,10 +341,10 @@ func (d *ResourceDetector) listCPPDerivedCRBs(policyID, policyName string) (*wor // excludeClusterPolicy excludes cluster propagation policy. // If propagation policy was claimed, cluster propagation policy should not exist. -func excludeClusterPolicy(objLabels map[string]string) bool { - if _, ok := objLabels[policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel]; !ok { +func excludeClusterPolicy(obj metav1.Object) (hasClaimedClusterPolicy bool) { + if _, ok := obj.GetLabels()[policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel]; !ok { return false } - delete(objLabels, policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel) + CleanupCPPClaimMetadata(obj) return true } diff --git a/pkg/detector/policy_test.go b/pkg/detector/policy_test.go index df5722cf48c8..90cc8bc9b9cb 100644 --- a/pkg/detector/policy_test.go +++ b/pkg/detector/policy_test.go @@ -447,7 +447,7 @@ func Test_cleanUnmatchedCRBs(t *testing.T) { } } -func Test_removeRBsMarks(t *testing.T) { +func Test_removeRBsClaimMetadata(t *testing.T) { scheme := runtime.NewScheme() utilruntime.Must(clientgoscheme.AddToScheme(scheme)) utilruntime.Must(appsv1.AddToScheme(scheme)) @@ -664,15 +664,15 @@ func Test_removeRBsMarks(t *testing.T) { RESTMapper: fakeClient.RESTMapper(), InformerManager: genMgr, } - err := resourceDetector.removeRBsMarks(tt.bindings, tt.selectors, tt.removeLabels, tt.removeAnnotations) + err := resourceDetector.removeRBsClaimMetadata(tt.bindings, tt.selectors, tt.removeLabels, tt.removeAnnotations) if (err != nil) != tt.wantErr { - t.Errorf("removeRBsMarks() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("removeRBsClaimMetadata() error = %v, wantErr %v", err, tt.wantErr) } }) } } -func Test_removeCRBsMarks(t *testing.T) { +func Test_removeCRBsClaimMetadata(t *testing.T) { scheme := runtime.NewScheme() utilruntime.Must(clientgoscheme.AddToScheme(scheme)) utilruntime.Must(appsv1.AddToScheme(scheme)) @@ -889,15 +889,15 @@ func Test_removeCRBsMarks(t *testing.T) { RESTMapper: fakeClient.RESTMapper(), InformerManager: genMgr, } - err := resourceDetector.removeCRBsMarks(tt.bindings, tt.selectors, tt.removeLabels, tt.removeAnnotations) + err := resourceDetector.removeCRBsClaimMetadata(tt.bindings, tt.selectors, tt.removeLabels, tt.removeAnnotations) if (err != nil) != tt.wantErr { - t.Errorf("removeCRBsMarks() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("removeCRBsClaimMetadata() error = %v, wantErr %v", err, tt.wantErr) } }) } } -func Test_removeResourceMarksIfNotMatched(t *testing.T) { +func Test_removeResourceClaimMetadataIfNotMatched(t *testing.T) { scheme := runtime.NewScheme() utilruntime.Must(clientgoscheme.AddToScheme(scheme)) utilruntime.Must(appsv1.AddToScheme(scheme)) @@ -1113,13 +1113,13 @@ func Test_removeResourceMarksIfNotMatched(t *testing.T) { InformerManager: genMgr, } - updated, err := resourceDetector.removeResourceMarksIfNotMatched(tt.objectReference, tt.selectors, tt.labels, tt.annotations) + updated, err := resourceDetector.removeResourceClaimMetadataIfNotMatched(tt.objectReference, tt.selectors, tt.labels, tt.annotations) if (err != nil) != tt.wantErr { - t.Errorf("removeResourceMarksIfNotMatched() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("removeResourceClaimMetadataIfNotMatched() error = %v, wantErr %v", err, tt.wantErr) } if updated != tt.wantUpdated { - t.Errorf("removeResourceMarksIfNotMatched() = %v, want %v", updated, tt.wantUpdated) + t.Errorf("removeResourceClaimMetadataIfNotMatched() = %v, want %v", updated, tt.wantUpdated) } }) } @@ -1363,26 +1363,54 @@ func Test_listCPPDerivedCRBs(t *testing.T) { func Test_excludeClusterPolicy(t *testing.T) { tests := []struct { - name string - objLabels map[string]string - want bool + name string + obj metav1.Object + result metav1.Object + hasClaimedClusterPolicy bool }{ { - name: "propagation policy was claimed", - objLabels: map[string]string{}, - want: false, + name: "propagation policy was claimed", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{}, + }, + }, + }, + hasClaimedClusterPolicy: false, }, { name: "propagation policy was not claimed", - objLabels: map[string]string{ - policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyPermanentIDLabel: "f2507cgb-f3f3-4a4b-b289-5691a4fef979", "foo": "bar"}, + "annotations": map[string]interface{}{policyv1alpha1.ClusterPropagationPolicyAnnotation: "nginx", "foo1": "bar1"}, + }, + }, + }, + result: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{"foo": "bar"}, + "annotations": map[string]interface{}{"foo1": "bar1"}, + }, + }, }, - want: true, + hasClaimedClusterPolicy: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := excludeClusterPolicy(tt.objLabels) - assert.Equal(t, tt.want, got) + got := excludeClusterPolicy(tt.obj) + assert.Equal(t, tt.obj, tt.result) + assert.Equal(t, tt.hasClaimedClusterPolicy, got) }) } }