Fix for Machine (sometimes) getting stuck with empty phase when finalizer addition exhausts retry budget - #1142
Fix for Machine (sometimes) getting stuck with empty phase when finalizer addition exhausts retry budget#1142elankath wants to merge 3 commits into
Conversation
gardener-prow
Bot
added
do-not-merge/needs-kind
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>
200fb6e to
6d3103f
Compare
gardener-prow
Bot
removed
the
do-not-merge/needs-kind
gardener-prow
Bot
added
the
do-not-merge/work-in-progress
…on 409 conflicts Treat 409 Conflict in addMachineFinalizers as a no-op (return nil) so the worker retry budget is not consumed by informer cache lag. Fixes gardener#1141 Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>
Fixes gardener#1141 Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>
80ac4b6 to
7e13c8d
Compare
|
NOTE: We have a flaky test "set LDRCBST annotation on the machineSet and TriggerDeletionByMCM annotation is not set on the machineSet" which is timing dependent and tends to fail. The fix for this test should be addressed in some other PR. |
elankath
removed
the
do-not-merge/work-in-progress
| // Informer cache is stale; the watch event will re-enqueue the machine once the | ||
| // cache catches up, so this is not a retriable error from the worker's perspective. | ||
| // Returning nil avoids burning a retry slot from the fixed budget (DefaultMaxRetries). | ||
| return machineutils.ShortRetry, nil |
There was a problem hiding this comment.
There is a ConflictRetry timeout defined in utils. Would you instead want to use that is the 5s ShortRetry enough?
There was a problem hiding this comment.
I don't think it matters, the caller discards the retry period. Can we change the signature to return just an error then? The function is only used in one place which discards the retry period returned.
| // had the MCM finalizer added would be silently dropped here. Re-enqueue it so | ||
| // reconcileClusterMachine is reached and the machine advances past the empty phase. | ||
| if !sets.NewString(oldMachine.Finalizers...).HasAll(newMachine.Finalizers...) { | ||
| klog.V(4).Infof("updateMachine: machine %q gained finalizer — re-enqueuing despite unchanged Generation", newMachine.Name) |
There was a problem hiding this comment.
Would come handy when debugging
| klog.V(4).Infof("updateMachine: machine %q gained finalizer — re-enqueuing despite unchanged Generation", newMachine.Name) | |
| klog.V(3).Infof("updateMachine: machine %q gained finalizer — re-enqueuing despite unchanged Generation", newMachine.Name) |
| } | ||
|
|
||
| // Add finalizers if not present on machine object | ||
| _, err = c.addMachineFinalizers(ctx, machine) |
There was a problem hiding this comment.
I was wondering if we can just requeue the machine here like:
retryPeriod, err := c.addMachineFinalizers(ctx, machine)
if err != nil {
c.enqueueMachineAfter(machine, time.Duration(retryPeriod), "failed to add MachineFinalizer")
return nil
}instead of tracking the update event and requeuing in updateMachine().
And I am not sure if we should proceed if the finalizer is not added on the machine (since we return nil for ConflictError in c.addMachineFinalizers()).
There was a problem hiding this comment.
BTW I was wondering if MachineFinalizers should be added by machine controller or machineset controller. And if machine controller does add it, should we be requequing the machine or continue?
4 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1141
Special notes for your reviewer:
Release note: