Merge pull request #1396 from SimonHoenscheid/shoenscheid_systemd_cleanup

Cleanup Systemd drop-in file handling

Author: chelnak

.fixtures.yml

@@ -10,3 +10,4 @@ fixtures:
     puppet_agent: 'https://github.com/puppetlabs/puppetlabs-puppet_agent.git'
     stdlib: "https://github.com/puppetlabs/puppetlabs-stdlib.git"
     yumrepo_core: "https://github.com/puppetlabs/puppetlabs-yumrepo_core.git"
+    systemd: "https://github.com/voxpupuli/puppet-systemd.git"

manifests/params.pp

@@ -26,7 +26,7 @@
   $package_ensure               = 'present'
   $module_workdir               = pick($module_workdir,'/tmp')
   $password_encryption          = undef
-  $extra_systemd_config         = ''
+  $extra_systemd_config         = undef
   $manage_datadir               = true
   $manage_logdir                = true
   $manage_xlogdir               = true

manifests/server/instance/config.pp

@@ -245,58 +245,14 @@
     }
   }
   # lint:ignore:140chars
-  # RHEL 7 and 8 both support drop-in files for systemd units.  The old include directive is deprecated and may be removed in future systemd releases.
-  # Gentoo also supports drop-in files.
+  # RHEL 7 and 8 both support drop-in files for systemd units. Gentoo also supports drop-in files.
+  # Edit 02/2023 RHEL basedc Systems and Gentoo need Variables set for $PGPORT, $DATA_DIR or $PGDATA, thats what the drop-in file is for.
   # lint:endignore:140chars
   if $facts['os']['family'] in ['RedHat', 'Gentoo'] and $facts['service_provider'] == 'systemd' {
-    # While Puppet 6.1 and newer can do a daemon-reload if needed, systemd
-    # doesn't appear to report that correctly in all cases.
-    # One such case seems to be when an overriding unit file is removed from /etc
-    # and the original one from /lib *should* be used again.
-    #
-    # This can be removed when Puppet < 6.1 support is dropped *and* the file
-    # old-systemd-override is removed.
-    $systemd_command = ['systemctl', 'daemon-reload']
-    exec { 'restart-systemd':
-      command     => $systemd_command,
-      refreshonly => true,
-      path        => '/bin:/usr/bin:/usr/local/bin',
-      before      => Class['postgresql::server::service'],
-    }
-
-    file {
-      default:
-        ensure => file,
-        owner  => root,
-        group  => root,
-        notify => [Exec['restart-systemd'], Class['postgresql::server::service']],
-        before => Class['postgresql::server::reload'];
-
-      'systemd-conf-dir':
-        ensure => directory,
-        path   => "/etc/systemd/system/${service_name}.service.d";
-
-      # Template uses:
-      # - $facts['os']['name']
-      # - $facts['os']['release']['major']
-      # - $service_name
-      # - $port
-      # - $datadir
-      # - $extra_systemd_config
-      'systemd-override':
-        path    => "/etc/systemd/system/${service_name}.service.d/${service_name}.conf",
-        content => template('postgresql/systemd-override.erb'),
-        require => File['systemd-conf-dir'];
-    }
-
-    if $service_enable != 'mask' {
-      # Remove old unit file to avoid conflicts
-      file { 'old-systemd-override':
-        ensure => absent,
-        path   => "/etc/systemd/system/${service_name}.service",
-        notify => [Exec['restart-systemd'], Class['postgresql::server::service']],
-        before => Class['postgresql::server::reload'],
-      }
+    postgresql::server::instance::systemd { $service_name:
+      port                 => $port,
+      datadir              => $datadir,
+      extra_systemd_config => $extra_systemd_config,
     }
   }
 }

manifests/server/instance/systemd.pp

