Set StoppedByUser earlier in the process of stopping #17077
Conversation
openshift-ci
bot
added
release-note
approved
|
Still needs a test |
mheon
force-pushed
the
set_stopping_early
branch
from
daf7926
to
85e6be7
Compare
|
Test added |
|
Do you have a sense for how easy/hard it is to trigger the restart? I started a loop-test, running against |
|
Hm. It should be 100%. I'll look into it more... |
|
Here's what I'm seeing:
Does that make any sense to you? Does it help? |
|
Update: even with your PR, $ bin/podman run -d --restart=always --name foo quay.io/libpod/testimage:20221018 date
[cid]
$ bin/podman logs foo | wc -l
26
$ bin/podman stop foo
foo
$ bin/podman logs foo|wc -l
195
$ !!
216 |
|
I think that's probably a bug... Just a separate one. I'll look into what Docker does to be sure. |
|
Yep, that's a separate bug. I'll handle it here. |
|
Ack. Any idea how to reproduce the original bug that you set out to fix? |
|
And, #17083 (purported fix for the hang) is merged, please rebase before pushing |
|
@edsantiago The only reproducer I'm aware of uses podman-compose, which introduces enough questions into what's going on that I haven't been able to reproduce without it. |
|
LGTM |
| Expect(stop).Should(Exit(0)) | ||
|
|
||
| // This is ugly, but I don't see a better way | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Does it need the 10? Could we get away with less? Perhaps 5?
|
Fedora root tests are timing out after 90 minutes. @edsantiago or @cevich I'm seeing this in a number of places, do we know the cause? |
|
Changes LGTM with happy tests |
|
@TomSweeneyRedHat timeout is fixed (:crossed_fingers:) in #17083 |
There was a problem hiding this comment.
Blocking for merge because test needed (as discussed in conversation, test passes on main). It's possible that there's no reproducer that will fail on main, and maybe that's fine, but I don't want anyone to restart the flakes, have them pass, and then merge this overnight.
|
At this point, I don't really know what's going on with restart policy and podman-compose, but this does not reproduce on main. Still, we aren't actually testing that restart policy works in this way anywhere, so I think this is still worth merging just to make sure we don't regress in the future. I'll rebase to fix CI. |
The StoppedByUser variable indicates that the container was requested to stop by a user. It's used to prevent restart policy from firing (so that a restart=always container won't restart if the user does a `podman stop`. The problem is we were setting it *very* late in the stop() function. Originally, this was fine, but after the changes to add the new Stopping state, the logic that triggered restart policy was firing before StoppedByUser was even set - so the container would still restart. Setting it earlier shouldn't hurt anything and guarantees that checks will see that the container was stopped manually. Fixes containers#17069 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
mheon
force-pushed
the
set_stopping_early
branch
from
85e6be7
to
1ab833f
Compare
|
SGTM. I was tempted to suggest ditching the expensive test, ... but I guess it's still a good regression check. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, 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 |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
The StoppedByUser variable indicates that the container was requested to stop by a user. It's used to prevent restart policy from firing (so that a restart=always container won't restart if the user does a
podman stop. The problem is we were setting it very late in the stop() function. Originally, this was fine, but after the changes to add the new Stopping state, the logic that triggered restart policy was firing before StoppedByUser was even set - so the container would still restart.Setting it earlier shouldn't hurt anything and guarantees that checks will see that the container was stopped manually.
Fixes #17069
Does this PR introduce a user-facing change?