From 8c6e1b4c35f27657b7fedbebfab92861a41c32cf 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 --- .golangci.yml | 1 - .../kubeadm/internal/controllers/helpers.go | 18 ++++-- .../cluster/cluster_controller_phases.go | 57 ++++++++++++------- .../clusterclass/clusterclass_controller.go | 14 ++++- .../machine/machine_controller_phases.go | 39 +++++++++++-- .../machinedeployment_controller.go | 18 ++++-- .../machineset/machineset_controller.go | 18 ++++-- util/util.go | 15 +++++ 8 files changed, 133 insertions(+), 47 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index fa00d72c5089..79ccd077c3ce 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -27,7 +27,6 @@ linters: - errchkjson - gci - ginkgolinter - - goconst - gocritic - godot - gofmt diff --git a/controlplane/kubeadm/internal/controllers/helpers.go b/controlplane/kubeadm/internal/controllers/helpers.go index 98dcfd0d6ba8..f9efeded85f4 100644 --- a/controlplane/kubeadm/internal/controllers/helpers.go +++ b/controlplane/kubeadm/internal/controllers/helpers.go @@ -147,17 +147,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: cluster.Name, + UID: 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: cluster.Name, - UID: 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 4afa3976f5e9..e11ed3176813 100644 --- a/internal/controllers/cluster/cluster_controller_phases.go +++ b/internal/controllers/cluster/cluster_controller_phases.go @@ -24,8 +24,11 @@ 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/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" @@ -103,27 +106,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C return external.ReconcileOutput{Paused: true}, nil } - // Initialize the patch helper. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return external.ReconcileOutput{}, err - } - - // Set external object ControllerReference to the Cluster. - if err := controllerutil.SetControllerReference(cluster, obj, r.Client.Scheme()); err != nil { - return external.ReconcileOutput{}, 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 external.ReconcileOutput{}, err } @@ -146,6 +129,38 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C return external.ReconcileOutput{Result: 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, cluster *clusterv1.Cluster) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index 1955db3ad3c0..954f96ac821d 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/internal/controllers/clusterclass/clusterclass_controller.go @@ -48,6 +48,7 @@ import ( tlog "sigs.k8s.io/cluster-api/internal/log" internalruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" "sigs.k8s.io/cluster-api/internal/topology/variables" + "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/cache" "sigs.k8s.io/cluster-api/util/conditions" @@ -396,18 +397,25 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste return nil } - // 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 ControllerReference 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", tlog.KObj{Obj: 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 e922accef42a..1d6f31323840 100644 --- a/internal/controllers/machine/machine_controller_phases.go +++ b/internal/controllers/machine/machine_controller_phases.go @@ -145,7 +145,24 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste return external.ReconcileOutput{Paused: true}, nil } - // 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 external.ReconcileOutput{}, err + } + + if !hasOnCreateOwnerRefs && + util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) && + obj.GetLabels()[clusterv1.ClusterNameLabel] == m.Spec.ClusterName { + return external.ReconcileOutput{Result: obj}, nil + } + patchHelper, err := patch.NewHelper(obj, r.Client) if err != nil { return external.ReconcileOutput{}, err @@ -163,7 +180,6 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste return external.ReconcileOutput{}, err } - // Set the Cluster label. labels := obj.GetLabels() if labels == nil { labels = make(map[string]string) @@ -171,7 +187,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 external.ReconcileOutput{}, err } @@ -388,7 +403,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) { @@ -399,6 +414,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 089f42e192ef..f92d232bbb2b 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -424,17 +424,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 b4e2cc149c85..d15dc998753c 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -1171,17 +1171,23 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu 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, r.Client) 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/util/util.go b/util/util.go index 3892d81cdd55..4481deeefe52 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 {