Fix systemctl daemon-reload after file additions

* Add a `systemd::daemon_reload` defined type
* Ensure that any file additions to the /etc/systemd/system space are
  followed by a call to `systemd::daemon_reload`
* Allow users to disable the calls to `systemd::daemon_reload`
* Allow users to globally disable the `systemctl daemon-reload` exec
  using a resource collector if necessary
* Hook the daemon reload between the file creation and the service as is
  usually necessary, where possible
* Add a 'best effort' optional exec as `systemd::lazy_daemon_reload` to
  try and clean up systems that were modified betweedn 2.9.0 and this
  release

Fixes #234
Fixes #199

manifests/daemon_reload.pp

@@ -0,0 +1,42 @@
+# @summary Run systemctl daemon-reload
+#
+# @api public
+#
+# @param name
+#   A globally unique name for the resource
+#
+# @param enable
+#   Enable the reload exec
+#
+#   * Added in case users want to disable the reload globally using a resource collector
+#
+# @param lazy_reload
+#   Enable a global lazy reload.
+#
+#   * This is meant for cleaning up systems that may have gotten out of sync so no particular
+#     care is taken in trying to actually "fix" things, that would require a custom type that
+#     manipulates the running catalog tree.
+#
+define systemd::daemon_reload (
+  Boolean $enable      = true,
+  Boolean $lazy_reload = false,
+) {
+  if $enable {
+    if $lazy_reload {
+      exec { "${module_name}-${name}-global-systemctl-daemon-check":
+        command => 'systemctl daemon-reload',
+        onlyif  => 'systemctl show "*" --property=NeedDaemonReload | grep -q "=yes"',
+        path    => $facts['path'],
+      }
+
+      # Give services a fighting chance of coming up properly
+      Exec["${module_name}-${name}-global-systemctl-daemon-check"] -> Service <||>
+    }
+
+    exec { "${module_name}-${name}-systemctl-daemon-reload":
+      command     => 'systemctl daemon-reload',
+      refreshonly => true,
+      path        => $facts['path'],
+    }
+  }
+}

manifests/dropin_file.pp

@@ -17,6 +17,7 @@
 # @param mode The mode to set on the dropin file
 # @param show_diff Whether to show the diff when updating dropin file
 # @param notify_service Notify a service for the unit, if it exists
+# @param daemon_reload Call systemd::daemon_reload
 #
 define systemd::dropin_file (
   Systemd::Unit                               $unit,
@@ -32,6 +33,7 @@
   String                                      $mode                    = '0444',
   Boolean                                     $show_diff               = true,
   Boolean                                     $notify_service          = false,
+  Boolean                                     $daemon_reload           = true,
 ) {
   include systemd
 
@@ -69,11 +71,26 @@
     show_diff               => $show_diff,
   }
 
+  if $daemon_reload {
+    ensure_resource('systemd::daemon_reload', $name)
+
+    File[$full_filename] ~> Systemd::Daemon_reload[$name]
+  }
+
   if $notify_service {
     File[$full_filename] ~> Service <| title == $unit or name == $unit |>
+
+    if $daemon_reload {
+      Systemd::Daemon_reload[$name] ~> Service <| title == $unit or name == $unit |>
+    }
+
     if $unit =~ /\.service$/ {
       $short_service_name = regsubst($unit, /\.service$/, '')
       File[$full_filename] ~> Service <| title == $short_service_name or name == $short_service_name |>
+
+      if $daemon_reload {
+        Systemd::Daemon_reload[$name] ~> Service <| title == $short_service_name or name == $short_service_name |>
+      }
     }
   }
 }

manifests/init.pp

@@ -174,6 +174,9 @@
 # @param oomd_settings
 #   Hash of systemd-oomd configurations for oomd.conf
 #
+# @param lazy_daemon_reload
+#   Perform a check across all potentially overridden service files and trigger a daemon reload if necessary
+#
 class systemd (
   Optional[Pattern['^.+\.target$']]                   $default_target = undef,
   Hash[String,String]                                 $accounting = {},
@@ -227,6 +230,7 @@
   Boolean                                             $manage_oomd = false,
   Enum['stopped','running']                           $oomd_ensure = 'running',
   Systemd::OomdSettings                               $oomd_settings = {},
+  Boolean                                             $lazy_daemon_reload = false,
 ) {
   contain systemd::install
 
@@ -316,4 +320,8 @@
       * => $resource,
     }
   }
+
+  if $lazy_daemon_reload {
+    systemd::daemon_reload { 'global-lazy': lazy_reload => $lazy_daemon_reload }
+  }
 }

manifests/service_limits.pp

@@ -74,7 +74,8 @@
       command     => "systemctl restart ${name}",
       path        => $facts['path'],
       refreshonly => true,
-      subscribe   => File["${path}/${name}.d/90-limits.conf"],
     }
+
+    Systemd::Dropin_file["${name}-90-limits.conf"] ~> Exec["restart ${name} because limits"]
   }
 }

manifests/timer.pp

@@ -58,6 +58,9 @@
 # @param ensure
 #   Defines the desired state of the timer
 #
+# @param daemon_reload
+#   Call `systemd::daemon_reload`
+#
 define systemd::timer (
   Enum['present', 'absent', 'file']        $ensure = 'present',
   Stdlib::Absolutepath                     $path = '/etc/systemd/system',
@@ -72,6 +75,7 @@
   Boolean                                  $show_diff = true,
   Optional[Variant[Boolean, Enum['mask']]] $enable = undef,
   Optional[Boolean]                        $active = undef,
+  Boolean                                  $daemon_reload = true,
 ) {
   assert_type(Pattern['^.+\.timer$'],$name)
 
@@ -83,28 +87,30 @@
 
   if $service_content or $service_source {
     systemd::unit_file { $_service_unit:
-      ensure    => $ensure,
-      content   => $service_content,
-      source    => $service_source,
-      path      => $path,
-      owner     => $owner,
-      group     => $group,
-      mode      => $mode,
-      show_diff => $show_diff,
-      before    => Systemd::Unit_File[$name],
+      ensure        => $ensure,
+      content       => $service_content,
+      source        => $service_source,
+      path          => $path,
+      owner         => $owner,
+      group         => $group,
+      mode          => $mode,
+      show_diff     => $show_diff,
+      before        => Systemd::Unit_File[$name],
+      daemon_reload => $daemon_reload,
     }
   }
 
   systemd::unit_file { $name:
-    ensure    => $ensure,
-    content   => $timer_content,
-    source    => $timer_source,
-    path      => $path,
-    owner     => $owner,
-    group     => $group,
-    mode      => $mode,
-    show_diff => $show_diff,
-    enable    => $enable,
-    active    => $active,
+    ensure        => $ensure,
+    content       => $timer_content,
+    source        => $timer_source,
+    path          => $path,
+    owner         => $owner,
+    group         => $group,
+    mode          => $mode,
+    show_diff     => $show_diff,
+    enable        => $enable,
+    active        => $active,
+    daemon_reload => $daemon_reload,
   }
 }

manifests/unit_file.pp

@@ -61,6 +61,9 @@
 # @param service_parameters
 #   hash that will be passed with the splat operator to the service resource
 #
+# @param daemon_reload
+#   call `systemd::daemon-reload` to ensure that the modified unit file is loaded
+#
 # @example manage unit file + service
 #   systemd::unit_file { 'foo.service':
 #     content => file("${module_name}/foo.service"),
@@ -85,6 +88,7 @@
   Optional[Boolean]                        $hasstatus = undef,
   Boolean                                  $selinux_ignore_defaults = false,
   Hash[String[1], Any]                     $service_parameters = {},
+  Boolean                                  $daemon_reload = true
 ) {
   include systemd
 
@@ -125,6 +129,12 @@
     selinux_ignore_defaults => $selinux_ignore_defaults,
   }
 
+  if $daemon_reload {
+    ensure_resource('systemd::daemon_reload', $name)
+
+    File["${path}/${name}"] ~> Systemd::Daemon_reload[$name]
+  }
+
   if $enable != undef or $active != undef {
     service { $name:
       ensure     => $active,
@@ -143,15 +153,10 @@
       Service[$name] -> File["${path}/${name}"]
     } else {
       File["${path}/${name}"] ~> Service[$name]
-    }
-  } else {
-    # Work around https://tickets.puppetlabs.com/browse/PUP-9473
-    # and react to changes on static unit files (ie: .service triggered by .timer)
-    exec { "${name}-systemctl-daemon-reload":
-      command     => 'systemctl daemon-reload',
-      refreshonly => true,
-      path        => $facts['path'],
-      subscribe   => File["${path}/${name}"],
+
+      if $daemon_reload {
+        Systemd::Daemon_reload[$name] ~> Service[$name]
+      }
     }
   }
 }

spec/classes/init_spec.rb

