Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Use HasOwnerReference in controller-runtime #11700

Closed
wants to merge 3 commits into from
+45 −59
Closed

🌱 Use HasOwnerReference in controller-runtime #11700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7f3da8
Use HasOwnerReference in controller-runtime
sivchari Jan 18, 2025
61c4110
fix lint
sivchari Jan 18, 2025
58ea31e
fix variable name
sivchari Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions exp/topology/desiredstate/desired_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/utils/ptr"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
Expand Down Expand Up @@ -169,7 +170,10 @@ func TestComputeInfrastructureCluster(t *testing.T) {
obj, err := computeInfrastructureCluster(ctx, scope)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(obj).ToNot(BeNil())
g.Expect(ownerrefs.HasOwnerReferenceFrom(obj, shim)).To(BeTrue())

b, err := ctrlutil.HasOwnerReference(obj.GetOwnerReferences(), shim, fakeScheme)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(b).To(BeTrue())
})
}

Expand Down Expand Up @@ -663,7 +667,9 @@ func TestComputeControlPlane(t *testing.T) {
obj, err := (&generator{}).computeControlPlane(ctx, s, nil)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(obj).ToNot(BeNil())
g.Expect(ownerrefs.HasOwnerReferenceFrom(obj, shim)).To(BeTrue())
b, err := ctrlutil.HasOwnerReference(obj.GetOwnerReferences(), shim, fakeScheme)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(b).To(BeTrue())
})
}

Expand Down
50 changes: 11 additions & 39 deletions internal/controllers/clusterclass/clusterclass_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package clusterclass

