Ensure pod infra containers have an exit command #7283
Conversation
|
[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
the
approved
mheon
force-pushed
the
pod_infra_has_exit_cmd
branch
from
00244f5 to
3a77203
Compare
|
Anyway to add a test? |
There was a problem hiding this comment.
LGTM, tested and it fixes #7103
[acui@localhost podman]$ ./bin/podman pod create --name test -p 5000:5000
564f217152dcceccc8f8200d8d4060f1b7602e64db104d07b8d3ee2dbc9ccb22
[acui@localhost podman]$ ./bin/podman create -d --pod test -p 5000:5000 registry:2
9c4d770b0cefd11b112ca13e415a2cd231fe1cb1866754fbc4d6eaa204902a42
[acui@localhost podman]$ ./bin/podman pod start test
564f217152dcceccc8f8200d8d4060f1b7602e64db104d07b8d3ee2dbc9ccb22
[acui@localhost podman]$ curl localhost:5000
[acui@localhost podman]$ ./bin/podman pod stop test
564f217152dcceccc8f8200d8d4060f1b7602e64db104d07b8d3ee2dbc9ccb22
[acui@localhost podman]$ ./bin/podman pod start test
564f217152dcceccc8f8200d8d4060f1b7602e64db104d07b8d3ee2dbc9ccb22
[acui@localhost podman]$ curl localhost:5000
3a77203 to
4bb4356
Compare
|
Pushed an update to fix pods where no infra container was created. |
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
mheon
force-pushed
the
pod_infra_has_exit_cmd
branch
from
4bb4356 to
fb1465e
Compare
|
Rebased, hopefully CI will be happy now |
|
Sad trombone. |
|
I think the bindings test is wrong, will fix tomorrow morning. Not sure what's going on with rootless remote and F31 - they seem to be consistently failing, but initial glance doesn't say why. |
mheon
force-pushed
the
pod_infra_has_exit_cmd
branch
from
fb1465e to
e3d8728
Compare
|
Bindings tests should be fixed |
Most Libpod containers are made via `pkg/specgen/generate` which includes code to generate an appropriate exit command which will handle unmounting the container's storage, cleaning up the container's network, etc. There is one notable exception: pod infra containers, which are made entirely within Libpod and do not touch pkg/specgen. As such, no cleanup process, network never cleaned up, bad things can happen. There is good news, though - it's not that difficult to add this, and it's done in this PR. Generally speaking, we don't allow passing options directly to the infra container at create time, but we do (optionally) proxy a pre-approved set of options into it when we create it. Add ExitCommand to these options, and set it at time of pod creation using the same code we use to generate exit commands for normal containers. Fixes containers#7103 Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
pod_infra_has_exit_cmd
branch
from
e3d8728 to
a071939
Compare
|
Current thinking: we have a race similar to #5747 but with stopping pods, not containers - and it's causing the bindings tests to fail as we hit the pod for status info before the cleanup process can run. |
|
Can you add a podman wait? |
|
I'm looking into a similar solution to what we used in #5747 - force a cleanup before the API call returns. |
This should help alleviate races where the pod is not fully cleaned up before subsequent API calls happen. Signed-off-by: Matthew Heon <[email protected]>
|
Added code to ensure cleanup happens before the Pod Stop API call finishes. |
| } | ||
|
|
||
| // Ignore containers that are running/paused | ||
| if !ctr.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) { |
There was a problem hiding this comment.
Not for this review, but it might be nice to create a isContainerPausedOrRunning() utility function. I think we've this big blob of test in a few places. But not for this PR.
|
LGTM |
|
Tests are green! @rhatdan @baude @giuseppe @TomSweeneyRedHat PTAL and merge, would like to get this backported to 2.0 for 2.0.5 |
|
/lgtm |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Most Libpod containers are made via
pkg/specgen/generatewhich includes code to generate an appropriate exit command which will handle unmounting the container's storage, cleaning up the container's network, etc. There is one notable exception: pod infra containers, which are made entirely within Libpod and do not touch pkg/specgen. As such, no cleanup process, network never cleaned up, bad things can happen.There is good news, though - it's not that difficult to add this, and it's done in this PR. Generally speaking, we don't allow passing options directly to the infra container at create time, but we do (optionally) proxy a pre-approved set of options into it when we create it. Add ExitCommand to these options, and set it at time of pod creation using the same code we use to generate exit commands for normal containers.