Skip to content

Commit d25a869

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #12994 from sbueringer/pr-improve-hook-utils
🌱 Improve mark hook utils
2 parents 93adf87 + 1c2aeaa commit d25a869

File tree

8 files changed

+137
-61
lines changed

8 files changed

+137
-61
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ func (r *KubeadmControlPlaneReconciler) triggerInPlaceUpdate(ctx context.Context
129129
// Note: Intentionally using client.Patch (via hooks.MarkAsPending + patchHelper) instead of SSA. Otherwise we would
130130
// have to ensure we preserve PendingHooksAnnotation on existing Machines in KCP and that would lead to race
131131
// conditions when the Machine controller tries to remove the annotation and KCP adds it back.
132-
if err := hooks.MarkAsPending(ctx, r.Client, desiredMachine, runtimehooksv1.UpdateMachine); err != nil {
132+
// Note: This call will update the resourceVersion on desiredMachine, so that WaitForCacheToBeUpToDate also considers this change.
133+
if err := hooks.MarkAsPending(ctx, r.Client, desiredMachine, true, runtimehooksv1.UpdateMachine); err != nil {
133134
return errors.Wrapf(err, "failed to complete triggering in-place update for Machine %s", klog.KObj(machine))
134135
}
135136

‎exp/topology/desiredstate/desired_state.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -649,7 +649,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco
649649
// After BeforeClusterUpgrade unblocked the upgrade, consider the upgrade started.
650650
// As a consequence, the system start tracking the intent of calling AfterClusterUpgrade once the upgrade is complete.
651651
// Note: this also prevent the BeforeClusterUpgrade to be called again (until after the upgrade is completed).
652-
if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil {
652+
if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade); err != nil {
653653
return "", err
654654
}
655655
}
@@ -685,7 +685,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco
685685
if machineDeploymentPendingUpgrade || machinePoolPendingUpgrade {
686686
hooksToBeCalled = append(hooksToBeCalled, runtimehooksv1.BeforeWorkersUpgrade, runtimehooksv1.AfterWorkersUpgrade)
687687
}
688-
if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, hooksToBeCalled...); err != nil {
688+
if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, false, hooksToBeCalled...); err != nil {
689689
return "", err
690690
}
691691
}

‎exp/topology/desiredstate/lifecycle_hooks.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco
187187
return false, err
188188
}
189189
if len(extensionHandlers) == 0 {
190-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
190+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
191191
return false, err
192192
}
193193
return true, nil
@@ -220,7 +220,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco
220220
)
221221
return false, nil
222222
}
223-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
223+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
224224
return false, err
225225
}
226226

@@ -248,7 +248,7 @@ func (g *generator) callBeforeWorkersUpgradeHook(ctx context.Context, s *scope.S
248248
return false, err
249249
}
250250
if len(extensionHandlers) == 0 {
251-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.BeforeWorkersUpgrade); err != nil {
251+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.BeforeWorkersUpgrade); err != nil {
252252
return false, err
253253
}
254254
return true, nil
@@ -282,7 +282,7 @@ func (g *generator) callBeforeWorkersUpgradeHook(ctx context.Context, s *scope.S
282282
)
283283
return false, nil
284284
}
285-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.BeforeWorkersUpgrade); err != nil {
285+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.BeforeWorkersUpgrade); err != nil {
286286
return false, err
287287
}
288288

@@ -311,7 +311,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc
311311
return false, err
312312
}
313313
if len(extensionHandlers) == 0 {
314-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterWorkersUpgrade); err != nil {
314+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterWorkersUpgrade); err != nil {
315315
return false, err
316316
}
317317
return true, nil
@@ -344,7 +344,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc
344344
)
345345
return false, nil
346346
}
347-
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterWorkersUpgrade); err != nil {
347+
if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterWorkersUpgrade); err != nil {
348348
return false, err
349349
}
350350

‎internal/controllers/machine/machine_controller_inplace_update.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,9 @@ func (r *Reconciler) completeInPlaceUpdate(ctx context.Context, s *scope) error
201201
}
202202
}
203203

