Skip to content

Commit

Permalink
Merge pull request #6055 from whitewindmills/automated-cherry-pick-of…
Browse files Browse the repository at this point in the history 
…-#6034-upstream-release-1.12

Automated cherry pick of #6034: fix the attached binding deletion problem
  • Loading branch information
@karmada-bot
karmada-bot authored Jan 16, 2025
2 parents bbb665b + 10b67db commit 9317f30
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 1 deletion.
7 changes: 7 additions & 0 deletions pkg/dependenciesdistributor/dependencies_distributor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,13 @@ 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 {
Expand Down
161 changes: 160 additions & 1 deletion pkg/dependenciesdistributor/dependencies_distributor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ 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"
Expand Down Expand Up @@ -2224,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{
Expand Down Expand Up @@ -2274,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{
Expand Down Expand Up @@ -2344,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{
Expand Down Expand Up @@ -2390,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{
Expand Down Expand Up @@ -2440,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{
Expand Down Expand Up @@ -2495,6 +2526,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{
Expand Down Expand Up @@ -2546,6 +2583,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{
Expand Down Expand Up @@ -2617,6 +2660,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{
Expand Down Expand Up @@ -2656,6 +2705,113 @@ func Test_createOrUpdateAttachedBinding(t *testing.T) {
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) {
Expand All @@ -2666,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) {
Expand Down

0 comments on commit 9317f30

Please sign in to comment.