[PUP-3483] Systemd provider doesn't scan for changed units Created: 2014/10/17  Updated: 2019/02/04  Resolved: 2018/11/16

Status: Closed
Project: Puppet
Component/s: Types and Providers
Affects Version/s: None
Fix Version/s: PUP 6.1.0

Type: Bug Priority: Critical
Reporter: Nicholas Jackson Assignee: Melissa Stone
Resolution: Fixed Votes: 25
Labels: community, resolved-issue-added, systemd, type_and_provider
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-9473 Systemd daemon-reload doesn't get tri... Needs Information
relates to PUP-9021 [Resource Type: service][Enhancement]... Closed
Template: PUP Bug Template
Team: Coremunity
Sprint: Platform Core KANBAN
CS Priority: Reviewed
Release Notes: New Feature
Release Notes Summary: When starting or restarting a service, the `systemd` provider checks to see whether the service requires a daemon reload, and reloads the systemd daemon if necessary.
QA Contact: Eric Thompson

 Description   

When a package updates it's sysvinit script or systemd unit (or for that matter if I were to be manually managing a systemd unit with a file resource), nothing ever tells systemd to check for changed units.

Typically you would invoke `systemctl daemon-reload` to scan for new and changed units.



 Comments   
Comment by Ben S [ 2014/12/04 ]

I have a created a module myself that modifies systemd unit files and used execs. I can't see major benefits for Puppet doing the systemctl daemon-reload all the time with the package resource if it detects the service provider as systemd.

  • Packages (should) do this as part of %post scripts
  • Distributions rarely change the init scripts for packages, even after multiple version bumps
  • When these init scripts do change, how often do they affect the service dependency tree? (requiring a daemon-reload)

I think performing a systemctl daemon-reload should be left to modules.

Comment by Kylo Ginsberg [ 2015/04/09 ]

Michael Stahnke thoughts on this?

Comment by Michael Stahnke [ 2015/07/07 ]

I agree with Ben S. Packaging should handle it, or in other cases a specific exec should be called.

Comment by Tom Limoncelli [ 2015/10/08 ]

I'd like to request a reconsideration of closing this. It is now 3 months later and it looks like the recommended solution has an unintentional negative side-effect.

Many modules need this kind of functionality. They all do what was recommended: Create a `Exec['systemd-daemon-reload']` which can be notify'ed any time a unit file is installed or modified. However this creates a new problem:

  1. Many different modules use the same name and collide.
  2. Some modules unique-ify the name, as a result systemd is told to "daemon-reload" multiple times, instead of having all the notify's collapsed into one Exec{}.
  3. Multiple public modules have started defining this exec, with their own unique params. In some cases that definition is wrapped in a conditional statement. As a result, depending on what modules you are using this may be executed in a way you don't suspect.

An example of issue 3 is https://github.com/puppetlabs/puppetlabs-firewall/blob/master/manifests/linux/redhat.pp#L46 . It defines this exec but has an 'unless' parameter on it. Any other module also defining this exec will often put an 'if (!defined' conditional around it, to ensure they don't redefine this commonly named resource. However, any module doing so is going to inadvertently comply to puppetlabs-firewall's 'unless' parameter.

There are probably many solutions but one would be: Puppet Labs should bless a single module that defines the Exec[], so that all other modules can notify it. A module such as https://github.com/camptocamp/puppet-systemd would be a candidate, though I think the name should be different.

Comment by Tim Meusel [ 2015/10/11 ]

I had the same thoughts as Tom, we currently run into several issues because every module defines their own exec. We need a global solution to trigger a systemd daemon-reload, but I'm not sure if a simple module that defines the exec is the best solution (but I also can't think of something better).

Comment by Damon Atkins [ 2015/10/25 ]

What if a person drops a file in /etc/init.d and starts the service with puppet then latter changes it.... it again requires a daemon-reload

Comment by Geoff Williams [ 2016/07/14 ]

I'm hitting this too - In my case I've created the exec resource but am now having to reference it all over the place as the packaging cannot fix the things I need since I'm writing puppet code to update the systemd unit files/environment settings after-the-fact. In addition to this I'm having to write code to special-case windows in order to not attempt to interact with systemd at all.

