voxpupuli / puppet-systemd

Puppet module to manage systemd
https://forge.puppet.com/puppet/systemd
Apache License 2.0
54 stars 142 forks source link

Support reload of `systemd --user` for non-zero UIDs #416

Closed traylenator closed 6 months ago

traylenator commented 7 months ago

Pull Request (PR) description

This change increased the minimum required Puppet version to 6.24.0 or 7.9.0 PUP-5704 to support arrays to the command attribute of the exec type.

Support calling the systemd --user for a particular uid.

A uid must be used rather than the probably more convenient username since the $XDG_RUNTIME_DIR for the user must be known.

This change assumes that XDG_RUNTIME_DIR is /run/user/<uid> for all uids.

Example run:

notify{'junk':
 notify => Systemd::Daemon_reload['foobar'],
}

systemd::daemon_reload{'foobar':
  uid => 1000,
}

results in

Notice: Compiled catalog for fedora. in environment production in 0.05 seconds
Notice: junk
Notice: /Stage[main]/Main/Notify[junk]/message: defined 'message' as 'junk'
Notice: /Stage[main]/Main/Systemd::Daemon_reload[foobar]/Exec[systemd-foobar-systemctl-user-1000-daemon-reload]: Triggered 'refresh' from 1 event
Notice: Applied catalog in 0.17 seconds

and a journal (debug on) of


Feb 28 11:44:34 fedora systemd[1]: user@1000.service: Got notification message from PID 1877 (RELOADING=1, MONOTONIC_USEC=5874581103)
Feb 28 11:44:34 fedora systemd[1]: user@1000.service: Changed running -> reload-notify
Feb 28 11:44:34 fedora systemd[1]: user@1000.service: Installed new job user@1000.service/nop as 6151
...
traylenator commented 7 months ago

It is going to be a pain passing in the uid since you often do not know it.

Open to alternatives?

Having a fact that provides details on the running systemd --users seems bad with terrible race conditions.

bastelfreak commented 7 months ago

@traylenator are you fine with this for now or should we wait for a solution to get the UID?

ekohl commented 7 months ago

It is going to be a pain passing in the uid since you often do not know it.

Would a deferred function to resolve a username to uid be possible?

traylenator commented 7 months ago

@traylenator are you fine with this for now or should we wait for a solution to get the UID?

I am fine with this as is. It unlocks quite a few things for Podman in particular and if we can support a user attribute all of this is still correct.

traylenator commented 7 months ago

It is going to be a pain passing in the uid since you often do not know it.

Would a deferred function to resolve a username to uid be possible?

You can certainly do a deferred inline template for the exec command - less sure about the user parameter. Will have a play after this.

A getpwnam is a very generic function so would probably add it to extlib anyway.

traylenator commented 7 months ago

I don't mind the array, but https://www.puppet.com/docs/puppet/7/types/exec.html#exec-attribute-command should really document that it was PUP-5704 that implemented it and you need at least Puppet 6.24.0 or Puppet 7.9.0.

Because of that we should raise the minimum version in metadata.json.

min puppet version bumped.

traylenator commented 7 months ago

getpwnam is a very generic function so would probably add it to extlib anyway.

Wrong function:

It would be https://github.com/wcooley/puppet-name_service_lookups/blob/master/lib%2Fpuppet%2Fparser%2Ffunctions%2Fgetpwuid.rb

of course.

bastelfreak commented 7 months ago

@traylenator oh having those in extlib would be awesome! (and maybe be converted to the modern API :eyes: )

ekohl commented 7 months ago

Almost feels generic enough to belong in stdlib.

traylenator commented 6 months ago

Rebased - since we crossed the backwards incompatible threshold - would be good.

bastelfreak commented 6 months ago

the code looks fine to me. But is it actually a backwards-incompatible change?

traylenator commented 6 months ago

the code looks fine to me. But is it actually a backwards-incompatible change?

Only for exec.command = Array , so increase in puppet version.

traylenator commented 6 months ago

I made the mistake at looking at this again:

machinectl shell mycentos@.host /usr/bin/systemctl --user daemon-reload

ran as root avoids the need to know the UID and assuming where things are.

traylenator commented 6 months ago

443 is a much better solution and avoids all the user -> uid hassles.

traylenator commented 6 months ago

Thanks for all the reviews - I've hopefully taken over all the comments to the new one.