diff --git a/pkg/dependenciesdistributor/dependencies_distributor.go b/pkg/dependenciesdistributor/dependencies_distributor.go index 9df80b1cde05..3e573ccf698e 100644 --- a/pkg/dependenciesdistributor/dependencies_distributor.go +++ b/pkg/dependenciesdistributor/dependencies_distributor.go @@ -558,6 +558,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 4e57622ffa42..6b60434114d5 100644 --- a/pkg/dependenciesdistributor/dependencies_distributor_test.go +++ b/pkg/dependenciesdistributor/dependencies_distributor_test.go @@ -24,12 +24,18 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "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" 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/fedinformer/genericmanager" "github.com/karmada-io/karmada/pkg/util/fedinformer/keys" @@ -257,3 +263,635 @@ func TestDependenciesDistributor_findOrphanAttachedBindingsByDependencies(t *tes }) } } + +func Test_createOrUpdateAttachedBinding(t *testing.T) { + Scheme := runtime.NewScheme() + utilruntime.Must(scheme.AddToScheme(Scheme)) + utilruntime.Must(workv1alpha2.Install(Scheme)) + tests := []struct { + name string + attachedBinding *workv1alpha2.ResourceBinding + wantErr bool + wantBinding *workv1alpha2.ResourceBinding + setupClient func() client.Client + }{ + { + name: "update attached binding", + 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, + }, + }, + }, + }, + }, + }, + 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, + }, + }, + }, + }, + }, + }, + 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: "create attached binding", + attachedBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + 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{ + 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, + }, + }, + }, + }, + }, + }, + wantErr: false, + wantBinding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + 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{ + 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, + }, + }, + }, + }, + }, + }, + setupClient: func() client.Client { + 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) { + d := &DependenciesDistributor{ + Client: tt.setupClient(), + } + err := d.createOrUpdateAttachedBinding(tt.attachedBinding) + 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 { + t.Errorf("createOrUpdateAttachedBinding(), Client.Get() error = %v, wantErr %v", err, tt.wantErr) + } + if !reflect.DeepEqual(existBinding, tt.wantBinding) { + t.Errorf("createOrUpdateAttachedBinding(), Client.Get() = %v, want %v", existBinding, tt.wantBinding) + } + }) + } +}