To me, it would be a much cleaner solution if the puppet service resource could handle any required reloads once and for all so that I don't have to

Comment by Tom Limoncelli [ 2016/07/14 ]

I think the best solution is the ::systemd module from camptocamp:

Any time you modify a unit file (.service) have it notify Exec['systemctl-daemon-reload'].

https://github.com/camptocamp/puppet-systemd

The problem, however, is that modules like puppetlabs-firewall can't be expected to depend on a third-party module like camptocamp/puppet-systemd. The correct solution is for Puppet to make The One True Exec['systemctl-daemon-reload'] and have everyone standardize on it.

Comment by R.I.Pienaar [ 2016/07/26 ]

this same problem basically exist with all the various hassles around managing apt and apt-get update, and there the solution is that everyone uses puppetlabs-apt, I agree that to be a logical approach to handling this too

Comment by Tom Limoncelli [ 2016/07/26 ]

R.I.Pienaar I'm glad there is a precedent. So, if there is a puppetlabs-apt, we need a puppetlabs-systemd?

Comment by R.I.Pienaar [ 2016/07/26 ]

Tom Limoncelli It's an option and it seems sane to me, but I have no idea who will make such a module, an 'official' one would bubble to the top of ones being used I suppose

Comment by Daniel Parks [ 2016/08/25 ]

This should be built into puppet.

The exec thing is a giant pain and a horrible kludge for all the reasons listed above. It's only going to get worse as time goes on.

Running daemon-reload every time Puppet restarts a service is not very heavy: if Puppet runs avery 30 minutes and restarts at least one service, that's only daemon-reloading twice an hour.

If avoiding unnecessary daemon-reloads is really important, then puppet can scan for changed unit files easily enough.

Comment by Tom Limoncelli [ 2016/08/25 ]

