Ignore mount errors except ErrContainerUnknown when cleaningup container #11604
Ignore mount errors except ErrContainerUnknown when cleaningup container #11604openshift-merge-robot merged 1 commit intocontainers:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
libpod/runtime_cstorage.go
Outdated
There was a problem hiding this comment.
No need for two ifs, throw the other one in here and or them together so we still get a good Infof.
Unless you think that DeleteContainer will still work if we get a ErrLayerUnknown but not an ErrContainerUnknown?
There was a problem hiding this comment.
Ah, reading back to the old conversation in the linked issue, that does appear to be the assumption...
Maybe we should just logrus.Errorf() any errors that come out of Unmount and unconditionally proceed to the DeleteContainer()?
There was a problem hiding this comment.
Ok how about now, we always try to delete, if we can not determine if the container is mounted or if we fail to unmount the container.
b8f6b5e to
34f719f
Compare
9923cb8 to
f040928
Compare
|
@containers/podman-maintainers PTAL |
|
LGTM |
vrothberg
left a comment
There was a problem hiding this comment.
The title suggest that we're ignoring ErrLayerUnknown but the code doesn't do that. It'll ignore all errors (but ErrContainerUnknown).
libpod/runtime_cstorage.go
Outdated
There was a problem hiding this comment.
| logrus.Errorf("error checking if container %q is mounted, attempting to delete: %v", idOrName, err) | |
| logrus.Errorf("Error checking if container %q is mounted, attempting to delete: %v", idOrName, err) |
There was a problem hiding this comment.
Since we have lots of different cases for logrus.Errorf, I would rather just get this merged and then work on a separate PR to fix all of the logrus.Errorf calls. We need to decide if they should all be UpperCase or lower case.
There was a problem hiding this comment.
The convention for logs is always upper case (errors lower case), so I think the decision has been made on that already.
|
The original error case was to ignore ErrLayerUnknown. |
I don't understand. Do we want to ignore only this error or all? |
|
@vrothberg It was my suggestion - the code seems brittle enough that it seemed worth attempting the removal, regardless of error message. |
Ah, thanks for clarifying. Can we update the commit message to reflect that? It really looked unintentional to me. |
Fixes: containers#11207 [NO TESTS NEEDED] Since I don't know how to get into this situation. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
f040928 to
b6fecbb
Compare
|
LGTM |
Fixes: #11207
[NO TESTS NEEDED] Since I don't know how to get into this situation.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com