contrib/systemd cleanups #7023
contrib/systemd cleanups #7023
Conversation
Symlink the user to the system services in `contrib/systemd`. There is no diference between the services, so we can reduce redundancy while not breaking downstream packages which might already be referencing `./contrib/systemd/user`. Signed-off-by: Valentin Rothberg <[email protected]>
openshift-ci-robot
added
the
approved
There was a problem hiding this comment.
Thanks for the cleanups! While you are at it, any chance that Type=oneshot could be fixed as well? This is really not an one-shot kind of service, and the status of the unit is currently forever "activating" which is confusing. The default Type=simple should work fine, unless you implement the sd_notify(3) protocol, then Type=notify is correct.
| @@ -1,7 +1,8 @@ | |||
| [Unit] | |||
| Description=Podman API Service | |||
| Requires=podman.socket | |||
| After=podman.socket | |||
| Wants=network.target | |||
| After=podman.socket network-online.target | |||
There was a problem hiding this comment.
As former systemd developer/maintainer I recommend to not do that. podman.service does not need networking at startup, only for some operations -- but that shouldn't affect the boot order. network-online.target is a rather terrible and brittle hack, see man systemd.special.
Wants=network.target is also unnecessary here. Your unit shouldn't activate that -- at most you'd have After=network.target, but that's not even required as podman does not require the network to shut down cleanly.
Remove the stop timeout from the unit. As unit does not specify any stop command, the timeout is effectively 0 and a NOOP. Signed-off-by: Valentin Rothberg <[email protected]>
Do not set the killmode to process as it only kills the main process and leaves other processes untouched. Just remove the line and use the default cgroup killmode which will kill all processes in the service's cgroup. Fixes: containers#7021 Signed-off-by: Valentin Rothberg <[email protected]>
Do not hard-set the registries.conf to `/etc/containers/registries.conf`. Podman (and other c/image users) already default to it. However, ordinary non-root users should still be able to use the configs in their home directories which is now possible. Signed-off-by: Valentin Rothberg <[email protected]>
podman-api(1) does not exist, so set the man page to podman-system-service(1). Same for the .socket. Signed-off-by: Valentin Rothberg <[email protected]>
Set the type of the podman.service to simple. This will correctly report the status of the service once it has started. As a oneshot service, it does not transition from the startup state to running. Signed-off-by: Valentin Rothberg <[email protected]>
|
Thanks for reviewing and the feedback, @martinpitt. Your comments are addressed in the latest state. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martinpitt, vrothberg 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 |
|
@goochjj PTAL |
|
LGTM |
1 similar comment
|
LGTM |
|
/lgtm |
Don't hardcode /usr/bin/podman in unit files: instead, use template files with a path replaced at install time. Because 'make' can be invoked repeatedly, with different PREFIX, do not leave the generated files behind in our work directory: wipe them immediately after install. To get this to work, fix a longstanding bug in podman.spec.in, a PREFIX that should've been DESTDIR. Side note: containers#7023 made contrib/systemd/user a symlink to .../system but did not update paths in Makefile. The unrelated-looking path change you see here is a belated correction for that. Fixes: containers#10787 Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
This PR includes a series of fixes and clean ups for the systemd service and socket. Please refer to the individual commits for details.
Fixes: #7021