@@ -0,0 +1,26 @@
+# @summary This define handles systemd drop-in files for the postgres main instance (default) or additional instances
+# @param service_name Overrides the default PostgreSQL service name.
+# @param drop_in_ensure sets the Systemd drop-in file to present or absent
+# @api private
+define postgresql::server::instance::systemd (
+  Variant[String[1], Stdlib::Port] $port,
+  Stdlib::Absolutepath $datadir,
+  Optional[String[1]] $extra_systemd_config = undef,
+  String[1] $service_name                   = $name,
+  Enum[present, absent] $drop_in_ensure     = 'present',
+
+) {
+  # Template uses:
+  # - $port
+  # - $datadir
+  # - $extra_systemd_config
+  systemd::dropin_file { "${service_name}.conf":
+    ensure  => $drop_in_ensure,
+    unit    => "${service_name}.service",
+    owner   => 'root',
+    group   => 'root',
+    content => template('postgresql/systemd-override.erb'),
+    notify  => Class['postgresql::server::service'],
+    before  => Class['postgresql::server::reload'],
+  }
+}

metadata.json

@@ -16,6 +16,10 @@
       "name": "puppetlabs/apt",
       "version_requirement": ">= 2.0.0 < 10.0.0"
     },
+    {
+      "name": "puppet/systemd",
+      "version_requirement": ">= 4.0.1 < 5.0.0"
+    },
     {
       "name": "puppetlabs/concat",
       "version_requirement": ">= 4.1.0 < 8.0.0"
@@ -87,7 +91,7 @@
   "requirements": [
     {
       "name": "puppet",
-      "version_requirement": ">= 6.0.0 < 8.0.0"
+      "version_requirement": ">= 6.24.0 < 8.0.0"
     }
   ],
   "pdk-version": "2.5.0",

spec/classes/server/config_spec.rb

@@ -19,29 +19,22 @@
         .that_requires('Package[policycoreutils-python]')
     end
 
-    it 'removes the old systemd-override file' do
-      is_expected.to contain_file('old-systemd-override')
-        .with(ensure: 'absent', path: '/etc/systemd/system/postgresql.service')
-    end
-
     it 'has the correct systemd-override drop file' do
-      is_expected.to contain_file('systemd-override').with(
-        ensure: 'file', path: '/etc/systemd/system/postgresql.service.d/postgresql.conf',
-        owner: 'root', group: 'root'
-      ) .that_requires('File[systemd-conf-dir]')
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf').with(
+        ensure: 'file', owner: 'root', group: 'root',
+      ) .that_requires('File[/etc/systemd/system/postgresql.service.d]')
     end
 
     it 'has the correct systemd-override file #regex' do
-      is_expected.to contain_file('systemd-override') \
-        .with_content(%r{(?!^.include)})
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf')
     end
 
     context 'RHEL 7 host with Puppet 5' do
       include_examples 'RedHat 7'
 
       it 'has systemctl restart command' do
-        is_expected.to contain_exec('restart-systemd').with(
-          command: ['systemctl', 'daemon-reload'],
+        is_expected.to contain_exec('systemd-postgresql.service-systemctl-daemon-reload').with(
+          command: 'systemctl daemon-reload',
           refreshonly: true,
           path: '/bin:/usr/bin:/usr/local/bin',
         )
@@ -53,21 +46,20 @@
         <<-EOS
           class { 'postgresql::globals':
             manage_package_repo => true,
-            version => '9.4',
+            version => '10',
           }->
           class { 'postgresql::server': }
         EOS
       end
 
       it 'has the correct systemd-override file' do
-        is_expected.to contain_file('systemd-override').with(
-          ensure: 'file', path: '/etc/systemd/system/postgresql-9.4.service.d/postgresql-9.4.conf',
-          owner: 'root', group: 'root'
+        is_expected.to contain_file('/etc/systemd/system/postgresql-10.service.d/postgresql-10.conf').with(
+          ensure: 'file', owner: 'root', group: 'root',
         )
       end
 
       it 'has the correct systemd-override file #regex' do
-        is_expected.to contain_file('systemd-override') .without_content(%r{\.include})
+        is_expected.to contain_file('/etc/systemd/system/postgresql-10.service.d/postgresql-10.conf') .without_content(%r{\.include})
       end
     end
   end
@@ -84,41 +76,34 @@ class { 'postgresql::server': }
         .that_requires('Package[policycoreutils-python-utils]')
     end
 
-    it 'removes the old systemd-override file' do
-      is_expected.to contain_file('old-systemd-override')
-        .with(ensure: 'absent', path: '/etc/systemd/system/postgresql.service')
-    end
-
     it 'has the correct systemd-override drop file' do
-      is_expected.to contain_file('systemd-override').with(
-        ensure: 'file', path: '/etc/systemd/system/postgresql.service.d/postgresql.conf',
-        owner: 'root', group: 'root'
-      ) .that_requires('File[systemd-conf-dir]')
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf').with(
+        ensure: 'file', owner: 'root', group: 'root',
+      ) .that_requires('File[/etc/systemd/system/postgresql.service.d]')
     end
 
     it 'has the correct systemd-override file #regex' do
-      is_expected.to contain_file('systemd-override') .without_content(%r{\.include})
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf').without_content(%r{\.include})
     end
 
     describe 'with manage_package_repo => true and a version' do
       let(:pre_condition) do
         <<-EOS
           class { 'postgresql::globals':
             manage_package_repo => true,
-            version => '9.4',
+            version => '14',
           }->
           class { 'postgresql::server': }
         EOS
       end
 
       it 'has the correct systemd-override file' do
-        is_expected.to contain_file('systemd-override').with(
-          ensure: 'file', path: '/etc/systemd/system/postgresql-9.4.service.d/postgresql-9.4.conf',
-          owner: 'root', group: 'root'
+        is_expected.to contain_file('/etc/systemd/system/postgresql-14.service.d/postgresql-14.conf').with(
+          ensure: 'file', owner: 'root', group: 'root',
         )
       end
       it 'has the correct systemd-override file #regex' do
-        is_expected.to contain_file('systemd-override') .without_content(%r{\.include})
+        is_expected.to contain_file('/etc/systemd/system/postgresql-14.service.d/postgresql-14.conf') .without_content(%r{\.include})
       end
     end
   end
@@ -135,20 +120,14 @@ class { 'postgresql::server': }
         .that_requires('Package[policycoreutils-python-utils]')
     end
 
-    it 'removes the old systemd-override file' do
-      is_expected.to contain_file('old-systemd-override')
-        .with(ensure: 'absent', path: '/etc/systemd/system/postgresql.service')
-    end
-
     it 'has the correct systemd-override drop file' do
-      is_expected.to contain_file('systemd-override').with(
-        ensure: 'file', path: '/etc/systemd/system/postgresql.service.d/postgresql.conf',
-        owner: 'root', group: 'root'
-      ) .that_requires('File[systemd-conf-dir]')
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf').with(
+        ensure: 'file', owner: 'root', group: 'root',
+      ) .that_requires('File[/etc/systemd/system/postgresql.service.d]')
     end
 
     it 'has the correct systemd-override file #regex' do
-      is_expected.to contain_file('systemd-override') .without_content(%r{\.include})
+      is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf').without_content(%r{\.include})
     end
 
     describe 'with manage_package_repo => true and a version' do
@@ -163,14 +142,13 @@ class { 'postgresql::server': }
       end
 
       it 'has the correct systemd-override file' do
-        is_expected.to contain_file('systemd-override').with(
-          ensure: 'file', path: '/etc/systemd/system/postgresql-13.service.d/postgresql-13.conf',
-          owner: 'root', group: 'root'
+        is_expected.to contain_file('/etc/systemd/system/postgresql-13.service.d/postgresql-13.conf').with(
+          ensure: 'file', owner: 'root', group: 'root',
         )
       end
 
       it 'has the correct systemd-override file #regex' do
-        is_expected.to contain_file('systemd-override') .without_content(%r{\.include})
+        is_expected.to contain_file('/etc/systemd/system/postgresql-13.service.d/postgresql-13.conf') .without_content(%r{\.include})
       end
     end
   end
@@ -193,7 +171,7 @@ class { 'postgresql::server': }
     let(:pre_condition) do
       <<-EOS
         class { 'postgresql::globals':
-          version => '9.5',
+          version => '14',
         }->
         class { 'postgresql::server':
           manage_pg_hba_conf => true,
@@ -221,7 +199,7 @@ class { 'postgresql::server':
       let(:pre_condition) do
         <<-EOS
           class { 'postgresql::globals':
-            version => '9.5',
+            version => '14',
           }->
           class { 'postgresql::server': }
         EOS
@@ -231,22 +209,11 @@ class { 'postgresql::server': }
         is_expected.not_to contain_exec('/usr/sbin/semanage port -a -t postgresql_port_t -p tcp 5432')
       end
 
-      it 'removes the old systemd-override file' do
-        is_expected.to contain_file('old-systemd-override')
-          .with(ensure: 'absent', path: '/etc/systemd/system/postgresql-9.5.service')
-      end
-
       it 'has the correct systemd-override drop file' do
-        is_expected.to contain_file('systemd-override').with(
-          ensure: 'file', path: '/etc/systemd/system/postgresql-9.5.service.d/postgresql-9.5.conf',
-          owner: 'root', group: 'root'
+        is_expected.to contain_file('/etc/systemd/system/postgresql-14.service.d/postgresql-14.conf').with(
+          ensure: 'file', owner: 'root', group: 'root',
         )
       end
-
-      it 'has the correct systemd-override file #regex' do
-        is_expected.to contain_file('systemd-override') \
-          .with_content(%r{(?!^.include)})
-      end
     end
   end
 end

spec/classes/server_spec.rb

@@ -187,17 +187,17 @@ class { 'postgresql::globals':
       <<-EOS
       class { 'postgresql::globals':
         manage_package_repo => true,
-        version             => '99.5',
+        version             => '14',
         before              => Class['postgresql::server'],
       }
       EOS
     end
 
     it 'contains the correct package version' do
-      is_expected.to contain_class('postgresql::repo').with_version('99.5')
-      is_expected.to contain_file('/var/lib/postgresql/99.5/main') # FIXME: be more precise
-      is_expected.to contain_concat('/etc/postgresql/99.5/main/pg_hba.conf') # FIXME: be more precise
-      is_expected.to contain_concat('/etc/postgresql/99.5/main/pg_ident.conf') # FIXME: be more precise
+      is_expected.to contain_class('postgresql::repo').with_version('14')
+      is_expected.to contain_file('/var/lib/postgresql/14/main') # FIXME: be more precise
+      is_expected.to contain_concat('/etc/postgresql/14/main/pg_hba.conf') # FIXME: be more precise
+      is_expected.to contain_concat('/etc/postgresql/14/main/pg_ident.conf') # FIXME: be more precise
     end
   end
 

spec/default_facts.yml

@@ -6,3 +6,4 @@ ipaddress: "172.16.254.254"
 ipaddress6: "FE80:0000:0000:0000:AAAA:AAAA:AAAA"
 is_pe: false
 macaddress: "AA:AA:AA:AA:AA:AA"
+path: '/bin:/usr/bin:/usr/local/bin'

spec/defines/server/config_entry_spec.rb

@@ -28,7 +28,7 @@
       include_examples 'RedHat 7'
 
       it 'stops postgresql and changes the port #file' do
-        is_expected.to contain_file('systemd-override')
+        is_expected.to contain_file('/etc/systemd/system/postgresql.service.d/postgresql.conf')
       end
     end
   end

templates/systemd-override.erb

@@ -1,6 +1,3 @@
-<%- if @os['name'] == 'Fedora' and @os['release']['major'] <= '31' -%>
-.include /usr/lib/systemd/system/<%= @service_name %>.service
-<% end -%>
 [Service]
 Environment=PGPORT=<%= @port %>
 <%- if @os['family'] == 'Gentoo' -%>