Skip to content

Commit 2df42bd

Browse files
karmada-botkarmada-bot
authored
Merge pull request #6034 from whitewindmills/dependency_pr
fix the attached binding deletion problem
2 parents 272447e + b10c102 commit 2df42bd

File tree

2 files changed

+167
-1
lines changed

2 files changed

+167
-1
lines changed

‎pkg/dependenciesdistributor/dependencies_distributor.go

+7
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,13 @@ func (d *DependenciesDistributor) createOrUpdateAttachedBinding(attachedBinding
568568
bindingKey := client.ObjectKeyFromObject(attachedBinding)
569569
err := d.Client.Get(context.TODO(), bindingKey, existBinding)
570570
if err == nil {
571+
// If this binding exists and its owner is not the input object, return error and let garbage collector
572+
// delete this binding and try again later. See https://github.com/karmada-io/karmada/issues/6034.
573+
if ownerRef := metav1.GetControllerOfNoCopy(existBinding); ownerRef != nil && ownerRef.UID != attachedBinding.OwnerReferences[0].UID {
574+
return fmt.Errorf("failed to update resourceBinding(%s) due to different owner reference UID, will "+
575+
"try again later after binding is garbage collected, see https://github.com/karmada-io/karmada/issues/6034", bindingKey)
576+
}
577+
571578
// If the spec.Placement is nil, this means that existBinding is generated by the dependency mechanism.
572579
// If the spec.Placement is not nil, then it must be generated by PropagationPolicy.
573580
if existBinding.Spec.Placement == nil {

‎pkg/dependenciesdistributor/dependencies_distributor_test.go

+160-1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
"k8s.io/client-go/dynamic"
3535
dynamicfake "k8s.io/client-go/dynamic/fake"
3636
"k8s.io/client-go/kubernetes/scheme"
37+
"k8s.io/utils/ptr"
3738
"sigs.k8s.io/controller-runtime/pkg/client"
3839
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3940
"sigs.k8s.io/controller-runtime/pkg/event"
@@ -2224,6 +2225,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
22242225
Namespace: "test",
22252226
ResourceVersion: "1000",
22262227
Labels: map[string]string{"app": "nginx"},
2228+
OwnerReferences: []metav1.OwnerReference{
2229+
{
2230+
UID: "foo-bar",
2231+
Controller: ptr.To[bool](true),
2232+
},
2233+
},
22272234
},
22282235
Spec: workv1alpha2.ResourceBindingSpec{
22292236
Resource: workv1alpha2.ObjectReference{
@@ -2274,6 +2281,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
22742281
Namespace: "test",
22752282
ResourceVersion: "1001",
22762283
Labels: map[string]string{"app": "nginx", "foo": "bar"},
2284+
OwnerReferences: []metav1.OwnerReference{
2285+
{
2286+
UID: "foo-bar",
2287+
Controller: ptr.To[bool](true),
2288+
},
2289+
},
22772290
},
22782291
Spec: workv1alpha2.ResourceBindingSpec{
22792292
Resource: workv1alpha2.ObjectReference{
@@ -2344,6 +2357,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
23442357
Namespace: "test",
23452358
ResourceVersion: "1000",
23462359
Labels: map[string]string{"foo": "bar"},
2360+
OwnerReferences: []metav1.OwnerReference{
2361+
{
2362+
UID: "foo-bar",
2363+
Controller: ptr.To[bool](true),
2364+
},
2365+
},
23472366
},
23482367
Spec: workv1alpha2.ResourceBindingSpec{
23492368
RequiredBy: []workv1alpha2.BindingSnapshot{
@@ -2390,6 +2409,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
23902409
Name: "test-binding",
23912410
Namespace: "test",
23922411
Labels: map[string]string{"app": "nginx"},
2412+
OwnerReferences: []metav1.OwnerReference{
2413+
{
2414+
UID: "foo-bar",
2415+
Controller: ptr.To[bool](true),
2416+
},
2417+
},
23932418
},
23942419
Spec: workv1alpha2.ResourceBindingSpec{
23952420
Resource: workv1alpha2.ObjectReference{
@@ -2440,6 +2465,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
24402465
Namespace: "test",
24412466
ResourceVersion: "1",
24422467
Labels: map[string]string{"app": "nginx"},
2468+
OwnerReferences: []metav1.OwnerReference{
2469+
{
2470+
UID: "foo-bar",
2471+
Controller: ptr.To[bool](true),
2472+
},
2473+
},
24432474
},
24442475
Spec: workv1alpha2.ResourceBindingSpec{
24452476
Resource: workv1alpha2.ObjectReference{
@@ -2495,6 +2526,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
24952526
Namespace: "test",
24962527
ResourceVersion: "1000",
24972528
Labels: map[string]string{"app": "nginx"},
2529+
OwnerReferences: []metav1.OwnerReference{
2530+
{
2531+
UID: "foo-bar",
2532+
Controller: ptr.To[bool](true),
2533+
},
2534+
},
24982535
},
24992536
Spec: workv1alpha2.ResourceBindingSpec{
25002537
Resource: workv1alpha2.ObjectReference{
@@ -2546,6 +2583,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
25462583
Namespace: "test",
25472584
ResourceVersion: "1001",
25482585
Labels: map[string]string{"app": "nginx", "foo": "bar"},
2586+
OwnerReferences: []metav1.OwnerReference{
2587+
{
2588+
UID: "foo-bar",
2589+
Controller: ptr.To[bool](true),
2590+
},
2591+
},
25492592
},
25502593
Spec: workv1alpha2.ResourceBindingSpec{
25512594
Resource: workv1alpha2.ObjectReference{
@@ -2617,6 +2660,12 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
26172660
Namespace: "test",
26182661
ResourceVersion: "1000",
26192662
Labels: map[string]string{"foo": "bar"},
2663+
OwnerReferences: []metav1.OwnerReference{
2664+
{
2665+
UID: "foo-bar",
2666+
Controller: ptr.To[bool](true),
2667+
},
2668+
},
26202669
},
26212670
Spec: workv1alpha2.ResourceBindingSpec{
26222671
RequiredBy: []workv1alpha2.BindingSnapshot{
@@ -2656,6 +2705,113 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
26562705
return fake.NewClientBuilder().WithScheme(Scheme).WithObjects(rb).Build()
26572706
},
26582707
},
2708+
{
2709+
name: "update attached binding which is being deleted",
2710+
attachedBinding: &workv1alpha2.ResourceBinding{
2711+
ObjectMeta: metav1.ObjectMeta{
2712+
Name: "test-binding",
2713+
Namespace: "test",
2714+
ResourceVersion: "1000",
2715+
Labels: map[string]string{"app": "nginx"},
2716+
OwnerReferences: []metav1.OwnerReference{
2717+
{
2718+
UID: "foo-bar",
2719+
Controller: ptr.To[bool](true),
2720+
},
2721+
},
2722+
},
2723+
Spec: workv1alpha2.ResourceBindingSpec{
2724+
Resource: workv1alpha2.ObjectReference{
2725+
APIVersion: "apps/v1",
2726+
Kind: "Deployment",
2727+
Namespace: "fake-ns",
2728+
Name: "demo-app",
2729+
ResourceVersion: "22222",
2730+
},
2731+
RequiredBy: []workv1alpha2.BindingSnapshot{
2732+
{
2733+
Namespace: "test-1",
2734+
Name: "test-binding-1",
2735+
Clusters: []workv1alpha2.TargetCluster{
2736+
{
2737+
Name: "foo",
2738+
Replicas: 1,
2739+
},
2740+
},
2741+
},
2742+
},
2743+
ConflictResolution: policyv1alpha1.ConflictOverwrite,
2744+
},
2745+
},
2746+
wantErr: true,
2747+
wantBinding: &workv1alpha2.ResourceBinding{
2748+
ObjectMeta: metav1.ObjectMeta{
2749+
Name: "test-binding",
2750+
Namespace: "test",
2751+
ResourceVersion: "1001",
2752+
Labels: map[string]string{"app": "nginx", "foo": "bar"},
2753+
OwnerReferences: []metav1.OwnerReference{
2754+
{
2755+
UID: "foo-bar",
2756+
Controller: ptr.To[bool](true),
2757+
},
2758+
},
2759+
},
2760+
Spec: workv1alpha2.ResourceBindingSpec{
2761+
Resource: workv1alpha2.ObjectReference{
2762+
APIVersion: "apps/v1",
2763+
Kind: "Deployment",
2764+
Namespace: "fake-ns",
2765+
Name: "demo-app",
2766+
ResourceVersion: "22222",
2767+
},
2768+
RequiredBy: []workv1alpha2.BindingSnapshot{
2769+
{
2770+
Namespace: "default-1",
2771+
Name: "default-binding-1",
2772+
Clusters: []workv1alpha2.TargetCluster{
2773+
{
2774+
Name: "member1",
2775+
Replicas: 2,
2776+
},
2777+
},
2778+
},
2779+
},
2780+
ConflictResolution: policyv1alpha1.ConflictOverwrite,
2781+
},
2782+
},
2783+
setupClient: func() client.Client {
2784+
rb := &workv1alpha2.ResourceBinding{
2785+
ObjectMeta: metav1.ObjectMeta{
2786+
Name: "test-binding",
2787+
Namespace: "test",
2788+
ResourceVersion: "1000",
2789+
Labels: map[string]string{"foo": "bar"},
2790+
OwnerReferences: []metav1.OwnerReference{
2791+
{
2792+
UID: "bar-foo",
2793+
Controller: ptr.To[bool](true),
2794+
},
2795+
},
2796+
},
2797+
Spec: workv1alpha2.ResourceBindingSpec{
2798+
RequiredBy: []workv1alpha2.BindingSnapshot{
2799+
{
2800+
Namespace: "default-1",
2801+
Name: "default-binding-1",
2802+
Clusters: []workv1alpha2.TargetCluster{
2803+
{
2804+
Name: "member1",
2805+
Replicas: 2,
2806+
},
2807+
},
2808+
},
2809+
},
2810+
},
2811+
}
2812+
return fake.NewClientBuilder().WithScheme(Scheme).WithObjects(rb).Build()
2813+
},
2814+
},
26592815
}
26602816
for _, tt := range tests {
26612817
t.Run(tt.name, func(t *testing.T) {
@@ -2666,10 +2822,13 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
26662822
if (err != nil) != tt.wantErr {
26672823
t.Errorf("createOrUpdateAttachedBinding() error = %v, wantErr %v", err, tt.wantErr)
26682824
}
2825+
if tt.wantErr {
2826+
return
2827+
}
26692828
existBinding := &workv1alpha2.ResourceBinding{}
26702829
bindingKey := client.ObjectKeyFromObject(tt.attachedBinding)
26712830
err = d.Client.Get(context.TODO(), bindingKey, existBinding)
2672-
if (err != nil) != tt.wantErr {
2831+
if err != nil {
26732832
t.Errorf("createOrUpdateAttachedBinding(), Client.Get() error = %v, wantErr %v", err, tt.wantErr)
26742833
}
26752834
if !reflect.DeepEqual(existBinding, tt.wantBinding) {

0 commit comments

Comments
 (0)