Improve robustness of pod removal #3082
Conversation
Removing a pod must first removal all containers in the pod. Libpod requires the state to remain consistent at all times, so references to a deleted pod must all be cleansed first. Pods can have many containers in them. We presently iterate through all of them, and if an error occurs trying to clean up and remove any single container, we abort the entire operation (but cannot recover anything already removed - pod removal is not an atomic operation). Because of this, if a removal error occurs partway through, we can end up with a pod in an inconsistent state that is no longer usable. What's worse, if the error is in the infra container, and it's persistent, we get zombie pods - completely unable to be removed. When we saw some of these same issues with containers not in pods, we modified the removal code there to aggressively purge containers from the database, then try to clean up afterwards. Take the same approach here, and make cleanup errors nonfatal. Once we've gone ahead and removed containers, we need to see pod deletion through to the end - we'll log errors but keep going. Also, fix some other small things (most notably, we didn't make events for the containers removed). Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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-robot
added
approved
|
I'm thinking about modifying the API to also return a map[string]error for errors removing individual containers, so we can accurately return errors that occurred as we evicted individual containers (instead of only logging like we do now) |
Ensure that, if an error occurs somewhere along the way when we remove a pod, it's preserved until the end and returned, even as we continue to remove the pod. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
Pushed a patch that fixes without API changes. It's less elegant, but gets the job done. |
|
Test issues: this PR bumped CGroup removal from a warning that was never reported, to a reported error. It looks like removing cgroupfs CGroups on CI is very unreliable? |
|
@cevich I'm seeing the |
|
@mheon thx, useful to know it goes with passing tests also. (edit) Mmmm, I see it's coming from vendor code as well. I was going to send a PR and even add a unittest, but...sigh wouldn't be accepted upstream (most likely). |
|
LGTM |
| // Clean up network namespace, cgroups, mounts | ||
| if err := ctr.cleanup(ctx); err != nil { | ||
| return err | ||
| if removalErr == nil { | ||
| removalErr = err |
There was a problem hiding this comment.
I think it would be good to add a logrus.debug here. Might make it easier later on to track down exactly what went south. Ditto the other "removalErr = err" lines below.
There was a problem hiding this comment.
I think most of these should already be good errors including the container ID, but I'll slap some Wrapfs on them to be sure
There was a problem hiding this comment.
Alright, I checked, and everything not already wrapped is returning a well-formatted, wrapped error
There was a problem hiding this comment.
And the errors are clear enough to know that it came from ctr.cleanup failing at line 231 vs ctr.teardownStorage failing at line 240?
There was a problem hiding this comment.
If you grep for the specific strings we wrap with, yes - you'll be able to get the specific function that failed
There was a problem hiding this comment.
OK then, I'll trust you on this one @mheon, thanks for the discussion.
| // Clean up network namespace, cgroups, mounts | ||
| if err := ctr.cleanup(ctx); err != nil { | ||
| return err | ||
| if removalErr == nil { | ||
| removalErr = err |
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Removing a pod must first removal all containers in the pod. Libpod requires the state to remain consistent at all times, so references to a deleted pod must all be cleansed first.
Pods can have many containers in them. We presently iterate through all of them, and if an error occurs trying to clean up and remove any single container, we abort the entire operation (but cannot recover anything already removed - pod removal is not an atomic operation).
Because of this, if a removal error occurs partway through, we can end up with a pod in an inconsistent state that is no longer usable. What's worse, if the error is in the infra container, and it's persistent, we get zombie pods - completely unable to be removed.
When we saw some of these same issues with containers not in pods, we modified the removal code there to aggressively purge containers from the database, then try to clean up afterwards. Take the same approach here, and make cleanup errors nonfatal. Once we've gone ahead and removed containers, we need to see pod deletion through to the end - we'll log errors but keep
going.
Also, fix some other small things (most notably, we didn't make events for the containers removed).