@@ -21,6 +21,7 @@
         it { is_expected.not_to contain_class('systemd::coredump') }
         it { is_expected.not_to contain_class('systemd::oomd') }
         it { is_expected.not_to contain_exec('systemctl set-default multi-user.target') }
+        it { is_expected.not_to contain_systemd__daemon_reload('global-lazy') }
 
         context 'when enabling resolved and networkd' do
           let(:params) do
@@ -750,6 +751,14 @@
             it { is_expected.to contain_systemd__dropin_file('coredump_backtrace.conf').with_content(%r{^ExecStart=.*--backtrace$}) }
           end
         end
+
+        context 'with lazy daemon reloadin' do
+          let :params do
+            { lazy_daemon_reload: true }
+          end
+
+          it { is_expected.to contain_systemd__daemon_reload('global-lazy').with_lazy_reload(params[:lazy_daemon_reload]) }
+        end
       end
     end
   end

spec/defines/daemon_reload.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'systemd::daemon_reload' do
+  context 'supported operating systems' do
+    on_supported_os.each do |os, facts|
+      context "on #{os}" do
+        let(:facts) { facts }
+        let(:title) { 'irregardless' }
+
+        it { is_expected.to compile.with_all_deps }
+
+        context 'with defaults' do
+          it do
+            expect(subject).to contain_exec("systemd-#{title}-systemctl-daemon-reload").
+              with_command('systemctl daemon-reload').
+              with_refreshonly(true)
+
+            expect(subject).not_to contain_exec("systemd-#{title}-global-systemctl-daemon-check")
+          end
+        end
+
+        context 'when disabled' do
+          let(:params) do
+            { 'enable' => false }
+          end
+
+          it do
+            expect(subject).not_to contain_exec("systemd-#{title}-systemctl-daemon-reload")
+          end
+        end
+
+        context 'with lazy reloading' do
+          let(:pre_condition) { 'service { "test": }' }
+          let(:params) do
+            { 'lazy_reload' => true }
+          end
+
+          it do
+            expect(subject).to contain_exec("systemd-#{title}-systemctl-daemon-reload").
+              with_command('systemctl daemon-reload').
+              with_refreshonly(true)
+
+            expect(subject).to contain_exec("systemd-#{title}-global-systemctl-daemon-check").
+              with_command('systemctl daemon-reload').
+              with_onlyif('systemctl show "*" --property=NeedDaemonReload | grep -q "=yes"').
+              that_comes_before('Service[test]')
+          end
+        end
+      end
+    end
+  end
+end

spec/defines/dropin_file_spec.rb

@@ -28,13 +28,18 @@
           )
         }
 
+        it {
+          expect(subject).to create_systemd__daemon_reload(title)
+        }
+
         it {
           expect(subject).to create_file("/etc/systemd/system/#{params[:unit]}.d/#{title}").with(
             ensure: 'file',
             content: %r{#{params[:content]}},
             mode: '0444',
             selinux_ignore_defaults: false
-          )
+          ).
+            that_notifies("Systemd::Daemon_reload[#{title}]")
         }
 
         context 'notifies services' do
@@ -64,6 +69,7 @@
 
             it { is_expected.to compile.with_all_deps }
             it { is_expected.to contain_service('myservice').that_subscribes_to("File[#{filename}]") }
+            it { is_expected.to contain_systemd__daemon_reload(title).that_notifies('Service[myservice]') }
           end
         end
 
@@ -143,6 +149,18 @@
             )
           }
         end
+
+        context 'with daemon_reload = false' do
+          let(:params) do
+            super().merge(daemon_reload: false)
+          end
+
+          it { is_expected.to compile.with_all_deps }
+
+          it {
+            expect(subject).not_to create_systemd__daemon_reload(title)
+          }
+        end
       end
     end
   end

spec/defines/timer_spec.rb

@@ -84,13 +84,6 @@
 
           it { is_expected.to contain_systemd__unit_file('foobar.timer').with_content("[Timer]\nOnCalendar=hourly") }
           it { is_expected.to contain_systemd__unit_file('foobar.service').with_content("[Service]\nExecStart=/bin/echo timer-fired").that_comes_before('Systemd::Unit_file[foobar.timer]') }
-
-          it {
-            expect(subject).to create_exec('foobar.service-systemctl-daemon-reload').with(
-              command: 'systemctl daemon-reload',
-              refreshonly: true
-            )
-          }
         end
 
         context 'with a bad timer name' do