Dropin daemon reload #237
Dropin daemon reload #237
Conversation
Whenever a dropin snippet is added or removed, systemd needs to be informed to assemble the full unit file. If for some odd reason, someone manages both a unit (systemd::unit_file) and add a dropin (systemd::dropin_file), `systemctl daemon-reload` will be called twice. This is useless but also harmless. Ideally, one would simply fold all settings into the base unit and forgo the dropin. Fixes voxpupuli#234 Signed-off-by: Simon Deziel <simon@sdeziel.info>
Signed-off-by: Simon Deziel <simon@sdeziel.info>
|
I'm not sure this is correct. Puppet should already reload services that are managed. In #234 (comment) I asked for some more info. |
Which it does but the bug is for services that only dropin files are managed. I use those all the time to add a delta on top of what the distro/packager provided for example. |
| @@ -69,11 +69,20 @@ | |||
| show_diff => $show_diff, | |||
| } | |||
|
|
|||
| exec { "${name}-dropin-systemctl-daemon-reload": | |||
| command => 'systemctl daemon-reload', | |||
| refreshonly => true, | |||
There was a problem hiding this comment.
Rather than refreshonly I wonder if onlyif => 'systemctl show -p NeedDaemonReload | grep yes' would work.
There was a problem hiding this comment.
Seems like a good idea 🤔
There was a problem hiding this comment.
Needs the unit name in there of course.
onlyif => 'systemctl show -p NeedDaemonReload $unit | grep yes'
The downside is this being run potentially many times per unit per puppet run always but the simplicity wins for me.
systemd::unit_file would need the same code for an unmanned .service and .timer so would be nice to really only have one exec per unit. As mentioned else where - defined type and ensure_resource
There was a problem hiding this comment.
The onlyif incurs overhead for every run. Probably a personal preference, but refreshonly coupled with subscribe feels simple and self descriptive to me.
There was a problem hiding this comment.
The onlyif => "systemctl show -p NeedDaemonReload $unit | grep yes" way makes sense if for some reason a user has both a systemd::unit_file and a systemd::dropin_file for the same service.
I consider this to be an edge case because if you deploy the unit, why bother with a dropin? However, if you do need this unusual setup, it will work but systemctl daemon-reload will be called twice. The overhead of that second extraneous call that happens only on changes is probably not too far off from the NeedDaemonReload test it's avoiding for every single runs.
|
In #234 (comment) I suggested to use a define to avoid multiple reloads for the same service, in case multiple drop ins are used. |
|
I'm going to see if we can get this (or something very like it) through. This is a use case that has prevented us from upgrading since the change was put in place originally:
|
Sorry if that's not related to the core issue but I've been wanting to go back to |
|
@simondeziel See https://github.com/simp/pupmod-simp-gdm. Also, are you up for getting this changed now or should I refactor into another PR? At a minimum, we need to get the |
|
@simondeziel I started down the path to getting everything fixed, will cross-link a new PR. |
|
@simondeziel Can you try this? https://github.com/voxpupuli/puppet-systemd/tree/daemon-reload It works for me, but I'm working on a section to fix all of the systems that got broken by the current 3.X branch ( |
|
@trevor-vaughan the |
|
Closing in favor of #277 per the conversation above |
Release 5.3.1
Fixes #234