Change - Use systemd drop-in directory for unit overrides #1201

blackknight36 · 2020-10-27T16:11:43Z

The .include directive has been deprecated and support for it may be removed in a future systemd release. This fixes warnings issued by systemd which are shown as follows.

systemd[1]: /etc/systemd/system/postgresql-9.6.service:1: .include directives are deprecated, and support for them will be removed in a future version of systemd. Please use drop-in files instead.

puppet-community-rangefinder · 2020-10-27T16:11:51Z

postgresql::server::config is a class

that may have no external impact to Forge modules.

This module is declared in 71 of 575 indexed public `Puppetfiles`.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

daianamezdrea · 2020-11-02T10:07:05Z

Hi @blackknight36, thank you for submitting this PR, can you have a look at Travis failures which are coming from this?

manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 211
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 212
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 213
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 214
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 17, but found it in column 16) on line 219

blackknight36 · 2020-11-04T02:03:15Z

Hi @blackknight36, thank you for submitting this PR, can you have a look at Travis failures which are coming from this?

manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 211
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 212
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 213
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 214
manifests/server/config.pp - WARNING: indentation of => is not properly aligned (expected in column 17, but found it in column 16) on line 219

Yes, I should be able to fix it tomorrow.

ekohl · 2020-11-04T18:53:22Z

manifests/server/config.pp

        owner   => root,
        group   => root,
        content => template('postgresql/systemd-override.erb'),
        notify  => [Exec['restart-systemd'], Class['postgresql::server::service']],
        before  => Class['postgresql::server::reload'],
+        require => File['systemd-conf-dir'],


Isn't this autorequired since Puppet 2.7?

I'm not certain about that. I've had issues in the past with puppet trying to create files in a directory that doesn't exist which is why I added the requirement here. If you want to leave it off that's fine though.

In spec tests you can verify it:

it { is_expected.to contain_file('/etc/systemd/postgresql-server.d/override.conf').that_requires('/etc/systemd/postgresql-server.d') }

I just did a quick test and it appears that there is no autorequire for the parent directory. The unit test fails after adding a test similar to above.

ekohl · 2020-11-04T18:55:35Z

templates/systemd-override.erb

+<%- elsif scope.lookupvar('::operatingsystem') == 'Fedora' and scope.lookupvar('::operatingsystem') <= '31' -%>
 .include /lib/systemd/system/<%= @service_name %>.service
+<%- elsif scope.lookupvar('::operatingsystem') == 'Fedora' and scope.lookupvar('::operatingsystem') >= '32' -%>


I think drop ins should work on all supported variants of the Red Hat OS family.

Suggested change

<%- elsif scope.lookupvar('::operatingsystem') == 'Fedora' and scope.lookupvar('::operatingsystem') <= '31' -%>

.include /lib/systemd/system/<%= @service_name %>.service

<%- elsif scope.lookupvar('::operatingsystem') == 'Fedora' and scope.lookupvar('::operatingsystem') >= '32' -%>

<%- elsif @facts['os']['family'] == 'RedHat' -%>

In fact, I'm not sure if there's any systemd version that doesn't support this but then you need to modify the other conditionals in config.pp as well.

I didn't want to break servers running older Fedora releases and anything older than F32 is EOL now which is why I limited this change to more modern releases. I can run some tests on our Fedora 20 nodes to see if drop-in directories are supported but I didn't see anything in the documentation mentioning that feature.

Another way to handle this would be to create a "systemd_version" fact which may work better than limiting options to just the RedHat OS family.

It's supported on EL 7 which is based on Fedora 18ish IIRC. I wouldn't be surprised if it worked on even the ancient Fedora 17 that's listed in globals.pp.

The oldest Fedora server I have available right now is running Fedora 20. The man page for systemd.unit makes no mention of support for drop-in directories. The man page on CentOS 7 does mention them which makes sense since EL7 runs systemd version 219 vs. 208 on Fedora 20.

Knowing this I should be able to make these changes work with Fedora 20 and up.

@blackknight36 we have to include conditions for Redhat 8 since the deprecated warnings are coming there too.Will run the tests on all OSes and see if its break anything. Thanks alot for your contribution.

Thanks, I forgot to add that. I'll fix the template and push again.

Ideally the include statement should be removed on all OSes, not just Redhat. I'm sure that Gentoo also includes a newer version of systemd which would be affected in the future as well. I don't have a Gentoo server to test on however.

