Ignore mount errors except ErrContainerUnknown when cleaningup container #11604
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| @@ -112,12 +112,14 @@ func (r *Runtime) removeStorageContainer(idOrName string, force bool) error { | |||
| return errors.Wrapf(define.ErrCtrStateInvalid, "container %q is mounted and cannot be removed without using force", idOrName) | |||
| } | |||
| } else if _, err := r.store.Unmount(ctr.ID, true); err != nil { | |||
| if errors.Cause(err) == storage.ErrContainerUnknown { | |||
| if errors.Is(err, storage.ErrContainerUnknown) { | |||
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.
rhatdan
force-pushed
the
deleteContainer
branch
from
b8f6b5e to
34f719f
Compare
rhatdan
force-pushed
the
deleteContainer
branch
2 times, most recently
from
9923cb8 to
f040928
Compare
|
@containers/podman-maintainers PTAL |
|
LGTM |
libpod/runtime_cstorage.go
Outdated
| @@ -106,18 +106,18 @@ func (r *Runtime) removeStorageContainer(idOrName string, force bool) error { | |||
| logrus.Infof("Storage for container %s already removed", ctr.ID) | |||
| return nil | |||
| } | |||
| return errors.Wrapf(err, "error looking up container %q mounts", idOrName) | |||
| logrus.Errorf("error checking if container %q is mounted, attempting to delete: %v", idOrName, err) | |||
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 <[email protected]>
rhatdan
force-pushed
the
deleteContainer
branch
from
f040928 to
b6fecbb
Compare
|
LGTM |
rhatdan
changed the title
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #11207
[NO TESTS NEEDED] Since I don't know how to get into this situation.
Signed-off-by: Daniel J Walsh [email protected]