Fix, and reduce repetitiveness, in container cleanup error handling #18636
Conversation
openshift-ci
bot
added
release-note-none
approved
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
To pass CI, can you add the magic |
| reportErrorf := func(msg string, args ...any) { | ||
| err := fmt.Errorf(msg, args...) // Always use fmt.Errorf instead of just logrus.Errorf(…) because the format string probably contains %w | ||
| if cleanupErr == nil { | ||
| cleanupErr = err | ||
| } else { | ||
| logrus.Errorf("%s", err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is nice but maybe to avoid duplicating this function in wto palces we could export this to another function which just accepts cleanupErr as arg and then returns this anonymous function.
There was a problem hiding this comment.
I’m not familiar with the codebase and I’m fine with making whatever adjustments are requested. I’ll just push back a bit, once :)
My general impression is that this, ~silently proceeding on failures, is a fairly unusual error handling model, and I’m not too enthusiastic about codifying this approach and making it look like an approach to be followed elsewhere. In some ideal end state (which we could never test properly), I imagine we would track which parts of the cleanup succeeded and which are still outstanding, so that the user can re-start the cleanup and get the other parts done.
(Also, so far there are only two functions that use it. My personal, not at all evidence-based, rule of thumb is to consolidate at 3 instances.)
[NO NEW TESTS NEEDED] ... because testing this would require us to intentionally create an inconsistent state, which should ideally not be possible... (and because at this point I don't even know what the reported failure was.) Signed-off-by: Miloslav Trmač <[email protected]>
Use a shared helper instead of copy&pasting the handling of cleanupErr EIGHT times. This changes the wording of logged error text, and the error in one case, a bit. Signed-off-by: Miloslav Trmač <[email protected]>
Use a helper to handle the cleanupErr logic instead of copy&pasting it EIGHT times. Also modifies the returned errors to be wrapped with a context, and changes the text of the logged errors a bit. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
cleanupStorage-error
branch
from
3d72d2f to
032d4a9
Compare
|
Once the location of the new function is determined, the rest of the code LGTM. Nice condensation @mtrmac ! |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
https://github.com/containers/podman/pull/18634/checks?check_run_id=13617533713 contains a failure with
Fix that
nilusage; then look at the pattern, and replace copy&pasted logic with helpers, at the cost of changing the error / log messages a bit.See individual commits for more details. If you’d prefer just the two-line bug fix without the other changes, that’s fine with me too.
Does this PR introduce a user-facing change?