daemon reload problem with 3.0.0

[Rewerson]

After updating to 3.0.0 limits configuration seems to be broken.
In the manifest I have:

systemd::service_limits { 'php7.4-fpm.service':
  limits => {
    'LimitNOFILE' => 1048576,
  }
}

Puppet applied this code and tried to perform service restart:
Exec[restart php7.4-fpm.service because limits]

But in fact, new limits were not applied, because of lack of daemon reload. By code I see there is call of:
systemctl restart php7.4-fpm.service

By running it by hand after puppet run, I got:

~# systemctl restart php7.4-fpm.service
Warning: The unit file, source configuration file or drop-ins of php7.4-fpm.service changed on disk. Run 'systemctl daemon-reload' to reload units.

So, I need to reload daemon manually, and then restart the service.

OS Debian 10.9
Puppet server 7.1.0
Puppet agent 7.5.0

[ekohl]

I would advise you to never use $restart_service => true on systemd::service_limits since it, as you noticed, doesn't do any correct ordering. If you also manage the service itself, you may end up with double service restarts as well.

What I always end up doing is roughly:

systemd::service_limits { 'php7.4-fpm.service':
  limits          => {
    'LimitNOFILE' => 1048576,
  }
  restart_service => false,
  notify          => Service['php7.4-fpm.service'],
}

service { 'php7.4-fpm.service':
  ensure => running,
  enable => true,
}

IMHO it should default to false. However, sadly there was just a major release and this wasn't considered.

Also, it would be great if dropin has an auto collector. I took a stab at this in #191.

[Rewerson]

Wrong ordering and double reloads are less evil than no applying changes to service at all.
As said in docs for 3.0.0:

Typically this works well and removes the need for systemd::systemctl::daemon_reload as provided prior to camptocamp/systemd 3.0.0.

So, my case shows that mentioned typically is not applicable. My code worked well on 2.x branch.

[ekohl]

Looks like we never finished the discussion: #171 (comment)

[Rewerson]

Unfortunately, that commit was already merged. And now I think it caused strange behavior of module. Maybe, maintainer could say more, because on paper it really should work from the side of puppet itself on 6.1.0 and later, but it's not.

[ekohl]

Well, the thing is that it doesn't call the provider but just a simple exec. That's why I never use it. IMHO it should be removed or at least default to false.

[johanfleury]

So I’m facing the same issue with systemd::timer where daemon-reload is not called when the service definition changes.

# puppet agent -t --environment production
Info: Using configured environment 'production'
Info: Retrieving pluginfacts
Info: Retrieving plugin
Info: Loading facts
Info: Caching catalog for [SNIPPED]
Info: Applying configuration version '1622498808'
[SNIPPED]
Notice: /Stage[main]/Borg::Service/Systemd::Timer[borgmatic.timer]/Systemd::Unit_file[borgmatic.service]/File[/etc/systemd/system/borgmatic.service]/content:
--- /etc/systemd/system/borgmatic.service       2021-05-30 04:19:31.089365703 +0000
+++ /tmp/puppet-file20210531-5640-pz2ss 2021-05-31 22:07:04.408630248 +0000
[SNIPPED]

Notice: /Stage[main]/Borg::Service/Systemd::Timer[borgmatic.timer]/Systemd::Unit_file[borgmatic.service]/File[/etc/systemd/system/borgmatic.service]/content: content changed '{sha256}ddf42ac6fb4bbfd1b74a0cee34a74
a1f5f0aa96e81c3073c4c347ca55e9cc59c' to '{sha256}cbae81dba4beea3415750275e16a57ba15769d6cf2c736de24e6c49ae270e4c0'
Notice: Applied catalog in 10.35 seconds

# systemctl show --property=NeedDaemonReload borgmatic.service
NeedDaemonReload=yes

[ekohl]

It sounds like systemd::timer should be modified to perform that reload if needed. There you can indeed see that there is no service type managing the actual borgomatic.service service so no code to perform the daemon-reload.