From 96df6820bd2110db925800b54136ea5d2c119021 Mon Sep 17 00:00:00 2001 From: zoumo Date: Thu, 11 Sep 2025 21:16:41 +0800 Subject: [PATCH] fix(refmanager): adopt() should update child not parent object --- controller/refmanager/ref_manager_test.go | 49 +++++++++++++++++----- controller/refmanager/writer.go | 2 +- controller/refmanager/writer_test.go | 50 ++++++++++++++--------- 3 files changed, 70 insertions(+), 31 deletions(-) diff --git a/controller/refmanager/ref_manager_test.go b/controller/refmanager/ref_manager_test.go index d985294d..713b7adb 100644 --- a/controller/refmanager/ref_manager_test.go +++ b/controller/refmanager/ref_manager_test.go @@ -21,6 +21,7 @@ import ( "fmt" "github.com/samber/lo" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -250,7 +251,7 @@ func (s *ownerReferenceTestSute) TestClaimWhenCanAdoptFailed() { func (s *ownerReferenceTestSute) TestClaimAllOf() { writer := NewOwnerRefWriter(s.client) match, err := LabelSelectorAsMatch(s.owner.Spec.Selector) - s.NoError(err) + s.Require().NoError(err) m := NewObjectControllerRefManager(writer, s.owner, s.ownerGVK, match) adopted := []metav1.Object{ @@ -273,15 +274,43 @@ func (s *ownerReferenceTestSute) TestClaimAllOf() { allObjects = append(allObjects, released...) allObjects = append(allObjects, noChanged...) + for _, obj := range allObjects { + err := s.client.Create(context.TODO(), obj.(client.Object)) + s.Require().NoError(err) + } + result, err := m.ClaimAllOf(context.Background(), allObjects) - if s.NoError(err) { - s.Len(result, len(adopted)) - resultNames := lo.Map(result, func(o metav1.Object, _ int) string { - return o.GetName() - }) - adoptedNames := lo.Map(adopted, func(o metav1.Object, _ int) string { - return o.GetName() - }) - s.EqualValues(adoptedNames, resultNames) + s.Require().NoError(err) + // check adopted objects + s.Len(result, len(adopted)) + resultNames := lo.Map(result, func(o metav1.Object, _ int) string { + return o.GetName() + }) + adoptedNames := lo.Map(adopted, func(o metav1.Object, _ int) string { + return o.GetName() + }) + s.EqualValues(adoptedNames, resultNames) + + forEarch := func(objs []metav1.Object, check func(ownerRef *metav1.OwnerReference)) { + for _, obj := range objs { + pod := &corev1.Pod{} + err := s.client.Get(context.Background(), client.ObjectKeyFromObject(obj.(client.Object)), pod) + s.Require().NoError(err) + ownerRef := metav1.GetControllerOf(pod) + check(ownerRef) + } } + + // check adopted object owners + forEarch(adopted, func(ownerRef *metav1.OwnerReference) { + s.True(ReferSameObject(*ownerRef, s.ownerRef), "object should be adopted") + }) + // check released objects + forEarch(released, func(ownerRef *metav1.OwnerReference) { + s.Require().Nil(ownerRef) + }) + // check no changed objects + forEarch(noChanged, func(ownerRef *metav1.OwnerReference) { + s.True(ReferSameObject(*ownerRef, s.deferentOwnerRef), "object should not be changed") + }) } diff --git a/controller/refmanager/writer.go b/controller/refmanager/writer.go index 0fd98654..aad3383c 100644 --- a/controller/refmanager/writer.go +++ b/controller/refmanager/writer.go @@ -53,7 +53,7 @@ func (w *ownerRefWriter) Adopt(ctx context.Context, parent metav1.Object, parent } upsertOwner(child, *newOwner) - clientObj, ok := parent.(client.Object) + clientObj, ok := child.(client.Object) if !ok { return fmt.Errorf("failed to convert parent object to controller-runtime client.Object") } diff --git a/controller/refmanager/writer_test.go b/controller/refmanager/writer_test.go index 51649e5e..a52edde3 100644 --- a/controller/refmanager/writer_test.go +++ b/controller/refmanager/writer_test.go @@ -19,6 +19,7 @@ package refmanager import ( "context" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -28,20 +29,20 @@ func (s *ownerReferenceTestSute) TestAdopt() { testcase := []struct { name string - object client.Object + pod *corev1.Pod wantErr bool }{ { - name: "adopt orpha pod", - object: newFakePod(nil), + name: "adopt orpha pod", + pod: newFakePod(nil), }, { - name: "adopt owned pod", - object: newFakePod(s.owner), + name: "adopt owned pod", + pod: newFakePod(s.owner), }, { name: "adopt pod with conflict", - object: newFakePod(s.deferentOwner), + pod: newFakePod(s.deferentOwner), wantErr: true, }, } @@ -50,15 +51,20 @@ func (s *ownerReferenceTestSute) TestAdopt() { test := testcase[i] s.Run(test.name, func() { // create object - err := s.client.Create(context.TODO(), test.object) + err := s.client.Create(context.TODO(), test.pod) s.NoError(err) - err = writer.Adopt(context.TODO(), s.owner, s.ownerGVK, test.object) + err = writer.Adopt(context.TODO(), s.owner, s.ownerGVK, test.pod) if test.wantErr { s.Error(err) } else { s.NoError(err) - owner := metav1.GetControllerOfNoCopy(test.object) + // get pod from client + pod := &corev1.Pod{} + err = s.client.Get(context.TODO(), client.ObjectKeyFromObject(test.pod), pod) + s.Require().NoError(err) + owner := metav1.GetControllerOfNoCopy(pod) + s.Require().NotNil(owner) s.True(ReferSameObject(s.ownerRef, *owner), "the controller owner reference must be the same") } }) @@ -69,20 +75,20 @@ func (s *ownerReferenceTestSute) TestRelease() { writer := NewOwnerRefWriter(s.client) testcase := []struct { - name string - object client.Object + name string + pod client.Object }{ { - name: "release orphan pod, nothing changed", - object: newFakePod(nil), + name: "release orphan pod, nothing changed", + pod: newFakePod(nil), }, { - name: "release owned pod", - object: newFakePod(s.owner), + name: "release owned pod", + pod: newFakePod(s.owner), }, { - name: "release pod controlled by other object", - object: newFakePod(s.deferentOwner), + name: "release pod controlled by other object", + pod: newFakePod(s.deferentOwner), }, } @@ -90,12 +96,16 @@ func (s *ownerReferenceTestSute) TestRelease() { test := testcase[i] s.Run(test.name, func() { // create object - err := s.client.Create(context.TODO(), test.object) + err := s.client.Create(context.TODO(), test.pod) s.NoError(err) - writer.Release(context.TODO(), s.owner, test.object) // nolint - owner := metav1.GetControllerOfNoCopy(test.object) + writer.Release(context.TODO(), s.owner, test.pod) // nolint + // get pod from client + pod := &corev1.Pod{} + err = s.client.Get(context.TODO(), client.ObjectKeyFromObject(test.pod), pod) + s.Require().NoError(err) + owner := metav1.GetControllerOfNoCopy(pod) if owner != nil { s.False(ReferSameObject(s.ownerRef, *owner), "the controller owner reference must not be the same") }