Update to support Puppet 4 #18

trevor-vaughan · 2017-01-16T02:51:15Z

Optimized the limits files
Added a custom data type for service limits
Created a class for restarting the daemon so that other modules can
use it cleanly
Updated tests

Closes #17

zbikmarc · 2017-02-16T13:38:44Z

Any chance for merging it?

raphink · 2017-02-21T17:11:12Z

Gemfile

@@ -25,7 +25,8 @@ group :development, :unit_tests do
 end

 group :system_tests do
-  gem 'beaker',               :require => false
+  gem 'beaker', :git => 'https://github.com/trevor-vaughan/beaker.git', :branch => 'BKR-978-2.51.0'


We're managing this with modulesync, so we might need to use this in all of our modules...

@raphink This is now fixed in Beaker. I'll put in the correct updates.

raphink · 2017-02-21T17:11:53Z

That's quite a PR. I'll have to take time to review it.

oranenj · 2017-04-02T14:02:30Z

I took a look, and did not find anything objectionable.

Perhaps define types for the patterns used for limits? those seem like they might be generally useful when dealing with systemd. Also, don't some values accept 'infinity' as well?

Something like Systemd::Interval and Systemd::Size?

raphink · 2017-04-18T12:11:50Z

That LGTM too, I'll just have to resolve that modulesync bit. @trevor-vaughan could you rebase please?

trevor-vaughan · 2017-04-18T14:24:25Z

@oranenj You are correct about the 'infinity' option. I'll double check those. The systemd documentation is...fragmented at best.

I hope to have this done by the end of the week.

trevor-vaughan · 2017-05-19T20:48:36Z

@oranenj and @raphink Sorry that this took so long, the actual items supported by the configuration were a bit more involved than expected but it should all be properly updated now.

raphink · 2017-05-22T07:47:49Z

README.md

