From 854684529ed526e4acc205086b9994eda84ce88a Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Fri, 10 Jan 2025 13:52:10 +0100 Subject: [PATCH] patch: Call patchHelper only if necessary when reconciling external refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../kubeadm/internal/controllers/helpers.go | 18 ++++-- .../cluster/cluster_controller_phases.go | 56 ++++++++++++------- .../clusterclass/clusterclass_controller.go | 13 ++++- .../machine/machine_controller_phases.go | 39 +++++++++++-- .../machinedeployment_controller.go | 18 ++++-- .../machineset/machineset_controller.go | 18 ++++-- util/util.go | 15 +++++ 7 files changed, 131 insertions(+), 46 deletions(-) diff --git a/controlplane/kubeadm/internal/controllers/helpers.go b/controlplane/kubeadm/internal/controllers/helpers.go index 646d1c32a80b..82f68eb225b9 100644 --- a/controlplane/kubeadm/internal/controllers/helpers.go +++ b/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) } diff --git a/internal/controllers/cluster/cluster_controller_phases.go b/internal/controllers/cluster/cluster_controller_phases.go index 525e9621b340..0b0c77df517d 100644 --- a/internal/controllers/cluster/cluster_controller_phases.go +++ b/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) diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index a577066e6883..7f8f01d12a51 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/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) } diff --git a/internal/controllers/machine/machine_controller_phases.go b/internal/controllers/machine/machine_controller_phases.go index c9f71f7769b0..b108e99bbb34 100644 --- a/internal/controllers/machine/machine_controller_phases.go +++ b/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,7 +129,6 @@ 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) @@ -120,7 +136,6 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste 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. diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index ec92f246761a..4b714b3d6ba9 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/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) } diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 4ee0f88af938..aeb5ac4341ca 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/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) } diff --git a/util/util.go b/util/util.go index b5a5b6b946b3..cc1a16030b7f 100644 --- a/util/util.go +++ b/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 {