Manage ulimit with systemd::dropin_file and allow it to be unmanaged #391

laugmanuel · 2021-02-22T15:41:48Z

This PR changes two things:

Adds the new parameter ulimit_managed to explicitly enable/disable ulimit
- in systemd unit für instances and
- systemd dropin for default setup
Use systemd:: service_limits to manage the ulimits for the default setup.
Note: This adds a new dependency to camptocamp/puppet-systemd.
Use systemd::unit_file for each instance service

Fixes #390

Note: I can split this PR into two/three PRs if desired (or squash the commits together).

laugmanuel · 2021-02-22T15:58:58Z

Test failures seem to be unrelated...

ekohl

Perhaps this should also be modified to use systemd::unit_file:

puppet-redis/manifests/instance.pp

Lines 312 to 318 in 551e58b

    
           file { "/etc/systemd/system/${service_name}.service": 
        
             ensure  => file, 
        
             owner   => 'root', 
        
             group   => 'root', 
        
             mode    => '0644', 
        
             content => template('redis/service_templates/redis.service.erb'), 
        
           }

https://github.com/voxpupuli/puppet-redis#soft-dependency also needs an update if systemd becomes a hard dependency.

manifests/init.pp

manifests/ulimit.pp

manifests/instance.pp

manifests/init.pp

ekohl · 2021-02-22T16:40:00Z

manifests/ulimit.pp

-  if versioncmp($facts['puppetversion'],'6.1.0') < 0 {
-    include systemd::systemctl::daemon_reload
-    Augeas['Systemd redis ulimit'] ~> Class['systemd::systemctl::daemon_reload']
+    ensure => absent,
  }


There is no daemon reload code for Puppet < 6.1.0 in systemd::service_limits. Interestingly, in combination with restart_service that appears to be a bug.

Edit: to be clear, I don't think you should set $restart_service to true, but it should deal with daemon reloads. Or we should drop Puppet < 6.1.0.

The daemon reload is contained in systemd::dropin_file which is invoked indirectly by systemd::service_limits. As this PR would use the new way of defining the limits regardless of Puppet version, the reload should be handled there.

README.md

ekohl · 2021-02-23T18:07:01Z

manifests/instance.pp