import (
"context"
"fmt"
"testing"
"time"

Expand All @@ -33,10 +32,10 @@ import (
"k8s.io/component-base/featuregate"
utilfeature "k8s.io/component-base/featuregate/testing"
utilversion "k8s.io/component-base/version"
"k8s.io/klog/v2"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand Down Expand Up @@ -225,7 +224,7 @@ func assertInfrastructureClusterTemplate(ctx context.Context, actualClusterClass
if err := env.Get(ctx, actualInfraClusterTemplateKey, actualInfraClusterTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualInfraClusterTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualInfraClusterTemplate, actualClusterClass); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new func seems to be checking a diiferent list of fields (e.g the old one test for uid)

return err
}

Expand All @@ -245,7 +244,7 @@ func assertControlPlaneTemplate(ctx context.Context, actualClusterClass *cluster
if err := env.Get(ctx, actualControlPlaneTemplateKey, actualControlPlaneTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualControlPlaneTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualControlPlaneTemplate, actualClusterClass); err != nil {
return err
}

Expand All @@ -266,7 +265,7 @@ func assertControlPlaneTemplate(ctx context.Context, actualClusterClass *cluster
if err := env.Get(ctx, actualInfrastructureMachineTemplateKey, actualInfrastructureMachineTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, actualClusterClass); err != nil {
return err
}

Expand Down Expand Up @@ -300,7 +299,7 @@ func assertMachineDeploymentClass(ctx context.Context, actualClusterClass *clust
if err := env.Get(ctx, actualInfrastructureMachineTemplateKey, actualInfrastructureMachineTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, actualClusterClass); err != nil {
return err
}

Expand All @@ -320,7 +319,7 @@ func assertMachineDeploymentClass(ctx context.Context, actualClusterClass *clust
if err := env.Get(ctx, actualBootstrapTemplateKey, actualBootstrapTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualBootstrapTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualBootstrapTemplate, actualClusterClass); err != nil {
return err
}

Expand Down Expand Up @@ -349,7 +348,7 @@ func assertMachinePoolClass(ctx context.Context, actualClusterClass *clusterv1.C
if err := env.Get(ctx, actualInfrastructureMachinePoolTemplateKey, actualInfrastructureMachinePoolTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualInfrastructureMachinePoolTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualInfrastructureMachinePoolTemplate, actualClusterClass); err != nil {
return err
}

Expand All @@ -369,7 +368,7 @@ func assertMachinePoolClass(ctx context.Context, actualClusterClass *clusterv1.C
if err := env.Get(ctx, actualBootstrapTemplateKey, actualBootstrapTemplate); err != nil {
return err
}
if err := assertHasOwnerReference(actualBootstrapTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil {
if err := assertHasOwnerReference(actualBootstrapTemplate, actualClusterClass); err != nil {
return err
}

Expand All @@ -379,36 +378,9 @@ func assertMachinePoolClass(ctx context.Context, actualClusterClass *clusterv1.C
builder.BootstrapGroupVersion)
}

func assertHasOwnerReference(obj client.Object, ownerRef metav1.OwnerReference) error {
found := false
for _, ref := range obj.GetOwnerReferences() {
if isOwnerReferenceEqual(ref, ownerRef) {
found = true
break
}
}
if !found {
// Note: Kind might be empty here as it's usually only set on Unstructured objects.
// But as this is just test code we don't care too much about it.
return fmt.Errorf("%s %s does not have OwnerReference %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj), &ownerRef)
}
return nil
}

func isOwnerReferenceEqual(a, b metav1.OwnerReference) bool {
if a.APIVersion != b.APIVersion {
return false
}
if a.Kind != b.Kind {
return false
}
if a.Name != b.Name {
return false
}
if a.UID != b.UID {
return false
}
return true
func assertHasOwnerReference(owner, obj client.Object) error {
_, err := ctrlutil.HasOwnerReference(owner.GetOwnerReferences(), obj, fakeScheme)
return err
}

func TestReconciler_reconcileVariables(t *testing.T) {
Expand Down
13 changes: 0 additions & 13 deletions internal/controllers/clusterclass/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
Expand Down Expand Up @@ -81,18 +80,6 @@ func TestMain(m *testing.M) {
}))
}

// ownerReferenceTo converts an object to an OwnerReference.
// Note: We pass in gvk explicitly as we can't rely on GVK being set on all objects
// (only on Unstructured).
func ownerReferenceTo(obj client.Object, gvk schema.GroupVersionKind) *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Kind: gvk.Kind,
Name: obj.GetName(),
UID: obj.GetUID(),
}
}

// referenceExistsWithCorrectKindAndAPIVersion asserts that the passed ObjectReference is not nil and that it has the correct kind and apiVersion.
func referenceExistsWithCorrectKindAndAPIVersion(reference *corev1.ObjectReference, kind string, apiVersion schema.GroupVersion) error {
if reference == nil {
Expand Down
27 changes: 23 additions & 4 deletions internal/controllers/topology/cluster/reconcile_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
Expand Down Expand Up @@ -159,10 +160,28 @@ func (r *Reconciler) reconcileClusterShim(ctx context.Context, s *scope.Scope) e
// When the Cluster and the shim object are both owners,
// it's safe for us to remove the shim and garbage collect any potential orphaned resource.
if s.Current.InfrastructureCluster != nil && s.Current.ControlPlane.Object != nil {
clusterOwnsAll := ownerrefs.HasOwnerReferenceFrom(s.Current.InfrastructureCluster, s.Current.Cluster) &&
ownerrefs.HasOwnerReferenceFrom(s.Current.ControlPlane.Object, s.Current.Cluster)
shimOwnsAtLeastOne := ownerrefs.HasOwnerReferenceFrom(s.Current.InfrastructureCluster, shim) ||
ownerrefs.HasOwnerReferenceFrom(s.Current.ControlPlane.Object, shim)
infraClusterOwnsFound, err := ctrlutil.HasOwnerReference(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem equivalent
(previous code is testing only kind and name, new one is testing also group)

Copy link
Member

@sbueringer sbueringer Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly. This is not exactly the most trivial code here. I would only touch it if there's really a benefit to it. Just getting rid of a few lines in Cluster API doesn't meet that bar for me.

Also the old code looks simpler to me. A lot less error handling

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. surely it seems these changes to give the no beneffit. I close it, then I keep in mind to open the meanigful CL to cluster api.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. Don't worry about it 😃

s.Current.InfrastructureCluster.GetOwnerReferences(),
s.Current.Cluster,
r.Client.Scheme())
if err != nil {
return errors.Wrapf(err, "failed to check if the cluster owns the infrastructure cluster object")
}
controlPlaneClusterOwnsFound, err := ctrlutil.HasOwnerReference(s.Current.ControlPlane.Object.GetOwnerReferences(), s.Current.Cluster, r.Client.Scheme())
if err != nil {
return errors.Wrapf(err, "failed to check if the control plane owns the control plane object")
}
clusterOwnsAll := infraClusterOwnsFound && controlPlaneClusterOwnsFound

infraShimOwnsFound, err := ctrlutil.HasOwnerReference(s.Current.InfrastructureCluster.GetOwnerReferences(), shim, r.Client.Scheme())
if err != nil {
return errors.Wrapf(err, "failed to check if the cluster shim owns the infrastructure cluster object")
}
controlPlaneShimOwnsFound, err := ctrlutil.HasOwnerReference(s.Current.ControlPlane.Object.GetOwnerReferences(), shim, r.Client.Scheme())
if err != nil {
return errors.Wrapf(err, "failed to check if the cluster shim owns the control plane object")
}
shimOwnsAtLeastOne := infraShimOwnsFound || controlPlaneShimOwnsFound

if clusterOwnsAll && shimOwnsAtLeastOne {
if err := r.Client.Delete(ctx, shim); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/topology/cluster/reconcile_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ func TestReconcileShim(t *testing.T) {

// Run reconcileClusterShim using a nil client, so an error will be triggered if any operation is attempted
r := Reconciler{
Client: nil,
Client: env,
APIReader: env.GetAPIReader(),
patchHelperFactory: serverSideApplyPatchHelperFactory(nil, ssa.NewCache()),
}
Expand Down
2 changes: 2 additions & 0 deletions internal/topology/ownerrefs/ownerref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ func OwnerReferenceTo(obj client.Object, gvk schema.GroupVersionKind) *metav1.Ow
}

// HasOwnerReferenceFrom checks if obj has an ownerRef pointing to owner.
//
// Deprecated: This function is deprecated and will be removed in an upcoming release of Cluster API.
func HasOwnerReferenceFrom(obj, owner client.Object) bool {
for _, o := range obj.GetOwnerReferences() {
if o.Kind == owner.GetObjectKind().GroupVersionKind().Kind && o.Name == owner.GetName() {
Expand Down
Loading