@@ -24,15 +24,16 @@ Let this module handle file creation and systemd reloading.
 Or handle file creation yourself and trigger systemd.

 ```puppet
-include ::systemd
+include ::systemd::systemctl::daemon_reload


I think I'd rather keep the interface in the systemd class and contain the subclasses (systemd::systemctl::daemon_reload and systemd::tmpfiles) for dependencies instead. Is there something that prevents from doing that?

We can contain daemon_reload but we can't contain tmpfiles since tmpfiles is triggered from the use of the tmpfile defined type.

A notify to Class[systemd] makes sense for reloading the whole thing though so I'll switch that one to a contain and hope that we don't get more creative in the future.

I see you contained the class in init.pp, so I'm guessing the README is not up-to-date?

raphink · 2017-05-22T07:47:55Z

manifests/init.pp

 class systemd (
-  $service_limits = {}
+  Optional[Hash] $service_limits = undef


Why not make the default {} and check for the size of the hash? I find this a bit more logical than allowing for undef as a default value.

I'd like to leave this one if possible. The compiler is so bloated already that everything we can bind to a single symbol helps.

Also, the code is easier to read and not confusing if, for some reason, we decide to include data in modules later.

I'm 👍 for this. We use the same style in the Vox Pupuli modules.

raphink · 2017-05-22T07:50:48Z

manifests/service_limits.pp

+  }
+
+  if $limits and !empty($limits) {
+    $_content = template("${module_name}/limits.erb")


I don't see a $content variable. What is the need for $_content?

It denotes it as a private variable that should not be used outside of the class. It's been convention for a while but hasn't made it into the style guide yet.

When actual private variables become a thing, this will be easier.

OK, fine for me.

raphink · 2017-05-22T07:51:49Z

manifests/service_limits.pp

+      mode   => '0644',
+      seltype => 'systemd_unit_file_t',
+    }
+  )


I'm not a big fan of using ensure_resource. I'd rather put this in a separate class and include it.

I can't. The name is dynamic based on the path and title parameters. It must be created for each call to the define but there can be multiple calls to the define that target the same location.

If we break it out into another define, we'll still have to use ensure_resource

OK, but then that means that file is unique, so what's the point of using ensure_resource?

raphink · 2017-05-22T07:52:30Z

manifests/service_limits.pp

  }

+  File["${path}/${title}.d/90-limits.conf"] ~> Class['systemd::systemctl::daemon_reload']


Why not specify this directly above in the file declaration?

I find it easier to read, but I've switched it.

raphink · 2017-05-22T07:53:20Z

manifests/service_limits.pp

    }
+
+    File["${path}/${title}.d/90-limits.conf"] ~> Exec["restart ${title} because limits"]
+    Class['systemd::systemctl::daemon_reload'] -> Exec["restart ${title} because limits"]


This could also be specified in the exec declaration.

raphink · 2017-05-22T07:54:44Z

manifests/tmpfile.pp

  file { "${path}/${title}":
    ensure  => $ensure,
    content => $content,
    source  => $source,
    owner   => 'root',
    group   => 'root',
-    mode    => '0444',
-    notify  => Exec['systemd-tmpfiles-create'],
+    mode    => '0644',


I'm not sure why this was 0444 and why this needs to be 0644 now.

raphink · 2017-05-22T07:56:52Z

manifests/unit_file.pp

 ) {
  include ::systemd

+  if $title !~ Pattern['^.+\.(service|socket|device|mount|automount|swap|target|path|timer|slice|scope)$'] {
+    fail('$name must match Pattern["^.+\.(service|socket|device|mount|automount|swap|target|path|timer|slice|scope)$"]')


Or maybe ship a data type for this and use assert_type?

raphink · 2017-05-22T07:57:21Z

manifests/unit_file.pp

+  if $target {
+    $_ensure = 'link'
+  }
+  else {


style nitpick: I'd rather not have a NL here

It makes my C soul hurt, but OK.

raphink · 2017-05-22T07:57:39Z

manifests/unit_file.pp

    content => $content,
    source  => $source,
    target  => $target,
    owner   => 'root',
    group   => 'root',
-    mode    => '0444',
-    notify  => Exec['systemctl-daemon-reload'],
+    mode    => '0644',


Why change the mode?

trevor-vaughan · 2017-05-22T13:51:30Z

@raphink Code updated per the discussion above

* Optimized the limits files * Added a custom data type for service limits * Created a class for restarting the daemon so that other modules can use it cleanly * Updated tests Closes voxpupuli#17

Fixes the issue that if, for some reason, someone would need to trigger either networkd, or resolved independently, they would need to break the class abstration and call the Service resources directly.

trevor-vaughan · 2017-07-16T01:16:44Z

@bastelfreak Would you mind looking this over one last time. I made a few additional changes and would like some feedback.

I've pinged @raphink to see if he has any objections to the latest batch of updates and will push this tomorrow evening if I haven't heard anything by then. We can then rebase and work on #32

bastelfreak · 2017-07-16T07:00:39Z

manifests/init.pp

+  Boolean                                    $manage_resolved = false,
+  Variant[Enum['stopped','running'],Boolean] $resolved_ensure  = 'running',
+  Boolean                                    $manage_networkd = false,
+  Variant[Enum['stopped','running'],Boolean] $networkd_ensure = 'running',


on the vox pupuli modules we agreed to only allow 'stopped' and 'running'. we dropped the boolean during the migration from puppet3 to puppet4/5.

bastelfreak · 2017-07-16T07:15:06Z

manifests/unit_file.pp

-  $content = undef,
-  $source = undef,
-  $target = undef,
+  Enum['file', 'absent']         $ensure  = 'file',


compared to the service ensure above, we don't allow true/false here, which is good in my opinion.

same comment on file/present though

bastelfreak · 2017-07-16T07:20:39Z

README.md

 file { '/usr/lib/systemd/system/foo.service':
  ensure => file,
  owner  => 'root',
  group  => 'root',
  mode   => '0644',
  source => "puppet:///modules/${module_name}/foo.service",
 } ~>
-Exec['systemctl-daemon-reload']
+Class['systemd::systemctl::daemon_reload']


the ~> needs to be in front of Class[]

I honestly like to break these out explicitly, but I've been trying to follow the class.

bastelfreak · 2017-07-16T07:20:44Z

README.md

 file { '/etc/tmpfiles.d/foo.conf':
  ensure => file,
  owner  => 'root',
  group  => 'root',
  mode   => '0644',
  source => "puppet:///modules/${module_name}/foo.conf",
 } ~>
-Exec['systemd-tmpfiles-create']
+Class['systemd::tmpfiles']


the ~> needs to be in front of Class[]

Fixed these and the one missing trailing comma (which I also don't agree with) :-D

bastelfreak · 2017-07-16T07:22:00Z

@trevor-vaughan done

trevor-vaughan · 2017-07-17T00:04:08Z

@bastelfreak Mischief Managed. @raphink indicated that he could review this week, but not this weekend. Will pester mercilessly until we get things through ;-).

raphink · 2017-07-17T13:27:04Z

README.md

@@ -24,15 +24,16 @@ Let this module handle file creation and systemd reloading.
 Or handle file creation yourself and trigger systemd.

 ```puppet
