Skip to content

Commit d642ad2

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #11666 from sbueringer/pr-patch-reconcile-external
🌱 Call patchHelper only if necessary when reconciling external refs
2 parents 223fc8f + 196833d commit d642ad2

File tree

8 files changed

+131
-47
lines changed

8 files changed

+131
-47
lines changed

‎.golangci.yml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ linters:
2222
- errchkjson # invalid types passed to json encoder
2323
- gci # ensures imports are organized
2424
- ginkgolinter # ginkgo and gomega
25-
- goconst # strings that can be replaced by constants
2625
- gocritic # bugs, performance, style (we could add custom ones to this one)
2726
- godot # checks that comments end in a period
2827
- gofmt # warns about incorrect use of fmt functions

‎controlplane/kubeadm/internal/controllers/helpers.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,17 +161,23 @@ func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.C
161161

162162
// Note: We intentionally do not handle checking for the paused label on an external template reference
163163

164+
desiredOwnerRef := metav1.OwnerReference{
165+
APIVersion: clusterv1.GroupVersion.String(),
166+
Kind: "Cluster",
167+
Name: controlPlane.Cluster.Name,
168+
UID: controlPlane.Cluster.UID,
169+
}
170+
171+
if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
172+
return nil
173+
}
174+
164175
patchHelper, err := patch.NewHelper(obj, r.Client)
165176
if err != nil {
166177
return err
167178
}
168179

169-
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
170-
APIVersion: clusterv1.GroupVersion.String(),
171-
Kind: "Cluster",
172-
Name: controlPlane.Cluster.Name,
173-
UID: controlPlane.Cluster.UID,
174-
}))
180+
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
175181

176182
return patchHelper.Patch(ctx, obj)
177183
}

‎internal/controllers/cluster/cluster_controller_phases.go

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,12 @@ import (
2424
"github.com/pkg/errors"
2525
corev1 "k8s.io/api/core/v1"
2626
apierrors "k8s.io/apimachinery/pkg/api/errors"
27+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2728
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2829
"k8s.io/klog/v2"
2930
"k8s.io/utils/ptr"
3031
ctrl "sigs.k8s.io/controller-runtime"
32+
"sigs.k8s.io/controller-runtime/pkg/client"
3133
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3234
"sigs.k8s.io/controller-runtime/pkg/handler"
3335

@@ -101,27 +103,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
101103
return nil, err
102104
}
103105

104-
// Initialize the patch helper.
105-
patchHelper, err := patch.NewHelper(obj, r.Client)
106-
if err != nil {
107-
return nil, err
108-
}
109-
110-
// Set external object ControllerReference to the Cluster.
111-
if err := controllerutil.SetControllerReference(cluster, obj, r.Client.Scheme()); err != nil {
112-
return nil, err
113-
}
114-
115-
// Set the Cluster label.
116-
labels := obj.GetLabels()
117-
if labels == nil {
118-
labels = make(map[string]string)
119-
}
120-
labels[clusterv1.ClusterNameLabel] = cluster.Name
121-
obj.SetLabels(labels)
122-
123-
// Always attempt to Patch the external object.
124-
if err := patchHelper.Patch(ctx, obj); err != nil {
106+
if err := ensureOwnerRefAndLabel(ctx, r.Client, obj, cluster); err != nil {
125107
return nil, err
126108
}
127109

@@ -144,6 +126,38 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
144126
return obj, nil
145127
}
146128

