Skip to content

libpod: cleanupNetwork() return error#23553

Merged
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Luap99:net-cleanup-err
Aug 9, 2024
Merged

libpod: cleanupNetwork() return error#23553
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Luap99:net-cleanup-err

Conversation

@Luap99
Copy link
Copy Markdown
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

@openshift-ci
Copy link
Copy Markdown
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

Details 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
Copy Markdown
Member Author

Luap99 commented Aug 8, 2024

@mheon PTAL

@mheon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

edsantiago commented Aug 8, 2024

https://api.cirrus-ci.com/v1/artifact/task/4937874783600640/html/int-podman-rawhide-rootless-host-sqlite.log.html#t--Podman-kube-play-test-env-value-from-configmap-and-replace-should-reuse-the-configmap-volume--1

@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Aug 8, 2024

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
Copy Markdown
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
Copy Markdown
Member

edsantiago commented Aug 8, 2024

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

@Luap99
Copy link
Copy Markdown
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.

Luap99
Luap99 commented Aug 8, 2024
View reviewed changes
rhatdan
rhatdan reviewed Aug 8, 2024
View reviewed changes
rhatdan
rhatdan reviewed Aug 8, 2024
View reviewed changes
libpod/container_internal_linux.go Outdated
Copy link
Copy Markdown
Member

@rhatdan rhatdan Aug 8, 2024

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
Copy Markdown
Member

edsantiago commented Aug 8, 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

@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Aug 9, 2024

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

@Luap99
Copy link
Copy Markdown
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.

@Luap99
libpod: cleanupNetwork() return error
f2a03e5 
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 <pholzing@redhat.com>
@rhatdan
Copy link
Copy Markdown
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
@Luap99 Luap99 deleted the net-cleanup-err branch August 9, 2024 10:48
@Luap99 Luap99 mentioned this pull request Aug 9, 2024
Merged
@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.

Reviewers

@rhatdan rhatdan rhatdan left review comments

Assignees

@rhatdan rhatdan

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Luap99 @mheon @edsantiago @rhatdan