diff --git a/pkg/dependenciesdistributor/dependencies_distributor.go b/pkg/dependenciesdistributor/dependencies_distributor.go index bb3e1b17fe22..dcd1a747e0ed 100644 --- a/pkg/dependenciesdistributor/dependencies_distributor.go +++ b/pkg/dependenciesdistributor/dependencies_distributor.go @@ -560,6 +560,18 @@ func (d *DependenciesDistributor) createOrUpdateAttachedBinding(attachedBinding bindingKey := client.ObjectKeyFromObject(attachedBinding) err := d.Client.Get(context.TODO(), bindingKey, existBinding) if err == nil { + // If this binding exists and its owner is not the input object, return error and let garbage collector + // delete this binding and try again later. See https://github.com/karmada-io/karmada/issues/6034. + if ownerRef := metav1.GetControllerOfNoCopy(existBinding); ownerRef != nil && ownerRef.UID != attachedBinding.OwnerReferences[0].UID { + return fmt.Errorf("failed to update resourceBinding(%s) due to different owner reference UID, will "+ + "try again later after binding is garbage collected, see https://github.com/karmada-io/karmada/issues/6034", bindingKey) + } + + // If the spec.Placement is nil, this means that existBinding is generated by the dependency mechanism. + // If the spec.Placement is not nil, then it must be generated by PropagationPolicy. + if existBinding.Spec.Placement == nil { + existBinding.Spec.ConflictResolution = attachedBinding.Spec.ConflictResolution + } existBinding.Spec.RequiredBy = mergeBindingSnapshot(existBinding.Spec.RequiredBy, attachedBinding.Spec.RequiredBy) existBinding.Labels = util.DedupeAndMergeLabels(existBinding.Labels, attachedBinding.Labels) existBinding.Spec.Resource = attachedBinding.Spec.Resource diff --git a/pkg/dependenciesdistributor/dependencies_distributor_test.go b/pkg/dependenciesdistributor/dependencies_distributor_test.go index c1419835c624..31443d69bd60 100644 --- a/pkg/dependenciesdistributor/dependencies_distributor_test.go +++ b/pkg/dependenciesdistributor/dependencies_distributor_test.go @@ -34,11 +34,13 @@ import ( "k8s.io/client-go/dynamic" dynamicfake "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/event" configv1alpha1 "github.com/karmada-io/karmada/pkg/apis/config/v1alpha1" + policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" "github.com/karmada-io/karmada/pkg/util" "github.com/karmada-io/karmada/pkg/util/fedinformer/genericmanager" @@ -2223,6 +2225,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { Namespace: "test", ResourceVersion: "1000", Labels: map[string]string{"app": "nginx"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, }, Spec: workv1alpha2.ResourceBindingSpec{ Resource: workv1alpha2.ObjectReference{ @@ -2273,6 +2281,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { Namespace: "test", ResourceVersion: "1001", Labels: map[string]string{"app": "nginx", "foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, }, Spec: workv1alpha2.ResourceBindingSpec{ Resource: workv1alpha2.ObjectReference{ @@ -2343,6 +2357,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { Namespace: "test", ResourceVersion: "1000", Labels: map[string]string{"foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, }, Spec: workv1alpha2.ResourceBindingSpec{ RequiredBy: []workv1alpha2.BindingSnapshot{ @@ -2389,6 +2409,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { Name: "test-binding", Namespace: "test", Labels: map[string]string{"app": "nginx"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, }, Spec: workv1alpha2.ResourceBindingSpec{ Resource: workv1alpha2.ObjectReference{ @@ -2439,6 +2465,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { Namespace: "test", ResourceVersion: "1", Labels: map[string]string{"app": "nginx"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, }, Spec: workv1alpha2.ResourceBindingSpec{ Resource: workv1alpha2.ObjectReference{ @@ -2486,6 +2518,300 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { return fake.NewClientBuilder().WithScheme(Scheme).Build() }, }, + { + name: "update attached binding with ConflictResolution", + attachedBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1000", + Labels: map[string]string{"app": "nginx"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "fake-ns", + Name: "demo-app", + ResourceVersion: "22222", + }, + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "test-1", + Name: "test-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "foo", + Replicas: 1, + }, + }, + }, + { + Namespace: "default-2", + Name: "default-binding-2", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member2", + Replicas: 4, + }, + }, + }, + { + Namespace: "test-2", + Name: "test-binding-2", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "bar", + Replicas: 1, + }, + }, + }, + }, + ConflictResolution: policyv1alpha1.ConflictOverwrite, + }, + }, + wantErr: false, + wantBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1001", + Labels: map[string]string{"app": "nginx", "foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "fake-ns", + Name: "demo-app", + ResourceVersion: "22222", + }, + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "default-1", + Name: "default-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member1", + Replicas: 2, + }, + }, + }, + { + Namespace: "default-2", + Name: "default-binding-2", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member2", + Replicas: 4, + }, + }, + }, + { + Namespace: "default-3", + Name: "default-binding-3", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member3", + Replicas: 4, + }, + }, + }, + { + Namespace: "test-1", + Name: "test-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "foo", + Replicas: 1, + }, + }, + }, + { + Namespace: "test-2", + Name: "test-binding-2", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "bar", + Replicas: 1, + }, + }, + }, + }, + ConflictResolution: policyv1alpha1.ConflictOverwrite, + }, + }, + setupClient: func() client.Client { + rb := &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1000", + Labels: map[string]string{"foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "default-1", + Name: "default-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member1", + Replicas: 2, + }, + }, + }, + { + Namespace: "default-2", + Name: "default-binding-2", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member2", + Replicas: 3, + }, + }, + }, + { + Namespace: "default-3", + Name: "default-binding-3", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member3", + Replicas: 4, + }, + }, + }, + }, + }, + } + return fake.NewClientBuilder().WithScheme(Scheme).WithObjects(rb).Build() + }, + }, + { + name: "update attached binding which is being deleted", + attachedBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1000", + Labels: map[string]string{"app": "nginx"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "fake-ns", + Name: "demo-app", + ResourceVersion: "22222", + }, + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "test-1", + Name: "test-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "foo", + Replicas: 1, + }, + }, + }, + }, + ConflictResolution: policyv1alpha1.ConflictOverwrite, + }, + }, + wantErr: true, + wantBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1001", + Labels: map[string]string{"app": "nginx", "foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foo-bar", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "fake-ns", + Name: "demo-app", + ResourceVersion: "22222", + }, + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "default-1", + Name: "default-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member1", + Replicas: 2, + }, + }, + }, + }, + ConflictResolution: policyv1alpha1.ConflictOverwrite, + }, + }, + setupClient: func() client.Client { + rb := &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test", + ResourceVersion: "1000", + Labels: map[string]string{"foo": "bar"}, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "bar-foo", + Controller: ptr.To[bool](true), + }, + }, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "default-1", + Name: "default-binding-1", + Clusters: []workv1alpha2.TargetCluster{ + { + Name: "member1", + Replicas: 2, + }, + }, + }, + }, + }, + } + return fake.NewClientBuilder().WithScheme(Scheme).WithObjects(rb).Build() + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2496,10 +2822,13 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) { if (err != nil) != tt.wantErr { t.Errorf("createOrUpdateAttachedBinding() error = %v, wantErr %v", err, tt.wantErr) } + if tt.wantErr { + return + } existBinding := &workv1alpha2.ResourceBinding{} bindingKey := client.ObjectKeyFromObject(tt.attachedBinding) err = d.Client.Get(context.TODO(), bindingKey, existBinding) - if (err != nil) != tt.wantErr { + if err != nil { t.Errorf("createOrUpdateAttachedBinding(), Client.Get() error = %v, wantErr %v", err, tt.wantErr) } if !reflect.DeepEqual(existBinding, tt.wantBinding) {