I think worries about drop ins are really not needed. This is literally the only reason I know that includes are a thing. On my Gentoo boxes I use drop ins regularly. I think that on all distros you can rely on them.

manifests/server/config.pp

puppet-community-rangefinder · 2020-11-05T13:57:23Z

postgresql::server::config is a class

that may have no external impact to Forge modules.

This module is declared in 71 of 575 indexed public `Puppetfiles`.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

maage · 2020-11-11T11:58:18Z

I see this issue at CentOS 8 too:

Wed 2020-11-11 11:50:46 UTC hostname systemd[1]: /etc/systemd/system/postgresql-13.service:1: .include directives are deprecated, and support for them will be removed in a future version of systemd. Please use drop-in files instead.

$ rpm -q systemd
systemd-239-31.el8_2.2.x86_64

$ facter os
{
  architecture => "x86_64",
  family => "RedHat",
  hardware => "x86_64",
  name => "CentOS",
  release => {
    full => "8.2.2004",
    major => "8",
    minor => "2"
  },
  selinux => {
    config_mode => "enforcing",
    config_policy => "targeted",
    current_mode => "enforcing",
    enabled => true,
    enforced => true,
    policy_version => "31"
  }
}

blackknight36 · 2020-11-11T15:06:32Z

I see this issue at CentOS 8 too:

Wed 2020-11-11 11:50:46 UTC hostname systemd[1]: /etc/systemd/system/postgresql-13.service:1: .include directives are deprecated, and support for them will be removed in a future version of systemd. Please use drop-in files instead.
$ rpm -q systemd
systemd-239-31.el8_2.2.x86_64

I can update this PR to include CentOS 8 as well.

blackknight36 · 2020-11-11T15:12:02Z

Another thing I noticed is that the postgresql::server::config class does not appear to address RHEL 5 or 6 although these releases are supported according to the metadata file. Should there be checks set up for these releases as well?

maage · 2020-11-11T16:09:38Z

RHEL/CentOS 5/6 does not use systemd, they use old upstart, service - command and /etc/init.d -directory setup.

Regarding to upstream supports:

https://access.redhat.com/support/policy/updates/errata#Life_Cycle_Dates

RH extended support is going to end at the end of 2020-11. And RHEL6 maintenance support is going to end also 2020-11, but extended support is still possible until 2024-06.

metadata.json also does not mention Fedora (currently 31, 32, 33).

I guess any supported OS version should be supported with testing and no testing is necessary for unsupported OS combos.

sheenaajay · 2020-11-16T11:41:04Z

Thanks alot @blackknight36 for the changes. Could you please fix the travis failures.
Thank you @ekohl and @maage for the valuable comments.

codecov-io · 2020-11-16T16:55:42Z

Codecov Report

Merging #1201 (4f976e6) into main (08e3a51) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1201   +/-   ##
=======================================
  Coverage   65.81%   65.81%           
=======================================
  Files          14       14           
  Lines         351      351           
=======================================
  Hits          231      231           
  Misses        120      120

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 711a729...4f976e6. Read the comment docs.

blackknight36 · 2020-11-17T01:13:17Z

Thanks alot @blackknight36 for the changes. Could you please fix the travis failures.
Thank you @ekohl and @maage for the valuable comments.

The Travis failures have been fixed. Please let me know if you need any other changes.

ekohl

Not a module maintainer, but I'd strongly urge you to drop the old include behavior.

templates/systemd-override.erb

manifests/server/config.pp

spec/unit/classes/server/config_spec.rb

manifests/server/config.pp

spec/unit/classes/server/config_spec.rb

The .include directive has been deprecated and support for it may be removed in a future systemd release. Drop-in directories are supported on all current RedHat, Fedora, and Gentoo releases. This branch contains changes to support this feature and disables the old behavior of using Include directives on RedHat based nodes.

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

sheenaajay · 2020-12-07T12:40:25Z

Thank you @blackknight36 @ekohl for your contribution.

vchepkov · 2021-01-27T00:05:44Z

This change breaks existing postgres instance (pre 6.9.0). One need to issue systemctl daemon-reload first, after old unit file was deleted and only after that restart the newly configured service, otherwise service fails

blackknight36 · 2021-01-27T16:32:28Z

This change breaks existing postgres instance (pre 6.9.0). One need to issue systemctl daemon-reload first, after old unit file was deleted and only after that restart the newly configured service, otherwise service fails

