Merge pull request #11666 from sbueringer/pr-patch-reconcile-external

🌱 Call patchHelper only if necessary when reconciling external refs

Author: k8s-ci-robot

.golangci.yml

@@ -22,7 +22,6 @@ linters:
     - errchkjson # invalid types passed to json encoder
     - gci # ensures imports are organized
     - ginkgolinter # ginkgo and gomega
-    - goconst # strings that can be replaced by constants
     - gocritic # bugs, performance, style (we could add custom ones to this one)
     - godot # checks that comments end in a period
     - gofmt # warns about incorrect use of fmt functions

controlplane/kubeadm/internal/controllers/helpers.go

@@ -161,17 +161,23 @@ func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.C
 
 	// Note: We intentionally do not handle checking for the paused label on an external template reference
 
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "Cluster",
+		Name:       controlPlane.Cluster.Name,
+		UID:        controlPlane.Cluster.UID,
+	}
+
+	if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
+		return nil
+	}
+
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return err
 	}
 
-	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
-		APIVersion: clusterv1.GroupVersion.String(),
-		Kind:       "Cluster",
-		Name:       controlPlane.Cluster.Name,
-		UID:        controlPlane.Cluster.UID,
-	}))
+	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
 
 	return patchHelper.Patch(ctx, obj)
 }

internal/controllers/cluster/cluster_controller_phases.go

@@ -24,10 +24,12 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 
@@ -101,27 +103,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 		return nil, err
 	}
 
-	// Initialize the patch helper.
-	patchHelper, err := patch.NewHelper(obj, r.Client)
-	if err != nil {
-		return nil, err
-	}
-
-	// Set external object ControllerReference to the Cluster.
-	if err := controllerutil.SetControllerReference(cluster, obj, r.Client.Scheme()); err != nil {
-		return nil, err
-	}
-
-	// Set the Cluster label.
-	labels := obj.GetLabels()
-	if labels == nil {
-		labels = make(map[string]string)
-	}
-	labels[clusterv1.ClusterNameLabel] = cluster.Name
-	obj.SetLabels(labels)
-
-	// Always attempt to Patch the external object.
-	if err := patchHelper.Patch(ctx, obj); err != nil {
+	if err := ensureOwnerRefAndLabel(ctx, r.Client, obj, cluster); err != nil {
 		return nil, err
 	}
 
@@ -144,6 +126,38 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 	return obj, nil
 }
 
+func ensureOwnerRefAndLabel(ctx context.Context, c client.Client, obj *unstructured.Unstructured, cluster *clusterv1.Cluster) error {
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "Cluster",
+		Name:       cluster.Name,
+		Controller: ptr.To(true),
+	}
+
+	if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) &&
+		obj.GetLabels()[clusterv1.ClusterNameLabel] == cluster.Name {
+		return nil
+	}
+
+	patchHelper, err := patch.NewHelper(obj, c)
+	if err != nil {
+		return err
+	}
+
+	if err := controllerutil.SetControllerReference(cluster, obj, c.Scheme()); err != nil {
+		return err
+	}
+
+	labels := obj.GetLabels()
+	if labels == nil {
+		labels = make(map[string]string)
+	}
+	labels[clusterv1.ClusterNameLabel] = cluster.Name
+	obj.SetLabels(labels)
+
+	return patchHelper.Patch(ctx, obj)
+}
+
 // reconcileInfrastructure reconciles the Spec.InfrastructureRef object on a Cluster.
 func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)

internal/controllers/clusterclass/clusterclass_controller.go

@@ -430,18 +430,25 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
 		return errors.Wrapf(err, "failed to get the external object for the ClusterClass. refGroupVersionKind: %s, refName: %s, refNamespace: %s", ref.GroupVersionKind(), ref.Name, ref.Namespace)
 	}
 
-	// Initialize the patch helper.
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "ClusterClass",
+		Name:       clusterClass.Name,
+	}
+
+	if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
+		return nil
+	}
+
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return err
 	}
 
-	// Set external object owner reference to the ClusterClass.
 	if err := controllerutil.SetOwnerReference(clusterClass, obj, r.Client.Scheme()); err != nil {
 		return errors.Wrapf(err, "failed to set ClusterClass owner reference for %s %s", obj.GetKind(), klog.KObj(obj))
 	}
 
-	// Patch the external object.
 	return patchHelper.Patch(ctx, obj)
 }
 

internal/controllers/machine/machine_controller_phases.go

@@ -94,7 +94,24 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
 		return nil, err
 	}
 
