Skip to content

Conversation

@Luap99
Copy link
Member

@Luap99 Luap99 commented May 31, 2023

Systemd doesn't support never and logs a warning, systemd uses no as default so we do not have to specify it at all.

Check systemd.service(5) for the systemd docs.

Fixes #18743

Does this PR introduce a user-facing change?

None

Systemd doesn't support `never` and logs a warning, systemd uses no as
default so we do not have to specify it at all.

Check systemd.service(5) for the systemd docs.

Fixes containers#18743

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Luap99
Copy link
Member Author

Luap99 commented May 31, 2023

@edsantiago PTAL

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023
@TomSweeneyRedHat
Copy link
Member

if @edsantiago is hip with this, LGTM

@edsantiago
Copy link
Member

LGTM but I think it's more than just that one test:

$ ack 'Restart.*nev'
cmd/podman/common/create.go
634:                    [removed, I think this one is OK, it's not systemd]
 
contrib/systemd/system/podman-play-kube@.service
10:Restart=never
 
pkg/util/utils.go
674:            if strings.ToLower(splitRestart[0]) == "never" {
               [I think this one is ok also]

And, in the process of searching for combinations of restart+never (while ignoring the kube restartPolicy), I found:

# Kill the pod and make sure the service is not running.
# The restart policy is set to "never" since there is no
# design yet for propagating exit codes up to the service
# container.
run_podman pod kill test_pod
for i in {0..20}; do
run systemctl is-active $service_name
if [[ $output == "failed" ]]; then
break
fi
sleep 0.5
done
is "$output" "inactive" "systemd service transitioned to 'inactive' state: $service_name"

which seems to be obsolete. Would you be willing to take those on as part of this PR? If not, np, I can file an issue. I'm just not comfortable submitting a PR because there are edge cases I'm sure I haven't considered.

@Luap99
Copy link
Member Author

Luap99 commented Jun 1, 2023

contrib/systemd/system/podman-play-kube@.service

This is not checked into git, I do not have such file locally. Looks like it was renamed and then fixed in d7f6d35

The 250-systemd test change I cannot really tell we should wait for @vrothberg to comment on that. It tests the behavior without exit policy and the kube template does not set one so it is fine IMO.

@edsantiago
Copy link
Member

Ack, thanks. And sorry for false-alarming on a dead file.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2023
@openshift-merge-robot openshift-merge-robot merged commit 50f9345 into containers:main Jun 1, 2023
@Luap99 Luap99 deleted the systemd-restart branch June 1, 2023 11:53
@vrothberg
Copy link
Member

Opened #18854 to remove the confusing comment.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

quadlet (and kube?): systemd Restart=never is invalid

5 participants