Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libpod: cleanupNetwork() return error #23553

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Aug 8, 2024

Return the error not just log as the caller can then decide to log this and exit > 0. I also removed the c.valid check as I do not see what the purpose of this would be. c.valid is only false when the ctr was removed but then we should never get there as Cleanup() will not work on a container in removing state.

Does this PR introduce a user-facing change?

None

Copy link
Contributor

openshift-ci bot commented Aug 8, 2024

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2024
@Luap99
Copy link
Member Author

Luap99 commented Aug 8, 2024

@mheon PTAL

@mheon
Copy link
Member

mheon commented Aug 8, 2024

LGTM

@Luap99 Luap99 added the No New Tests Allow PR to proceed without adding regression tests label Aug 8, 2024
@Luap99
Copy link
Member Author

Luap99 commented Aug 8, 2024

@edsantiago please pick this into the no flake try PRs, I have not seen the flake in #23487 for 3 pushes now but I am not sure if this is just luck because I don't think this is actually changing anything other than the error message.

@edsantiago
Copy link
Member

Also this hang in debian is a new one I don't recall having seen before, don't know if it's related to any recent work

@Luap99
Copy link
Member Author

Luap99 commented Aug 8, 2024

Ack I saw a hang on my PR as well but I cannot see how this change would cause this.

@edsantiago
Copy link
Member

that's #22246, the one Matt is working on

@Luap99
Copy link
Member Author

Luap99 commented Aug 8, 2024

that's #22246, the one Matt is working on

Right but #22246 hangs on pod rm --all not on stop --all which you linked which is the bit that concerns me here.

c.state.NetNS = ""
c.state.NetworkStatus = nil

if c.valid {
return c.save()
// always safe even when there was an error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe or save?

@edsantiago
Copy link
Member

Is this what you were hoping for?

$ podman [options] stop --all -t 0
Error: removing container 953ccd87aeda89841fa7a2e00020d84c78d74a07d67b6983cce00c6e7572ade5 network: unmounting network namespace for container 953ccd87aeda89841fa7a2e00020d84c78d74a07d67b6983cce00c6e7572ade5: failed to remove ns path: remove /run/user/3445/netns/netns-c979509f-2f3d-b0f5-3e2b-3421610b3fe1: device or resource busy

@edsantiago
Copy link
Member

Or this one but I don't see the new error message here either

@Luap99
Copy link
Member Author

Luap99 commented Aug 9, 2024

Is this what you were hoping for?

$ podman [options] stop --all -t 0
Error: removing container 953ccd87aeda89841fa7a2e00020d84c78d74a07d67b6983cce00c6e7572ade5 network: unmounting network namespace for container 953ccd87aeda89841fa7a2e00020d84c78d74a07d67b6983cce00c6e7572ade5: failed to remove ns path: remove /run/user/3445/netns/netns-c979509f-2f3d-b0f5-3e2b-3421610b3fe1: device or resource busy

yes that one should be fixed with the fixes in c/common I think containers/common#2112 #23519

But that doesn't fix the very clear issue of podman calling cleanup twice.

Return the error not just log as the caller can then decide to log this
and exit > 0. I also removed the c.valid check as I do not see what the
purpose of this would be. c.valid is only false when the ctr was removed
but then we should never get there as Cleanup() will not work on a
container in removing state.

Signed-off-by: Paul Holzinger <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d305a34 into containers:main Aug 9, 2024
83 checks passed
@Luap99 Luap99 deleted the net-cleanup-err branch August 9, 2024 10:48
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 8, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants