Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix systemctl daemon-reload after file additions #277

Merged
merged 8 commits into from
Jun 17, 2022
Merged

Fix systemctl daemon-reload after file additions #277

merged 8 commits into from
Jun 17, 2022

Conversation

trevor-vaughan
Copy link
Contributor

@trevor-vaughan trevor-vaughan commented Jun 10, 2022 •
edited
Loading

  • 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

Fixes #234
Fixes #199

@trevor-vaughan
Fix systemctl daemon-reload after file additions
fbcbbc4 
* 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
@trevor-vaughan
Copy link
Contributor Author

@op-ct For reference. Fixes the issues with simp/pupmod-simp-gdm/pull/77.

@trevor-vaughan trevor-vaughan added the bug Something isn't working label Jun 10, 2022
@trevor-vaughan trevor-vaughan mentioned this pull request Jun 10, 2022
Closed
@trevor-vaughan trevor-vaughan self-assigned this Jun 10, 2022
ekohl
ekohl reviewed Jun 10, 2022
View reviewed changes
manifests/dropin_file.pp Outdated Show resolved Hide resolved
ekohl
ekohl reviewed Jun 10, 2022
View reviewed changes
manifests/daemon_reload.pp Outdated Show resolved Hide resolved
@trevor-vaughan trevor-vaughan mentioned this pull request Jun 11, 2022
Merged
@trevor-vaughan
Copy link
Contributor Author

@ekohl and/or @alexjfisher Are you all OK with this?

ekohl
ekohl reviewed Jun 17, 2022
View reviewed changes
manifests/init.pp Outdated Show resolved Hide resolved
@trevor-vaughan
Copy link
Contributor Author

@ekohl I removed the lazy reload and am creating a ticket for follow-on work to fix it properly.

@trevor-vaughan trevor-vaughan mentioned this pull request Jun 17, 2022
Closed
@trevor-vaughan trevor-vaughan mentioned this pull request Jun 17, 2022
Merged
@trevor-vaughan
Copy link
Contributor Author

Proper management of NFS also needs this fix.

ekohl
ekohl reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits. We do have a few more redundant daemon-reload calls now which does worry me a bit. Puppet 6.1.0 and higher should automatically call systemctl daemon-reload if needed on a managed service. Do you have drop in files for unmanaged services that you run into this or is the deamon-reload code in Puppet broken? (I did run into theforeman/puppet-puppet#832 but that was only on Ubuntu 20.04).

I think I can live with some redundant daemon-reloads but there are a few small code suggestions inline that I'd like to see applied.

manifests/daemon_reload.pp Outdated Show resolved Hide resolved
manifests/timer.pp Show resolved Hide resolved
spec/classes/init_spec.rb Outdated Show resolved Hide resolved
spec/defines/daemon_reload.rb Outdated Show resolved Hide resolved
trevor-vaughan and others added 3 commits June 17, 2022 16:40
@trevor-vaughan @ekohl
Update manifests/daemon_reload.pp
2c58e0d 
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
@trevor-vaughan @ekohl
Update spec/classes/init_spec.rb
0cdb420 
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
@trevor-vaughan @ekohl
Update spec/defines/daemon_reload.rb
643d1a1 
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
@trevor-vaughan trevor-vaughan mentioned this pull request Jun 17, 2022
Closed
ekohl
ekohl approved these changes Jun 17, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed partially on IRC (captured in #284) this looks good to me. I'd label this an enhancement since IMHO it deserves a minor version bump rather than just a patch bump.

@ekohl
Copy link
Member

ekohl commented Jun 17, 2022

Oh, and can we squash this? Can be done on merge.

@alexjfisher any last thoughts?

@ekohl ekohl added enhancement New feature or request and removed bug Something isn't working labels Jun 17, 2022
@trevor-vaughan trevor-vaughan merged commit 2ed469f into master Jun 17, 2022
@trevor-vaughan trevor-vaughan deleted the daemon-reload branch June 17, 2022 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel left review comments

@ekohl ekohl ekohl approved these changes

@alexjfisher alexjfisher Awaiting requested review from alexjfisher

Assignees

@trevor-vaughan trevor-vaughan

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

systemd::dropin_file doesn't cause a systemd daemon-reload
4 participants
@trevor-vaughan @ekohl @alexjfisher @simondeziel