-	// Initialize the patch helper.
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "Machine",
+		Name:       m.Name,
+		Controller: ptr.To(true),
+	}
+
+	hasOnCreateOwnerRefs, err := hasOnCreateOwnerRefs(cluster, m, obj)
+	if err != nil {
+		return nil, err
+	}
+
+	if !hasOnCreateOwnerRefs &&
+		util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) &&
+		obj.GetLabels()[clusterv1.ClusterNameLabel] == m.Spec.ClusterName {
+		return obj, nil
+	}
+
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return nil, err
@@ -112,15 +129,13 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
 		return nil, err
 	}
 
-	// Set the Cluster label.
 	labels := obj.GetLabels()
 	if labels == nil {
 		labels = make(map[string]string)
 	}
 	labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
 	obj.SetLabels(labels)
 
-	// Always attempt to Patch the external object.
 	if err := patchHelper.Patch(ctx, obj); err != nil {
 		return nil, err
 	}
@@ -351,7 +366,7 @@ func removeOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, o
 	for _, owner := range obj.GetOwnerReferences() {
 		ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
 		if err != nil {
-			return errors.Wrapf(err, "Could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
+			return errors.Wrapf(err, "could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
 		}
 		if (ownerGV.Group == clusterv1.GroupVersion.Group && owner.Kind == "MachineSet") ||
 			(cpGVK != nil && ownerGV.Group == cpGVK.GroupVersion().Group && owner.Kind == cpGVK.Kind) {
@@ -362,6 +377,22 @@ func removeOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, o
 	return nil
 }
 
+// hasOnCreateOwnerRefs will check if any MachineSet or control plane owner references from passed objects are set.
+func hasOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, obj *unstructured.Unstructured) (bool, error) {
+	cpGVK := getControlPlaneGVKForMachine(cluster, m)
+	for _, owner := range obj.GetOwnerReferences() {
+		ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
+		if err != nil {
+			return false, errors.Wrapf(err, "could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
+		}
+		if (ownerGV.Group == clusterv1.GroupVersion.Group && owner.Kind == "MachineSet") ||
+			(cpGVK != nil && ownerGV.Group == cpGVK.GroupVersion().Group && owner.Kind == cpGVK.Kind) {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 // getControlPlaneGVKForMachine returns the Kind of the control plane in the Cluster associated with the Machine.
 // This function checks that the Machine is managed by a control plane, and then retrieves the Kind from the Cluster's
 // .spec.controlPlaneRef.

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -527,17 +527,23 @@ func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cl
 		return err
 	}
 
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "Cluster",
+		Name:       cluster.Name,
+		UID:        cluster.UID,
+	}
+
+	if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
+		return nil
+	}
+
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
 		return err
 	}
 
-	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
-		APIVersion: clusterv1.GroupVersion.String(),
-		Kind:       "Cluster",
-		Name:       cluster.Name,
-		UID:        cluster.UID,
-	}))
+	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
 
 	return patchHelper.Patch(ctx, obj)
 }

internal/controllers/machineset/machineset_controller.go

@@ -1571,17 +1571,23 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu
 		return false, err
 	}
 
+	desiredOwnerRef := metav1.OwnerReference{
+		APIVersion: clusterv1.GroupVersion.String(),
+		Kind:       "Cluster",
+		Name:       cluster.Name,
+		UID:        cluster.UID,
+	}
+
+	if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
+		return false, nil
+	}
+
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return false, err
 	}
 
-	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
-		APIVersion: clusterv1.GroupVersion.String(),
-		Kind:       "Cluster",
-		Name:       cluster.Name,
-		UID:        cluster.UID,
-	}))
+	obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
 
 	return false, patchHelper.Patch(ctx, obj)
 }

util/util.go

@@ -39,6 +39,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	k8sversion "k8s.io/apimachinery/pkg/version"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -334,6 +335,20 @@ func RemoveOwnerRef(ownerReferences []metav1.OwnerReference, inputRef metav1.Own
 	return ownerReferences
 }
 
+// HasExactOwnerRef returns true if the exact OwnerReference is already in the slice.
+// It matches based on APIVersion, Kind, Name and Controller.
+func HasExactOwnerRef(ownerReferences []metav1.OwnerReference, ref metav1.OwnerReference) bool {
+	for _, r := range ownerReferences {
+		if r.APIVersion == ref.APIVersion &&
+			r.Kind == ref.Kind &&
+			r.Name == ref.Name &&
+			ptr.Deref(r.Controller, false) == ptr.Deref(ref.Controller, false) {
+			return true
+		}
+	}
+	return false
+}
+
 // indexOwnerRef returns the index of the owner reference in the slice if found, or -1.
 func indexOwnerRef(ownerReferences []metav1.OwnerReference, ref metav1.OwnerReference) int {
 	for index, r := range ownerReferences {