@@ -309,8 +313,8 @@
  }

  if $manage_service_file {
-    file { "/etc/systemd/system/${service_name}.service":
-      ensure  => file,
+    systemd::unit_file { "${service_name}.service":


If you pass enable and active as well, the unit file can manage the service and also properly apply daemon_reload. This is particularly important now that camptocamp/systemd has dropped the daemon_reload class in master.

See 58aa20c for what I think is roughly needed.

ekohl · 2021-02-23T18:11:00Z

manifests/ulimit.pp

  file { "/etc/systemd/system/${redis::service_name}.service.d/limit.conf":
-    ensure => file,
-    owner  => 'root',
-    group  => 'root',
-    mode   => '0444',
-  }
-  augeas { 'Systemd redis ulimit' :
-    incl    => "/etc/systemd/system/${redis::service_name}.service.d/limit.conf",
-    lens    => 'Systemd.lns',
-    changes => [
-      "defnode nofile Service/LimitNOFILE \"\"",
-      "set \$nofile/value \"${redis::ulimit}\"",
-    ],
-  }
-  # Only necessary for Puppet < 6.1.0,
-  # See https://github.com/puppetlabs/puppet/commit/f8d5c60ddb130c6429ff12736bfdb4ae669a9fd4
-  if versioncmp($facts['puppetversion'],'6.1.0') < 0 {
-    include systemd::systemctl::daemon_reload
-    Augeas['Systemd redis ulimit'] ~> Class['systemd::systemctl::daemon_reload']
+    ensure => absent,


Thinking more about this, it's a bit dubious how this should be handled. Previously it was an augeas lens which means it did manage one entry but users were free to manage other entries via other means.

The idea was that there is no duplicate of old and new settings. But you are right; if users added their custom limits, they should not get removed.
We could switch the augeas lens to exclude the the NoFile line or just stop managing the file at all (which will result in duplicates of NOFile)

Or we just stick to using the current approach and I'll drop switching to puppet-systemd from this PR.

I don't mind doing a major version bump for it and making it clear that this is the behavior. Could use a comment perhaps?

manifests/instance.pp

ekohl · 2021-03-30T11:54:45Z

manifests/ulimit.pp

-    owner                   => 'root',
-    group                   => 'root',
-    selinux_ignore_defaults => true,
+  systemd::service_limits { "${redis::service_name}.service":


What do you think about moving it into instance.pp? 2c2a322

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

laugmanuel · 2021-04-14T06:57:19Z

@ekohl, currently I'm a little bit hesitant as I can't quite overlook the changes/impact and get a full picture. WDYT about closing this PR in favour of new ones based on your commits (2c2a322 and 58aa20c)?

ekohl · 2021-04-14T09:56:26Z

I would be OK with that.

ekohl · 2021-04-19T13:01:38Z

I opened #396

Allow ulimit to be unmanaged

16a0ae4

laugmanuel changed the title ~~390 systemd dropin~~ Manage ulimit with systemd::dropin_file and allow it to be unmanaged Feb 22, 2021

ekohl requested changes Feb 22, 2021

View reviewed changes

manifests/init.pp Show resolved Hide resolved

manifests/ulimit.pp Outdated Show resolved Hide resolved

manifests/ulimit.pp Outdated Show resolved Hide resolved

Manage ulimit with systemd::dropin_file

0bbd6e6

ekohl reviewed Feb 22, 2021

View reviewed changes

Use systemd::unit_file for redis::instance

032e1f8

ekohl reviewed Feb 23, 2021

View reviewed changes

ekohl reviewed Mar 30, 2021

View reviewed changes

manifests/instance.pp Outdated Show resolved Hide resolved

ekohl reviewed Mar 30, 2021

View reviewed changes

Update manifests/instance.pp

5663a02

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

laugmanuel closed this Apr 14, 2021

ekohl mentioned this pull request Apr 19, 2021

Set ulimit via systemd using camptocamp/systemd #396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manage ulimit with systemd::dropin_file and allow it to be unmanaged #391

Manage ulimit with systemd::dropin_file and allow it to be unmanaged #391

laugmanuel commented Feb 22, 2021 •

edited

Loading

laugmanuel commented Feb 22, 2021

ekohl left a comment

ekohl Feb 22, 2021 •

edited

Loading

laugmanuel Feb 22, 2021

ekohl Feb 23, 2021

ekohl Mar 30, 2021

ekohl Feb 23, 2021

laugmanuel Feb 24, 2021

laugmanuel Feb 24, 2021

ekohl Mar 30, 2021 •

edited

Loading

ekohl Mar 30, 2021

laugmanuel commented Apr 14, 2021

ekohl commented Apr 14, 2021

ekohl commented Apr 19, 2021

	file { "/etc/systemd/system/${service_name}.service":
	ensure => file,
	owner => 'root',
	group => 'root',
	mode => '0644',
	content => template('redis/service_templates/redis.service.erb'),
	}

Manage ulimit with systemd::dropin_file and allow it to be unmanaged #391

Manage ulimit with systemd::dropin_file and allow it to be unmanaged #391

Conversation

laugmanuel commented Feb 22, 2021 • edited Loading

laugmanuel commented Feb 22, 2021

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

laugmanuel Feb 22, 2021

Choose a reason for hiding this comment

ekohl Feb 23, 2021

Choose a reason for hiding this comment

ekohl Mar 30, 2021

Choose a reason for hiding this comment

ekohl Feb 23, 2021

Choose a reason for hiding this comment

laugmanuel Feb 24, 2021

Choose a reason for hiding this comment

laugmanuel Feb 24, 2021

Choose a reason for hiding this comment

ekohl Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

ekohl Mar 30, 2021

Choose a reason for hiding this comment

laugmanuel commented Apr 14, 2021

ekohl commented Apr 14, 2021

ekohl commented Apr 19, 2021

laugmanuel commented Feb 22, 2021 •

edited

Loading

ekohl Feb 22, 2021 •

edited

Loading

ekohl Mar 30, 2021 •

edited

Loading