129+
func ensureOwnerRefAndLabel(ctx context.Context, c client.Client, obj *unstructured.Unstructured, cluster *clusterv1.Cluster) error {
130+
desiredOwnerRef := metav1.OwnerReference{
131+
APIVersion: clusterv1.GroupVersion.String(),
132+
Kind: "Cluster",
133+
Name: cluster.Name,
134+
Controller: ptr.To(true),
135+
}
136+
137+
if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) &&
138+
obj.GetLabels()[clusterv1.ClusterNameLabel] == cluster.Name {
139+
return nil
140+
}
141+
142+
patchHelper, err := patch.NewHelper(obj, c)
143+
if err != nil {
144+
return err
145+
}
146+
147+
if err := controllerutil.SetControllerReference(cluster, obj, c.Scheme()); err != nil {
148+
return err
149+
}
150+
151+
labels := obj.GetLabels()
152+
if labels == nil {
153+
labels = make(map[string]string)
154+
}
155+
labels[clusterv1.ClusterNameLabel] = cluster.Name
156+
obj.SetLabels(labels)
157+
158+
return patchHelper.Patch(ctx, obj)
159+
}
160+
147161
// reconcileInfrastructure reconciles the Spec.InfrastructureRef object on a Cluster.
148162
func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctrl.Result, error) {
149163
log := ctrl.LoggerFrom(ctx)

‎internal/controllers/clusterclass/clusterclass_controller.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -430,18 +430,25 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
430430
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)
431431
}
432432

433-
// Initialize the patch helper.
433+
desiredOwnerRef := metav1.OwnerReference{
434+
APIVersion: clusterv1.GroupVersion.String(),
435+
Kind: "ClusterClass",
436+
Name: clusterClass.Name,
437+
}
438+
439+
if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
440+
return nil
441+
}
442+
434443
patchHelper, err := patch.NewHelper(obj, r.Client)
435444
if err != nil {
436445
return err
437446
}
438447

439-
// Set external object owner reference to the ClusterClass.
440448
if err := controllerutil.SetOwnerReference(clusterClass, obj, r.Client.Scheme()); err != nil {
441449
return errors.Wrapf(err, "failed to set ClusterClass owner reference for %s %s", obj.GetKind(), klog.KObj(obj))
442450
}
443451

444-
// Patch the external object.
445452
return patchHelper.Patch(ctx, obj)
446453
}
447454

‎internal/controllers/machine/machine_controller_phases.go

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,24 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
9494
return nil, err
9595
}
9696

97-
// Initialize the patch helper.
97+
desiredOwnerRef := metav1.OwnerReference{
98+
APIVersion: clusterv1.GroupVersion.String(),
99+
Kind: "Machine",
100+
Name: m.Name,
101+
Controller: ptr.To(true),
102+
}
103+
104+
hasOnCreateOwnerRefs, err := hasOnCreateOwnerRefs(cluster, m, obj)
105+
if err != nil {
106+
return nil, err
107+
}
108+
109+
if !hasOnCreateOwnerRefs &&
110+
util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) &&
111+
obj.GetLabels()[clusterv1.ClusterNameLabel] == m.Spec.ClusterName {
112+
return obj, nil
113+
}
114+
98115
patchHelper, err := patch.NewHelper(obj, r.Client)
99116
if err != nil {
100117
return nil, err
@@ -112,15 +129,13 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
112129
return nil, err
113130
}
114131

115-
// Set the Cluster label.
116132
labels := obj.GetLabels()
117133
if labels == nil {
118134
labels = make(map[string]string)
119135
}
120136
labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
121137
obj.SetLabels(labels)
122138