(If anyone would like to discuss this at PuppetConf 2016, I'd like to hash it out and get consensus.)

Comment by Hunter (Hunner) Haugen [ 2016/11/30 ]

So, this seems major if not critical, given the systemd adoption rates.

Comment by Kevin Henner [ 2016/12/29 ]

Just to pile on here: from a learning and usability perspective, the "package, file, service" pattern should be as simple as possible, while exec resources are a more advanced topic with a list of warnings and caveats. Exec is the kind of hammer that makes a lot of things look like nails, so the more we can avoid pointing newer users in its direction, the better.

Comment by Thomas Mueller [ 2017/04/19 ]

When a package updates it's sysvinit script or systemd unit

... then the package should take care of reloading. as for example adviced here: https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd

or for that matter if I were to be manually managing a systemd unit with a file resource

then you should use a puppet module that abstracts this for you. like https://forge.puppet.com/camptocamp/systemd

Comment by Tom Limoncelli [ 2017/04/19 ]

Thomas Mueller I think we agree on all points except I feel that https://forge.puppet.com/camptocamp/systemd should be make part of puppet so that everyone uses the same one.

Comment by Hunter (Hunner) Haugen [ 2017/09/07 ]

Using time it appears that checking whether a unit file needs to be reloaded via systemctl show rsyslog --property=NeedDaemonReload takes about 0.007 real and doing a systemctl daemon-reload takes 0.04 real. I did not see any mechanism for making systemd auto-daemon-reload as needed, so perhaps we should check NeedDaemonReload at the beginning of any status/restart/start/stop operation, but only once per ensure evaluation (since the unit should not otherwise change during a single transaction).

I did not see any way to integrate the NeedDaemonReload check as part of systemctl list-unit-files that self.instances performs.

Comment by Vincent Lours [ 2018/07/19 ]

Hi guys,

Based on Hunter (Hunner) Haugen's comment, I was able to implement this functionality in the Puppet Agent code.
Even if I'm not a Ruby Expert, it was quite simple to figure how to implement it.

Including this code in the Puppet Agent code will check if the "NeedDaemonReload" is set to 'yes' or everything else (default: 'no')
Which will cause a "systemctl daemon-reload" only once, when required.

The NeedDaemonReload check will be called on every "start" or "restart" actions.
But the daemon-reload action will occur only once at the first match, as it will fix all other NeedDaemonReload requests once executed.

Including this code in the puppet agent will definitively remove the need of any Notify to a Exec['systemctl-daemon-reload']
for all systemd Services managed in puppet.

The code has been checked/validated with puppet-agent version 5.5.3-1.el7.x86_64.
Feel Free to test and improve it.

Patch version of the updated systemd.rb file

# diff -ruN /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/service/systemd.rb.ori /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/service/systemd.rb
--- /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/service/systemd.rb.ori	2018-07-20 05:21:43.852724847 +0000
+++ /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/service/systemd.rb	2018-07-20 05:37:42.513155599 +0000
@@ -121,6 +121,15 @@
     end
   end
 
+  def daemon_reload?
+    cmd = [command(:systemctl), 'show', @resource[:name], '--property=NeedDaemonReload']
+    daemon_reload = execute(cmd, :failonfail => false).strip.split('=').last
+    if daemon_reload == 'yes'
+      daemon_reload_cmd = [command(:systemctl), 'daemon-reload']
+      execute(daemon_reload_cmd, :failonfail => false).strip
+    end
+  end
+
   def enable
     self.unmask
     systemctl_change_enable(:enable)
@@ -136,11 +145,13 @@
   end
 
   def restartcmd
+    daemon_reload?
     [command(:systemctl), "restart", @resource[:name]]
   end
 
   def startcmd
     self.unmask
+    daemon_reload?
     [command(:systemctl), "start", @resource[:name]]
   end

Comment by Vincent Lours [ 2018/08/01 ]

Hi Eric Thompson,

Can you review on the suggested fix and pass it to the Engineering team if it fits puppet's requirements ?
That should definitively remove the need of any Notify to a Exec['systemctl-daemon-reload'] for all systemd services.

Comment by Eric Thompson [ 2018/08/16 ]

Vincent Lours thanks for your contribution!

if you could create a PR against puppet here: https://github.com/puppetlabs/puppet
that would help.
more info on how to do that is in the https://github.com/puppetlabs/puppet/blob/master/CONTRIBUTING.md doc

Comment by Vincent Lours [ 2018/08/21 ]

Eric Thompson, thanks for your message.
I've made the modifications, but I'm still waiting for the CLA confirmation message (issue CLA-31) to move forward.

I hope to be able to submit the PR soon ^^

Comment by Jeff Silverman [ 2018/10/23 ]

There's a use case, that I didn't see described here, for having puppet automatically do daemon-reload when "service -> enable" is set for a service or when a systemd unit file is added. That is in the case of software installations that are not coming from a package. For example, built or compiled in-house and installed from a tarball. I'm surprised I didn't see this use case listed here. (Or maybe I just missed it – this Jira is long now).

Another, related use case is third party software provided without any init scripts at all. (i.e. I have to write my own init script or systemd unit file). I can't be the only one that has to write init scripts for custom software.

Also, my expectation as a Puppet user is that when I define a Service as enabled in a manifest, that the underlying puppet engine that handles this task knows to do daemon-reload if it needs to. Should that be my expectation? (Reading between the lines here, it sounds like I shouldn't expect that)

Thank you

Comment by Vincent Lours [ 2018/10/23 ]

Hi Jeff Silverman,

As you, I use to create my own systemd unit file.
But from my understanding, the 2 actions listed in your post does not required any daemon-reload.

According the man documentation, the "enable" command reload the systemd configuration after creating the symlink:

enable NAME...
Enable one or more unit files or unit file instances, as specified on the command line. This will create a number of symlinks as encoded in the
"[Install]" sections of the unit files. After the symlinks have been created, the systemd configuration is reloaded (in a way that is equivalent
to daemon-reload) to ensure the changes are taken into account immediately. [...]

And I've made few tests. When a new unit file is created, the systemd is adding it automatically to the config:

Example of creation of new unit file

sh-4.2# cat /etc/centos-release
CentOS Linux release 7.5.1804 (Core)
sh-4.2# ls -l /usr/lib/systemd/system/mytest.service
ls: cannot access /usr/lib/systemd/system/mytest.service: No such file or directory
sh-4.2# systemctl list-unit-files | grep mytest
sh-4.2# vi /usr/lib/systemd/system/mytest.service
sh-4.2# cat /usr/lib/systemd/system/mytest.service
[Unit]
Description=Test Service
After=network.target
 
[Install]
WantedBy=multi-user.target
 
[Service]
Type=oneshot
RemainAfterExit=yes
KillMode=none
ExecStart=/usr/bin/logger -t "test.service" "The Test service has been started"
ExecStop=/usr/bin/logger -t "test.service" "The Test service has been stopped"
sh-4.2# ls -l /usr/lib/systemd/system/mytest.service
-rw-r--r-- 1 root root 308 Oct 23 23:01 /usr/lib/systemd/system/mytest.service
sh-4.2# systemctl list-unit-files | grep mytest
mytest.service                            disabled
sh-4.2# systemctl show mytest --property=NeedDaemonReload
NeedDaemonReload=no
sh-4.2# systemctl start mytest
sh-4.2# systemctl status mytest -l
● mytest.service - Test Service
   Loaded: loaded (/usr/lib/systemd/system/mytest.service; disabled; vendor preset: disabled)
   Active: active (exited) since Tue 2018-10-23 23:02:20 UTC; 5s ago
  Process: 125 ExecStart=/usr/bin/logger -t test.service The Test service has been started (code=exited, status=0/SUCCESS)
 Main PID: 125 (code=exited, status=0/SUCCESS)
 
Oct 23 23:02:20 686235598b69 systemd[1]: Starting Test Service...
Oct 23 23:02:20 686235598b69 test.service[125]: The Test service has been started
Oct 23 23:02:20 686235598b69 systemd[1]: Started Test Service.
sh-4.2# systemctl is-enabled mytest
disabled
sh-4.2# systemctl enable mytest
Created symlink from /etc/systemd/system/multi-user.target.wants/mytest.service to /usr/lib/systemd/system/mytest.service.
sh-4.2# systemctl is-enabled mytest
enabled
sh-4.2# systemctl show mytest --property=NeedDaemonReload
NeedDaemonReload=no
sh-4.2# systemctl status mytest -l
● mytest.service - Test Service
   Loaded: loaded (/usr/lib/systemd/system/mytest.service; enabled; vendor preset: disabled)
   Active: active (exited) since Tue 2018-10-23 23:02:20 UTC; 51s ago
 Main PID: 125 (code=exited, status=0/SUCCESS)
 
Oct 23 23:02:20 686235598b69 systemd[1]: Starting Test Service...
Oct 23 23:02:20 686235598b69 test.service[125]: The Test service has been started
Oct 23 23:02:20 686235598b69 systemd[1]: Started Test Service.
sh-4.2# systemctl restart mytest
sh-4.2# systemctl status mytest -l
● mytest.service - Test Service
   Loaded: loaded (/usr/lib/systemd/system/mytest.service; enabled; vendor preset: disabled)
   Active: active (exited) since Tue 2018-10-23 23:03:26 UTC; 2s ago
  Process: 147 ExecStop=/usr/bin/logger -t test.service The Test service has been stopped (code=exited, status=0/SUCCESS)
  Process: 148 ExecStart=/usr/bin/logger -t test.service The Test service has been started (code=exited, status=0/SUCCESS)
 Main PID: 148 (code=exited, status=0/SUCCESS)
 
Oct 23 23:03:26 686235598b69 systemd[1]: Starting Test Service...
Oct 23 23:03:26 686235598b69 systemd[1]: Started Test Service.

Anyway, I've submitted a Pull Request to include (if required) automatic daemon-reload on start & restart actions from puppet systemd provider.
That should avoid a lot of daemon-reload exec.
Feel free to check the code.

Cheers

Comment by Scott Garman [ 2018/11/08 ]

Merged to puppet master at https://github.com/puppetlabs/puppet/commit/d4297af039b64a1fb0a05cced25a13557bcf37d4

Comment by Melissa Stone [ 2018/11/16 ]

This has passed CI as a part of puppet-agent 6.0.4.172.g8835ced

Generated at Sat Aug 24 21:44:22 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.