-include ::systemd
+include ::systemd::systemctl::daemon_reload


I see you contained the class in init.pp, so I'm guessing the README is not up-to-date?

raphink · 2017-07-17T13:27:52Z

manifests/init.pp

-  $service_limits          = {},
-  Boolean $manage_resolved = true,
-  Boolean $manage_networkd = true,
+  Optional[Hash]            $service_limits  = undef,


We might want to create a Data Type for this eventually to have finer validation.

@raphink To the first comment: No, this is correct. You can do either of those cases in a valid manner.

systemd::unit_file`` includes systemd` so that it is independently feature complete and since this is the 90% use case of this module.

@raphink To the second comment....I already made one of those :-|

raphink · 2017-07-17T13:29:14Z

manifests/networkd.pp

+#   The state that the ``networkd`` service should be in
+#
+class systemd::networkd (
+  Variant[Enum['stopped','running'],Boolean] $ensure = 'running',


If it's a private class and the values are limited to Enum['stopped','running'] in init.pp, I'm not sure how it could ever be a Boolean?

raphink · 2017-07-17T13:29:27Z

manifests/resolved.pp

+#   The state that the ``resolved`` service should be in
+#
+class systemd::resolved (
+  Variant[Enum['stopped','running'],Boolean] $ensure  = $::systemd::resolved_ensure,


Same Enum/Boolean comment here

raphink · 2017-07-17T13:29:49Z

manifests/networkd.pp

+  assert_private()
+
+  $_enable_networkd = $ensure ? {
+    /stopped/ => false,


Why use regexp here?

raphink · 2017-07-17T13:31:32Z

manifests/service_limits.pp

+  }
+
+  if $limits and !empty($limits) {
+    $_content = template("${module_name}/limits.erb")


OK, fine for me.

raphink · 2017-07-17T13:32:44Z

manifests/service_limits.pp

+      mode   => '0644',
+      seltype => 'systemd_unit_file_t',
+    }
+  )


OK, but then that means that file is unique, so what's the point of using ensure_resource?

raphink · 2017-07-17T13:34:16Z

manifests/tmpfile.pp

-  $path = '/etc/tmpfiles.d',
-  $content = undef,
-  $source = undef,
+  Enum['file','absent'] $ensure  = 'file',


same comment on file/present

raphink · 2017-07-17T13:35:06Z

manifests/unit_file.pp

-  $content = undef,
-  $source = undef,
-  $target = undef,
+  Enum['file', 'absent']         $ensure  = 'file',


same comment on file/present though

raphink · 2017-07-17T13:35:53Z

manifests/unit_file.pp

 ) {
  include ::systemd

+  assert_type(Systemd::Unit, $title)


I would rather assert $name and use a path attribute in the file below, to allow users to choose their resource title as they want.

trevor-vaughan · 2017-07-18T01:31:37Z

@raphink

raphink · 2017-07-19T07:23:08Z

OK for me. @trevor-vaughan @bastelfreak I'll let you squash and merge.

Dismissing based on #18 (comment)

mwhahaha · 2017-07-20T01:40:11Z

types/servicelimits.pp

+    Optional['LimitMSGQUEUE']   => Pattern['^(infinity|((\d+(K|M|G|T|P|E)(:\d+(K|M|G|T|P|E))?)))$'],
+    Optional['LimitNICE']       => Variant[Integer[0,40], Pattern['^(-\+([0-1]?[0-9]|20))|([0-3]?[0-9]|40)$']],
+    Optional['LimitRTPRIO']     => Integer[0],
+    Optional['LimitRTTIME']     => Pattern['^\d+(ms|s|m|h|d|w|M|y)?(:\d+(ms|s|m|h|d|w|M|y)?)?$']


FYI this is MemoryLimit so this broke us

This is missing...

See #23 for context

@mwhahaha It would be lovely if the systemd man pages weren't a rats nest of crazy.

Would it make sense to add all of the things in this section to the Systemd::ServiceLimits type?

http://www.dsm.fordham.edu/cgi-bin/man-cgi.pl?topic=systemd.resource-control&ampsect=5

Yes if you're going to validate them, then you need to support all of them. IMHO i wouldn't validate the keys/values because it'll be a constant battle to keep the module to supporting all the things added to systemd over time. It would be better to do simple validation to just ensure that the key is a valid string (no special chars) and just accept whatever values.

@mwhahaha At that point, you might as well just drop your own file in place. That's all this does anyway after some validation. Really, the only thing it adds is validation.

Well we were using this to trigger the required restarts and systemd actions. So dropping the file is not the only thing that is needed.

For reference, https://github.com/openstack/puppet-tripleo/blob/8ca7a1e390ce50ac66a6861dae59bb44fbf0324a/manifests/profile/base/database/mongodb.pp#L64-L69

I was just saying that trying to validate via puppet the underlying (and changing) systemd config file requirements in this puppet module seems to add more work than it's worth.

@mwhahaha Quite possibly, yes. We'll see as time goes on. In theory, as people automate more around systemd, they'll need to get their act together and stop changing everything regularly.

Then again, when you want systemd to be the automation tool, I guess that can get difficult.

trevor-vaughan force-pushed the puppet4-update branch from 0ea30e4 to 0b13016 Compare January 16, 2017 03:01

gregswift mentioned this pull request Feb 8, 2017

Add logic to fill out systemd local environment overrides rackerlabs/puppet-repose#62

Closed

This was referenced Feb 10, 2017

investigate update to camptocamp/systemd module #21

Closed

Investigate move to camptocamp/systemd module once puppet4 issues are resolved. rackerlabs/puppet-repose#63

Closed

raphink reviewed Feb 21, 2017

View reviewed changes

trevor-vaughan force-pushed the puppet4-update branch from 0b13016 to a95438d Compare May 19, 2017 20:47

raphink requested changes May 22, 2017

View reviewed changes

ekohl mentioned this pull request Jul 13, 2017

General cleanup + add Puppet4 datatypes #32

Merged

trevor-vaughan added 5 commits July 15, 2017 20:26

Update to support Puppet 4

49ba508

* Optimized the limits files * Added a custom data type for service limits * Created a class for restarting the daemon so that other modules can use it cleanly * Updated tests Closes voxpupuli#17

Code Review Updates

1b706a8

lint updates

684b4e7

remove PE from metadata

fa06d8d

rebase and update spec tests

333a362

trevor-vaughan force-pushed the puppet4-update branch from 7168bb0 to 333a362 Compare July 16, 2017 01:00

Break out networkd and resolved into classes

4f8c24f

Fixes the issue that if, for some reason, someone would need to trigger either networkd, or resolved independently, they would need to break the class abstration and call the Service resources directly.

not all systems run resolved or networkd

1acb7bf

trevor-vaughan force-pushed the puppet4-update branch from 48c7908 to 1acb7bf Compare July 16, 2017 01:31

bastelfreak reviewed Jul 16, 2017

View reviewed changes

Added missing files and updated syntax

d7945d8

bastelfreak approved these changes Jul 17, 2017

View reviewed changes

Update for Puppet 5

31a8b55

raphink previously requested changes Jul 17, 2017

View reviewed changes

Code Review Adjustments

f2c3883

missed linter update

4508f56

trevor-vaughan merged commit 6e1414c into voxpupuli:master Jul 19, 2017

mwhahaha reviewed Jul 20, 2017

View reviewed changes

EmilienM mentioned this pull request Jul 20, 2017

PR#18 broke service limits capacity #35

Closed

reenberg mentioned this pull request Jan 29, 2020

Discussion: use class instead of exec for notification #2

Closed

raphink added the enhancement New feature or request label Aug 21, 2020

		}

		File["${path}/${title}.d/90-limits.conf"] ~> Class['systemd::systemctl::daemon_reload']

Update to support Puppet 4 #18

Update to support Puppet 4 #18

Conversation

trevor-vaughan commented Jan 16, 2017

zbikmarc commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphink commented Feb 21, 2017

oranenj commented Apr 2, 2017

raphink commented Apr 18, 2017

trevor-vaughan commented Apr 18, 2017

trevor-vaughan commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-vaughan commented May 22, 2017

trevor-vaughan commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bastelfreak commented Jul 16, 2017

trevor-vaughan commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-vaughan commented Jul 18, 2017

raphink commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment