Fix deadlock between 'podman ps' and 'container inspect' commands #16327
Conversation
| @@ -581,20 +581,22 @@ func (p *Pod) Status() (map[string]define.ContainerStatus, error) { | |||
| } | |||
|
|
|||
| func containerStatusFromContainers(allCtrs []*Container) (map[string]define.ContainerStatus, error) { | |||
| // We need to lock all the containers | |||
There was a problem hiding this comment.
Of course, I saw this comment and my assumption is this is not correct now. Please let me know if I am wrong.
|
/approve i'm looking deeper on this r/n. i dont know about that assumption but there is an easy enough way to find ou8t |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, tyler92 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
|
@mheon i dont see evidence that this is no longer true unless i missed some change to container and pod locking. What might make more sense would be to iterate the containers, grab a lock, get the container information, release the lock? One loop vs two ? |
|
Code looks fine as written, though personally would have avoided the anonymous function. The core issue is similar to the reason we had to rewrite pod removal - holding more than one container lock simultaneously, without specific ordering, can cause deadlocks with other commands that want to lock, say, a container and its dependencies. We got rid of one instance of this (pod removal), this looks like another. |
|
LGTM, for reference |
I Can rewrite with explicit Unlock calls and without an anonymous function |
Fixes: containers#16326 [NO NEW TESTS NEEDED] Signed-off-by: Mikhail Khachayants <[email protected]>
tyler92
force-pushed
the
fix-deadlock-pod-ps-inspect
branch
from
4d332a5 to
f355900
Compare
|
I've published my small pet-project tool for debugging such deadlocks (in case it's impossible to use IDE or GDB): https://github.com/tyler92/podman-locks. It would be great if someday it will help someone other than me. (I hope it's allowed to attach such links here) |
Thanks for sharing! I usually use the BPF tools from |
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Thanks, I will take it into account! The main difference with my tool is that it can be launched after the deadlock has occurred and the podman process has already started as usual without any external tools. |
|
/cherry-pick v4.3.0 |
|
@vrothberg: new pull request could not be created: failed to create pull request against containers/podman#v4.3.0 from head openshift-cherrypick-robot:cherry-pick-16327-to-v4.3.0: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","field":"base","code":"invalid"}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"} In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick v4.3 |
|
@ashley-cui: new pull request created: #16448 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Thanks, @ashley-cui :) |
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #16326
[NO NEW TESTS NEEDED]
Signed-off-by: Mikhail Khachayants [email protected]
Does this PR introduce a user-facing change?