improve netns cleanup#2112
Merged
openshift-merge-bot[bot] merged 4 commits intocontainers:mainfrom Aug 8, 2024
Merged
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
rhatdan
reviewed
Aug 7, 2024
Member
|
LGTM |
giuseppe
reviewed
Aug 7, 2024
| // the file will work and the kernel will destroy the bind mount in the | ||
| // other ns because of this. We also need it so pasta doesn't leak. | ||
| rErr = fmt.Errorf("failed to unmount NS: at %s: %w", nsPath, err) | ||
| // EINVAL means the path exists but is not mounted, just try to remove the path below |
Member
There was a problem hiding this comment.
could we just use containers/storage/pkg/system.EnsureRemoveAll() here?
Member
Author
There was a problem hiding this comment.
Mhh, I guess it would work but the logic there seems much more complicated especially in the normal case were a unmount+remove works. We don't need any of the recursive dir logic, EnsureRemoveAll() would then read /proc/thread-self/mountinfo every time which seems wasteful
When I wrote this originally I thought we must avoid leaking the netns so I tried to decrement first. However now I think this wrong because podman actially calls into the cleanup function again if it returned an error on the next cleanup attempt. As such we ended up doing a double decrement and the ref counter went below zero causing a sort of issues[1]. Now if we have a bug the other way around were we not decrement correctly this is much less of a problem. It simply means we leak once netns file and the pasta/slirp4netns process which isn't a problem other than needed a bit of resources. [1] containers/podman#21569 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The Run() function is used to run long running command in the netns, namly podman unshare --rootless-netns used that. As such the function actually unlocks for the main command as otherwise a user could hold the lock forever effectively causing deadlocks. Now because we unlock the ref count might change during that time. And just because we create the netns doesn't mean there are now other users of the netns. Therefore the cleanup in runInner() was wrong in that case causing problems for other running containers. To fix this make sure we do not cleanup in the Run() case unless the count is 0. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Podman might call us more than once on the same path. If the path is not mounted or does not exists simply return no error. Second, retry the unmount/remove until the unmount succeeded. For some reason we must use MNT_DETACH as otherwise the unmount call will fail all time the time. However MNT_DETACH means it unmounts async in the background. Now if we call remove on the file and the unmount was not done yet it will fail with EBUSY. In this case we try again until it works or we get another error. This should help containers/podman#19721 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99
changed the title
Member
Author
|
Ok this should be good to go, it will not fix the flake though |
Member
Author
|
@mheon PTAL |
Member
|
LGTM |
Member
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
see commits