204-
if err := hooks.MarkAsDone(ctx, r.Client, s.machine, runtimehooksv1.UpdateMachine); err != nil {
204+
// Note: This call will not update the resourceVersion on machine, so that the patchHelper in the main
205+
// Reconcile func won't get a conflict.
206+
if err := hooks.MarkAsDone(ctx, r.Client, s.machine, false, runtimehooksv1.UpdateMachine); err != nil {
205207
return err
206208
}
207209

‎internal/controllers/topology/cluster/cluster_controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -549,7 +549,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
549549
return ctrl.Result{}, err
550550
}
551551
if len(extensionHandlers) == 0 {
552-
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
552+
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster, false); err != nil {
553553
return ctrl.Result{}, err
554554
}
555555
return ctrl.Result{}, nil
@@ -577,7 +577,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
577577
}
578578
// The BeforeClusterDelete hook returned a non-blocking response. Now the cluster is ready to be deleted.
579579
// Lets mark the cluster as `ok-to-delete`
580-
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
580+
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster, false); err != nil {
581581
return ctrl.Result{}, err
582582
}
583583
log.Info(fmt.Sprintf("Cluster deletion is unblocked by %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete)))

‎internal/controllers/topology/cluster/reconcile_state.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func (r *Reconciler) callAfterHooks(ctx context.Context, s *scope.Scope) error {
187187
func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *scope.Scope) error {
188188
// If the cluster topology is being created then track to intent to call the AfterControlPlaneInitialized hook so that we can call it later.
189189
if !s.Current.Cluster.Spec.InfrastructureRef.IsDefined() && !s.Current.Cluster.Spec.ControlPlaneRef.IsDefined() {
190-
if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
190+
if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
191191
return err
192192
}
193193
}
@@ -211,7 +211,7 @@ func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *sc
211211
return err
212212
}
213213
s.HookResponseTracker.Add(runtimehooksv1.AfterControlPlaneInitialized, hookResponse)
214-
if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
214+
if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
215215
return err
216216
}
217217
}
@@ -259,7 +259,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
259259
return err
260260
}
261261
if len(extensionHandlers) == 0 {
262-
return hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade)
262+
return hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade)
263263
}
264264

265265
// DeepCopy cluster because ConvertFrom has side effects like adding the conversion annotation.
@@ -285,7 +285,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
285285
}
286286

287287
// The hook is successfully called; we can remove this hook from the list of pending-hooks.
288-
if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil {
288+
if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade); err != nil {
289289
return err
290290
}
291291

‎internal/hooks/tracking.go‎

Lines changed: 47 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -29,24 +29,28 @@ import (
2929

3030
runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
3131
runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
32-
"sigs.k8s.io/cluster-api/util/patch"
3332
)
3433

3534
// MarkAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes.
3635
// Usually this function is called when an operation is starting in order to track the intent to call an After<operation> hook later in the process.
37-
func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
36+
func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool, hooks ...runtimecatalog.Hook) error {
3837
hookNames := []string{}
3938
for _, hook := range hooks {
4039
hookNames = append(hookNames, runtimecatalog.HookName(hook))
4140
}
4241

43-
patchHelper, err := patch.NewHelper(obj, c)
44-
if err != nil {
45-
return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ","))
42+
orig := obj.DeepCopyObject().(client.Object)
43+
44+
if changed := MarkObjectAsPending(obj, hooks...); !changed {
45+
return nil
4646
}
4747

48-
MarkObjectAsPending(obj, hooks...)
49-
if err := patchHelper.Patch(ctx, obj); err != nil {
48+
// In some cases it is preferred to not update resourceVersion in the input object,
49+
// because this could lead to conflict errors e.g. when patching at the end of a reconcile loop.
50+
if !updateResourceVersionOnObject {
51+
obj = obj.DeepCopyObject().(client.Object)
52+
}
53+
if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil {
5054
return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ","))
5155
}
5256

@@ -55,7 +59,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
5559

