Fix unless-stopped containers not restarting after podman-restart-service stop them#28236
Fix unless-stopped containers not restarting after podman-restart-service stop them#28236Honny1 wants to merge 1 commit intocontainers:mainfrom
unless-stopped containers not restarting after podman-restart-service stop them#28236Conversation
|
Honny1
force-pushed
the
fix-unless-stopped
branch
from
a17cb8d to
e7f7ead
Compare
Honny1
force-pushed
the
fix-unless-stopped
branch
from
e7f7ead to
faab7cb
Compare
cmd/podman/containers/stop.go
Outdated
| if registry.IsRemote() { | ||
| _ = flags.MarkHidden("cidfile") | ||
| _ = flags.MarkHidden("ignore") | ||
| _ = flags.MarkHidden(notStoppedByUserFlagName) |
There was a problem hiding this comment.
if it is already hidden no point to do it here again
| if !notStoppedByUser { | ||
| c.state.StoppedByUser = true | ||
| } |
There was a problem hiding this comment.
Does this work? If I have restart always does this not mean that with the new flag the cleanup process will keep restarting the container even if that is not what we want, i.e. see logic in shouldRestart() will be triggered in podman container cleanup I think
There was a problem hiding this comment.
No, adding --not-stopped-by-user does not cause cleanup to keep restarting containers in the normal stop flow, even with --restart=always, because the stop path transitions through Stopping state and therefore does not set RestartPolicyMatch=true.
I am not sure whether this is a bug, but when Podman calls stopInternal, the container will not restart, even if the restart policy is always. I will add comments to clarify this.
There was a problem hiding this comment.
We might want a stronger protection here because I can see this getting broken by future changes to how stop works?
Honny1
force-pushed
the
fix-unless-stopped
branch
from
faab7cb to
0ad1c8a
Compare
…service` stop them When `podman-restart.service` stops containers, it marks them as "stopped by user" which breaks the `unless-stopped` restart policy. Add hidden `--not-stopped-by-user` flag to prevent this, allowing `unless-stopped` containers to restart on next boot. Fixes: containers#28152 Fixes: https://issues.redhat.com/browse/RUN-4357 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Honny1
force-pushed
the
fix-unless-stopped
branch
from
0ad1c8a to
1841881
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
LGTM |
|
PTAL @containers/podman-maintainers |
|
My first reaction is that I really hate the API change; StoppedByUser isn't really something we should be exposing, even as a hidden flag... I don't know if I have a better way, though. |
| // An explicit stop is treated as a user-driven lifecycle action. Because of | ||
| // that, this path may not trigger automatic restart-policy handling in cleanup, | ||
| // even when notStoppedByUser is true. | ||
| func (c *Container) StopWithArgs(timeout uint, notStoppedByUser bool) (finalErr error) { |
There was a problem hiding this comment.
I don't think there's much value in exposing notStoppedByUser as an argument. If we do this, I think we have a StopService() that calls stopInternal with stoppedByUser = false
|
|
||
| // Internal, non-locking function to stop container | ||
| // notStoppedByUser controls whether to set the StoppedByUser state field. | ||
| func (c *Container) stopInternal(timeout uint, notStoppedByUser bool) error { |
There was a problem hiding this comment.
Don't do notStoppedByUser - confusing to negate. stoppedByUser is less confusing.
4 participants
When
podman-restart.servicestops containers, it marks them as "stopped by user" which breaks theunless-stoppedrestart policy. Add hidden--not-stopped-by-userflag to prevent this, allowingunless-stoppedcontainers to restart on next boot.Fixes: #28152
Fixes: https://issues.redhat.com/browse/RUN-4357
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?