-
-
Notifications
You must be signed in to change notification settings - Fork 143
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.
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
Drop Puppet 4 and 5 support + daemon-reload code #171
Conversation
@@ -75,7 +75,6 @@ | |||
path => $::path, | |||
refreshonly => true, | |||
subscribe => File["${path}/${name}.d/90-limits.conf"], | |||
require => Class['systemd::systemctl::daemon_reload'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unsure about this (which is also why this is a draft). This isn't a service type so it doesn't perform the reload if needed. Not sure how to deal with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raphink any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks likes it should stay then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the class to require is gone, should there be a local daemon reload?
b683806
to
954cd0a
Compare
Rebased since the included PR was merged so the diff is smaller. No actual change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have since found out that there are edge cases where you do need daemon-reload. Not sure if we want account for that. I can see about writing up something in the README.
Alternative to #179 |
This picks version 6.1.0 as a new lower bound since that contains code to automatically run daemon-reload if needed. Versions 4 and 5 are EOL.
954cd0a
to
7062aa9
Compare
Just a trivial rebase now to see if the tests are green, no actual code change. |
Since Puppet 6.1.0 it's no longer needed to run daemon-reload manually when restarting a service. That means it's possible to drop this code.
7062aa9
to
a76a4b6
Compare
I added a section to the README but then I realized I hadn't addressed the service limits case. Can anyone take a look if this is a good direction or would you prefer to solve it with code? |
@ekohl Since it only occurs during a removal, I'd much rather have it handled in code if possible. Without that, we'll all end up with a bunch of confused end users at some point. When the upstream code is corrected, a new version can be released that removes the workaround. |
It happens in an odd edge case. The following code doesn't suffer from it: systemd::unit_file { 'myservice.service':
ensure => absent,
active => true,
} That's because the module considers that an invalid state. This does: systemd::unit_file { 'myservice.service':
ensure => absent,
}
service { 'myservice':
ensure => running,
require => Systemd::Unit_file['myservice.service'],
} What happened in puppetlabs-postgresql was exactly that. It switched from a module provided unit file to a package provided unit file. I'm pushing the update as a separate commit for easier review. Let me know what you think. |
Prior to this commit, the follow code did not suffer from PUP-9473: systemd::unit_file { 'myservice.service': ensure => absent, active => true, } That's because the module considers that an invalid state and fails to compile. The follow code did trigger the bug: systemd::unit_file { 'myservice.service': ensure => absent, } service { 'myservice': ensure => running, require => Systemd::Unit_file['myservice.service'], } That's precisely what happens when a module switches from a module-provided unit file to a package-provided unit file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️ 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Puppet 5 support was dropped in voxpupuli#171, so no need to test with it anymore.
This is not needed anymore since Puppet 6. voxpupuli/puppet-systemd#171
…stemd module, because the class was deprecated by the change voxpupuli/puppet-systemd#171
This is apparently not needed anymore on Puppet 6. voxpupuli/puppet-systemd#171
* Drop Puppet 4 and 5 support This picks version 6.1.0 as a new lower bound since that contains code to automatically run daemon-reload if needed. Versions 4 and 5 are EOL. * Drop daemon-reload code Since Puppet 6.1.0 it's no longer needed to run daemon-reload manually when restarting a service. That means it's possible to drop this code. * Implement a workaround for PUP-9473. Prior to this commit, the follow code did not suffer from PUP-9473: systemd::unit_file { 'myservice.service': ensure => absent, active => true, } That's because the module considers that an invalid state and fails to compile. The follow code did trigger the bug: systemd::unit_file { 'myservice.service': ensure => absent, } service { 'myservice': ensure => running, require => Systemd::Unit_file['myservice.service'], } That's precisely what happens when a module switches from a module-provided unit file to a package-provided unit file.
… testing (voxpupuli#183) * metadata: allow Puppet 7 * .travis.yml: remove Puppet 5, add Puppet 7 Puppet 5 support was dropped in voxpupuli#171, so no need to test with it anymore.
* Drop Puppet 4 and 5 support This picks version 6.1.0 as a new lower bound since that contains code to automatically run daemon-reload if needed. Versions 4 and 5 are EOL. * Drop daemon-reload code Since Puppet 6.1.0 it's no longer needed to run daemon-reload manually when restarting a service. That means it's possible to drop this code. * Implement a workaround for PUP-9473. Prior to this commit, the follow code did not suffer from PUP-9473: systemd::unit_file { 'myservice.service': ensure => absent, active => true, } That's because the module considers that an invalid state and fails to compile. The follow code did trigger the bug: systemd::unit_file { 'myservice.service': ensure => absent, } service { 'myservice': ensure => running, require => Systemd::Unit_file['myservice.service'], } That's precisely what happens when a module switches from a module-provided unit file to a package-provided unit file.
… testing (voxpupuli#183) * metadata: allow Puppet 7 * .travis.yml: remove Puppet 5, add Puppet 7 Puppet 5 support was dropped in voxpupuli#171, so no need to test with it anymore.
`daemon_reload` no longer needed as of Puppet 6.1.0, and it was dropped in `puppet/camptocamp` 3.0[1] [1]: voxpupuli/puppet-systemd#171
This patch updates from camptocamp/systemd 2.x to puppet/systemd 3.x This bump is driven by simp/simp-core#829 `daemon_reload` no longer needed as of Puppet 6.1.0, and it was dropped in `puppet/camptocamp` 3.0[1] [1]: voxpupuli/puppet-systemd#171
Remove nested code blocks from string docs
…le 'systemd' (#13542) Do so, in particular, to support the migration from the deprecated module 'camptocamp/systemd' to the maintained module 'puppet/systemd'. References: • voxpupuli/puppet-systemd#171 • https://github.com/voxpupuli/puppet-systemd/blob/master/README.md#daemon-reloads
…le 'systemd', when relevant (#13542) Do so, in particular, to support the migration from the deprecated module 'camptocamp/systemd' to the maintained module 'puppet/systemd'. References: • voxpupuli/puppet-systemd#171 • https://github.com/voxpupuli/puppet-systemd/blob/master/README.md#daemon-reloads
…le 'systemd', when relevant (#13542) Do so, in particular, to support the migration from the deprecated module 'camptocamp/systemd' to the maintained module 'puppet/systemd'. References: • voxpupuli/puppet-systemd#171 • https://github.com/voxpupuli/puppet-systemd/blob/master/README.md#daemon-reloads
Since Puppet 6.1.0 it's no longer needed to run daemon-reload manually when restarting a service. That means it's possible to drop this code. Since Puppet 4 & 5 are now EOL, this should be ok.
Additionally it contains some documentation fixes (#172).
Alternative to #148.