Only remove the cleanup finalizer if the cleanup succeeds #7286
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0470fa7 to
41f5ebc
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
41f5ebc to
95eeab9
Compare
| } | ||
|
|
||
| solver, err := c.solverFor(ch.Spec.Type) | ||
| if err != nil { | ||
| log.Error(err, "error getting solver for challenge") | ||
| return nil |
There was a problem hiding this comment.
question: Previously we swallowed the error and returned nil. Same in the if err := solver.CleanUp(ctx, ch); err != nil block below.
What's the motivation for the change to now start returning the error here (and below)? What effect will it have?
There was a problem hiding this comment.
There are two situations in which a cleanup is performed:
acme.IsFinalState(ch.Status.State)!ch.DeletionTimestamp.IsZero()
The old code swallowed errors when !ch.DeletionTimestamp.IsZero() but returned errors when acme.IsFinalState(ch.Status.State). So this change will only affect the first case.
In that case, the deletion of the challenge resource will be blocked until the handleFinalizer is successful. An error will be returned if something went wrong and the CleanUp function will be retried.
The previous behavior for the first case was that the CleanUp function was called once and the error was only logged and the finalizer deleted unconditionally.
There was a problem hiding this comment.
🤔 I just found there is a counter-issue: #7234
Challenges that are stuck in their cleanup phase: cleanup is failing and the resource is not being deleted...
|
/hold |
|
Issues go stale after 90d of inactivity. |
|
Stale issues rot after 30d of inactivity. |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@inteon: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This addresses two problems:
acme.IsFinalStateblock, but in the former, the Status.Processing and Status.Presented fields were not being reset and in the latter case, the finalizer wasn't being removed if the cleanup succeeded.Part of:
supersedes #5126
Kind
/kind cleanup
Release Note