123-
// Always attempt to Patch the external object.
124139
if err := patchHelper.Patch(ctx, obj); err != nil {
125140
return nil, err
126141
}
@@ -351,7 +366,7 @@ func removeOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, o
351366
for _, owner := range obj.GetOwnerReferences() {
352367
ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
353368
if err != nil {
354-
return errors.Wrapf(err, "Could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
369+
return errors.Wrapf(err, "could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
355370
}
356371
if (ownerGV.Group == clusterv1.GroupVersion.Group && owner.Kind == "MachineSet") ||
357372
(cpGVK != nil && ownerGV.Group == cpGVK.GroupVersion().Group && owner.Kind == cpGVK.Kind) {
@@ -362,6 +377,22 @@ func removeOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, o
362377
return nil
363378
}
364379

380+
// hasOnCreateOwnerRefs will check if any MachineSet or control plane owner references from passed objects are set.
381+
func hasOnCreateOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, obj *unstructured.Unstructured) (bool, error) {
382+
cpGVK := getControlPlaneGVKForMachine(cluster, m)
383+
for _, owner := range obj.GetOwnerReferences() {
384+
ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
385+
if err != nil {
386+
return false, errors.Wrapf(err, "could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
387+
}
388+
if (ownerGV.Group == clusterv1.GroupVersion.Group && owner.Kind == "MachineSet") ||
389+
(cpGVK != nil && ownerGV.Group == cpGVK.GroupVersion().Group && owner.Kind == cpGVK.Kind) {
390+
return true, nil
391+
}
392+
}
393+
return false, nil
394+
}
395+
365396
// getControlPlaneGVKForMachine returns the Kind of the control plane in the Cluster associated with the Machine.
366397
// This function checks that the Machine is managed by a control plane, and then retrieves the Kind from the Cluster's
367398
// .spec.controlPlaneRef.

‎internal/controllers/machinedeployment/machinedeployment_controller.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -527,17 +527,23 @@ func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cl
527527
return err
528528
}
529529

530+
desiredOwnerRef := metav1.OwnerReference{
531+
APIVersion: clusterv1.GroupVersion.String(),
532+
Kind: "Cluster",
533+
Name: cluster.Name,
534+
UID: cluster.UID,
535+
}
536+
537+
if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
538+
return nil
539+
}
540+
530541
patchHelper, err := patch.NewHelper(obj, c)
531542
if err != nil {
532543
return err
533544
}
534545

535-
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
536-
APIVersion: clusterv1.GroupVersion.String(),
537-
Kind: "Cluster",
538-
Name: cluster.Name,
539-
UID: cluster.UID,
540-
}))
546+
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
541547

542548
return patchHelper.Patch(ctx, obj)
543549
}

‎internal/controllers/machineset/machineset_controller.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1571,17 +1571,23 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu
15711571
return false, err
15721572
}
15731573

1574+
desiredOwnerRef := metav1.OwnerReference{
1575+
APIVersion: clusterv1.GroupVersion.String(),
1576+
Kind: "Cluster",
1577+
Name: cluster.Name,
1578+
UID: cluster.UID,
1579+
}
1580+
1581+
if util.HasExactOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef) {
1582+
return false, nil
1583+
}
1584+
15741585
patchHelper, err := patch.NewHelper(obj, r.Client)
15751586
if err != nil {
15761587
return false, err
15771588
}
15781589

1579-
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), metav1.OwnerReference{
1580-
APIVersion: clusterv1.GroupVersion.String(),
1581-
Kind: "Cluster",
1582-
Name: cluster.Name,
1583-
UID: cluster.UID,
1584-
}))
1590+
obj.SetOwnerReferences(util.EnsureOwnerRef(obj.GetOwnerReferences(), desiredOwnerRef))
15851591

15861592
return false, patchHelper.Patch(ctx, obj)
15871593
}

‎util/util.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"k8s.io/apimachinery/pkg/runtime/schema"
4040
"k8s.io/apimachinery/pkg/types"
4141
k8sversion "k8s.io/apimachinery/pkg/version"
42+
"k8s.io/utils/ptr"
4243
ctrl "sigs.k8s.io/controller-runtime"
4344
"sigs.k8s.io/controller-runtime/pkg/client"
4445
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -334,6 +335,20 @@ func RemoveOwnerRef(ownerReferences []metav1.OwnerReference, inputRef metav1.Own
334335
return ownerReferences
335336
}
336337

338+
// HasExactOwnerRef returns true if the exact OwnerReference is already in the slice.
339+
// It matches based on APIVersion, Kind, Name and Controller.
340+
func HasExactOwnerRef(ownerReferences []metav1.OwnerReference, ref metav1.OwnerReference) bool {
341+
for _, r := range ownerReferences {
342+
if r.APIVersion == ref.APIVersion &&
343+
r.Kind == ref.Kind &&
344+
r.Name == ref.Name &&
345+
ptr.Deref(r.Controller, false) == ptr.Deref(ref.Controller, false) {
346+
return true
347+
}
348+
}
349+
return false
350+
}
351+
337352
// indexOwnerRef returns the index of the owner reference in the slice if found, or -1.
338353
func indexOwnerRef(ownerReferences []metav1.OwnerReference, ref metav1.OwnerReference) int {
339354
for index, r := range ownerReferences {

0 commit comments

Comments
 (0)