voxpupuli / puppet-systemd

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

fix: refresh service only based on drop-in file changes #406

Closed shieldwed closed 4 months ago

shieldwed commented 8 months ago

Pull Request (PR) description

Since multiple drop-in files of the same unit notify the same Systemd::Daemon_reload resource, changing a drop in file without notify_service still triggers a service refresh if at least one other drop-in file has notify_service set (default) regardless of it being changed.

Therefore, the Systemd::Daemon_reload should only be ordered before the Service but not notify it. The service will already be refreshed if the drop-in file changes and notify_service is set (default).

This Pull Request (PR) addresses the following issues

Consider the following Puppet code snippet with two drop-in files for the service nginx.service:

  systemd::dropin_file {
    default:
      unit => 'nginx.service',
      ;
    "restart.conf":
      notify_service => false,
      content        => @(EOT)
        [Service]
        Restart=on-failure
        | EOT
      ,
    ;
    "ulimit.conf":
      content => @(EOT)
        [Service]
        LimitNOFILE=4096
        | EOT
      ,
  }

On the first Puppet-run, this code will correctly refresh the service, as the ulimit.conf-file will be written. Once the drop-in files are in sync with this configuration, no more SystemD daemon reloads and service refreshes happen.

Now, changing the Restart configuration of nginx.service doesn't need the service itself to be restarted.

However, due to the second drop-in file specifying notify_service => true (default), this will lead to Systemd::Daemon_reload[$unit] being triggered (expected due to daemon_reload => true (default)), which then in turn triggers Service[nginx.service] and Service[nginx] if they exist.

shieldwed commented 8 months ago

I am quite uncertain on how to test these changes. Especially since tests for absence of certain relationships seem to be asked for in this case.

shieldwed commented 7 months ago

This probably impossible to test due to rspec-puppet having different transitive dependencies than real Puppet ...

I've implemented some tests for the changed cases. Can you please look at them again?

shieldwed commented 6 months ago

@ekohl can you please have another look at this PR? Thank you

shieldwed commented 4 months ago

Can someone please have a look at this again (e.g. @bastelfreak or @TheMeier perhaps)? Thank you.