5660
// MarkObjectAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes.
5761
// Usually this function is called when an operation is starting in order to track the intent to call an After<operation> hook later in the process.
58-
func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) {
62+
func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) (changed bool) {
5963
hookNames := []string{}
6064
for _, hook := range hooks {
6165
hookNames = append(hookNames, runtimecatalog.HookName(hook))
@@ -66,8 +70,16 @@ func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) {
6670
if annotations == nil {
6771
annotations = map[string]string{}
6872
}
69-
annotations[runtimev1.PendingHooksAnnotation] = addToCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...)
73+
74+
newAnnotationValue := addToCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...)
75+
76+
if annotations[runtimev1.PendingHooksAnnotation] == newAnnotationValue {
77+
return false
78+
}
79+
80+
annotations[runtimev1.PendingHooksAnnotation] = newAnnotationValue
7081
obj.SetAnnotations(annotations)
82+
return true
7183
}
7284

7385
// IsPending returns true if there is an intent to call a hook being tracked in the object's PendingHooksAnnotation.
@@ -83,30 +95,33 @@ func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
8395
// MarkAsDone removes the intent to call a Hook from the object's PendingHooksAnnotation.
8496
// Usually this func is called after all the registered extensions for the Hook returned an answer without requests
8597
// to hold on to the object's lifecycle (retryAfterSeconds).
86-
func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
87-
hookNames := []string{}
88-
for _, hook := range hooks {
89-
hookNames = append(hookNames, runtimecatalog.HookName(hook))
98+
func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool, hook runtimecatalog.Hook) error {
99+
if !IsPending(hook, obj) {
100+
return nil
90101
}
91102

92-
patchHelper, err := patch.NewHelper(obj, c)
93-
if err != nil {
94-
return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ","))
95-
}
103+
hookName := runtimecatalog.HookName(hook)
104+
105+
orig := obj.DeepCopyObject().(client.Object)
96106

97107
// Read the annotation of the objects and add the hook to the comma separated list
98108
annotations := obj.GetAnnotations()
99109
if annotations == nil {
100110
annotations = map[string]string{}
101111
}
102-
annotations[runtimev1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...)
112+
annotations[runtimev1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookName)
103113
if annotations[runtimev1.PendingHooksAnnotation] == "" {
104114
delete(annotations, runtimev1.PendingHooksAnnotation)
105115
}
106116
obj.SetAnnotations(annotations)
107117

108-
if err := patchHelper.Patch(ctx, obj); err != nil {
109-
return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ","))
118+
// In some cases it is preferred to not update resourceVersion in the input object,
119+
// because this could lead to conflict errors e.g. when patching at the end of a reconcile loop.
120+
if !updateResourceVersionOnObject {
121+
obj = obj.DeepCopyObject().(client.Object)
122+
}
123+
if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil {
124+
return errors.Wrapf(err, "failed to mark %q hook as done", hookName)
110125
}
111126

112127
return nil
@@ -125,16 +140,17 @@ func IsOkToDelete(obj client.Object) bool {
125140
}
126141

127142
// MarkAsOkToDelete adds the OkToDeleteAnnotation annotation to the object and patches it.
128-
func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error {
143+
func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool) error {
144+
if _, ok := obj.GetAnnotations()[runtimev1.OkToDeleteAnnotation]; ok {
145+
return nil
146+
}
147+
129148
gvk, err := apiutil.GVKForObject(obj, c.Scheme())
130149
if err != nil {
131150
return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to get GVK for object", klog.KObj(obj))
132151
}
133152

134-
patchHelper, err := patch.NewHelper(obj, c)
135-
if err != nil {
136-
return errors.Wrapf(err, "failed to mark %s %s as ok to delete", gvk.Kind, klog.KObj(obj))
137-
}
153+
orig := obj.DeepCopyObject().(client.Object)
138154

139155
annotations := obj.GetAnnotations()
140156
if annotations == nil {
@@ -143,7 +159,12 @@ func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) e
143159
annotations[runtimev1.OkToDeleteAnnotation] = ""
144160
obj.SetAnnotations(annotations)
145161

146-
if err := patchHelper.Patch(ctx, obj); err != nil {
162+
// In some cases it is preferred to not update resourceVersion in the input object,
163+
// because this could lead to conflict errors e.g. when patching at the end of a reconcile loop.
164+
if !updateResourceVersionOnObject {
165+
obj = obj.DeepCopyObject().(client.Object)
166+
}
167+
if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil {
147168
return errors.Wrapf(err, "failed to mark %s %s as ok to delete", gvk.Kind, klog.KObj(obj))
148169
}
149170

0 commit comments

Comments
 (0)