Thanks for the report. I will see if there is a way to avoid this. It would be best to create an issue as the PR has already been merged.

vchepkov · 2021-01-27T16:40:40Z

I would if issues were enabled on the repository.
I see that a fix is in progress #1230

…f_dir Change - Use systemd drop-in directory for unit overrides

blackknight36 requested a review from a team as a code owner October 27, 2020 16:11

david22swan changed the base branch from master to main November 2, 2020 12:24

sheenaajay assigned daianamezdrea Nov 2, 2020

ekohl requested changes Nov 4, 2020

View reviewed changes

blackknight36 closed this Nov 5, 2020

blackknight36 reopened this Nov 5, 2020

sheenaajay self-assigned this Nov 16, 2020

sheenaajay approved these changes Nov 23, 2020

View reviewed changes

sheenaajay added the bugfix label Nov 23, 2020

ekohl reviewed Nov 24, 2020

View reviewed changes

templates/systemd-override.erb Outdated Show resolved Hide resolved

templates/systemd-override.erb Outdated Show resolved Hide resolved

manifests/server/config.pp Outdated Show resolved Hide resolved

blackknight36 changed the title ~~Change - Use systemd conf dirs for overrides~~ Change - Use systemd drop-in directory for unit overrides Nov 26, 2020

ekohl reviewed Nov 26, 2020

View reviewed changes

manifests/server/config.pp Outdated Show resolved Hide resolved

manifests/server/config.pp Outdated Show resolved Hide resolved

spec/unit/classes/server/config_spec.rb Outdated Show resolved Hide resolved

manifests/server/config.pp Outdated Show resolved Hide resolved

ekohl requested changes Nov 27, 2020

View reviewed changes

manifests/server/config.pp Outdated Show resolved Hide resolved

spec/unit/classes/server/config_spec.rb Outdated Show resolved Hide resolved

spec/unit/classes/server/config_spec.rb Outdated Show resolved Hide resolved

spec/unit/classes/server/config_spec.rb Outdated Show resolved Hide resolved

Michael Watters and others added 5 commits November 30, 2020 11:01

Update manifests/server/config.pp

1cb5c18

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

Update spec/unit/classes/server/config_spec.rb

74d6b07

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

Update spec/unit/classes/server/config_spec.rb

3553517

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

Update spec/unit/classes/server/config_spec.rb

4f976e6

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

sheenaajay approved these changes Dec 7, 2020

View reviewed changes

sheenaajay merged commit efdd359 into puppetlabs:main Dec 7, 2020

blackknight36 deleted the use_systemd_conf_dir branch December 21, 2020 15:47

cegeka-jenkins pushed a commit to cegeka/puppet-postgresql that referenced this pull request Feb 3, 2022

Merge pull request puppetlabs#1201 from blackknight36/use_systemd_con…

a51ded0

…f_dir Change - Use systemd drop-in directory for unit overrides

Change - Use systemd drop-in directory for unit overrides #1201

Change - Use systemd drop-in directory for unit overrides #1201

Uh oh!

Conversation

blackknight36 commented Oct 27, 2020

Uh oh!

puppet-community-rangefinder bot commented Oct 27, 2020

postgresql::server::config is a class

This module is declared in 71 of 575 indexed public Puppetfiles.

Uh oh!

daianamezdrea commented Nov 2, 2020

Uh oh!

blackknight36 commented Nov 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

puppet-community-rangefinder bot commented Nov 5, 2020

postgresql::server::config is a class

This module is declared in 71 of 575 indexed public Puppetfiles.

Uh oh!

maage commented Nov 11, 2020

Uh oh!

blackknight36 commented Nov 11, 2020

Uh oh!

blackknight36 commented Nov 11, 2020

Uh oh!

maage commented Nov 11, 2020

Uh oh!

sheenaajay commented Nov 16, 2020

Uh oh!

codecov-io commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blackknight36 commented Nov 17, 2020

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sheenaajay commented Dec 7, 2020

Uh oh!

vchepkov commented Jan 27, 2021

Uh oh!

blackknight36 commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

This module is declared in 71 of 575 indexed public `Puppetfiles`.

ekohl Nov 4, 2020 •

edited

Loading

This module is declared in 71 of 575 indexed public `Puppetfiles`.

codecov-io commented Nov 16, 2020 •

edited

Loading

blackknight36 commented Jan 27, 2021 •

edited

Loading