Skip to content

Conversation

@Honny1
Copy link
Member

@Honny1 Honny1 commented Nov 27, 2025

  • Update documentation: Differentiate unless-stopped from always - containers stopped by the user before a reboot will not restart.
  • Add should-start-on-boot filter: Identify containers that require a restart after a system reboot.
  • Update command documentation: Add restart-policy and label! filters to the documentation for container commands (rm, ps, start, stop, pause, unpause, restart).
  • Add restart-policy and shoud-start-on-boot to completions.
  • Update service: Update podman-restart.service to use the needs-restart=true filter.
  • Preserve state: Preserve the StoppedByUser state across reboots.
  • Update API: Add a ShouldStartOnBoot() method to the Container API.
  • Update documentation: Add descriptions for the should-start-on-boot filter.

Fixes: https://issues.redhat.com/browse/RHEL-129405
Fixes: #20418

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

The `unless-stopped` restart policy now correctly matches Docker behavior by preserving the user-stopped state across reboots, and includes a new `should-start-on-boot` filter.

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 27, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1

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 Nov 27, 2025
@Honny1 Honny1 force-pushed the fix-unless-stopped-reboot branch from 5b28844 to 33a8d52 Compare November 27, 2025 13:06
@packit-as-a-service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

@Luap99
Copy link
Member

Luap99 commented Nov 27, 2025

I haven't looked at the code but I find needs-restart a confusing name, internally we also already have a ShouldRestart() function so it isn't at all clear what this means.

Given this only targets the on boot start , maybe something more like should-start-on-boot/ShouldStartOnBoot?

@Honny1 Honny1 force-pushed the fix-unless-stopped-reboot branch from 33a8d52 to b5b5d8f Compare November 27, 2025 13:40
@Honny1
Copy link
Member Author

Honny1 commented Nov 27, 2025

Given this only targets the on boot start , maybe something more like should-start-on-boot/ShouldStartOnBoot?

That is a much better name. I have renamed it

@Honny1 Honny1 force-pushed the fix-unless-stopped-reboot branch from b5b5d8f to 22bc44c Compare November 27, 2025 14:16
@Honny1 Honny1 marked this pull request as ready for review November 27, 2025 14:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2025
@Honny1
Copy link
Member Author

Honny1 commented Dec 1, 2025

PTAL @mheon @Luap99


// Indicate whether or not the container will should start after a reboot of system
func (c *Container) ShouldStartOnBoot() bool {
logrus.Debugf("Checking if container %s needs restart", c.ID())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should drop this, since it's part of a filter, we'll spam the debug logs

}

notExecuted := c.state.State == define.ContainerStateConfigured || c.state.State == define.ContainerStateCreated
if notExecuted {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use c.ensureState for this.

- Update documentation: Differentiate `unless-stopped` from `always` - containers stopped by the user before a reboot will not restart.
- Add `should-start-on-boot` filter: Identify containers that require a restart after a system reboot.
- Update command documentation: Add `restart-policy` and `label!` filters to the documentation for container commands (rm, ps, start, stop, pause, unpause, restart).
- Add `restart-policy` and `shoud-start-on-boot` to completions.
- Update service: Update `podman-restart.service` to use the `needs-restart=true` filter.
- Preserve state: Preserve the `StoppedByUser` state across reboots.
- Update API: Add a `ShouldStartOnBoot()` method to the Container API.
- Update documentation: Add descriptions for the `should-start-on-boot` filter.

Fixes: https://issues.redhat.com/browse/RHEL-129405
Fixes: containers#20418

Signed-off-by: Jan Rodák <[email protected]>
@Honny1 Honny1 force-pushed the fix-unless-stopped-reboot branch from 22bc44c to 4d3c631 Compare December 2, 2025 14:41
@Honny1
Copy link
Member Author

Honny1 commented Dec 2, 2025

@mheon I made changes based on your comments.

@mheon
Copy link
Member

mheon commented Dec 2, 2025

LGTM

@Honny1
Copy link
Member Author

Honny1 commented Dec 2, 2025

/packit retest-failed

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

The duplication in the filter description is ugly as hell, we should create an issue and make the filter option de-duplicated like the rest unless I am overlooking something why it isn't right now.

@Honny1
Copy link
Member Author

Honny1 commented Dec 4, 2025

@Luap99

I created an issue for the filter documentation deduplication: #27681

Also, it seems that combining text with /_lgtm doesn't work.

@Luap99
Copy link
Member

Luap99 commented Dec 4, 2025

/lgtm

Thanks

Also, it seems that combining text with /_lgtm doesn't work.

It does work normally, I suppose this is just one of the typical bot issues where it is offline or misses messages for short time. One of the reason why we want to remove the bot. Let's see if this works now :)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 5508d87 into containers:main Dec 4, 2025
79 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

podman-restart.service Does Not Restart "unless-stopped" Containers

3 participants