Add postfix exporter

[alexjfisher]

No description provided.

[anarcat]

note that i already had a WIP for this in #310

[alexjfisher]

#310 was my starting point.

[alexjfisher]

Build currently failing on CentOS8. A new camptocamp/postfix release would include https://github.com/camptocamp/puppet-postfix/pull/257/files and fix this.

[anarcat]

apart from the whitespace stuff, this is all LGTM, and i can confirm it works with the debian package (although keep in mind i have #303 applied here).

[alexjfisher]

I've converted to puppet-strings doc and done my best to group related parameters. (Shame puppet-strings doesn't have any proper support for doing this).
Several parameters are removed altogether; either because they did nothing (eg purge), or because they were essentially private and didn't need exposing in the class's API (eg os, arch).

[anarcat]

hmm... i kind of use that env_vars stuff here. :p it's the way i use to pass parameters to the systemd service file without having to overwrite it completely (see #367 and #303).

can we keep that?

[anarcat]

2d1d8d1 reverts the env_vars stuff. I'll also note that if you really want to remove it from this exporter, you should at least remove it from the REFERENCES.md and the puppet strings docs. :p

[alexjfisher]

I'm not 100% against putting env_vars back in, but it didn't look like the postfix_exporter supported any. @anarcat Could you share how you currently declare this class for Debian (I can include that in an @example). Have you considered using a systemd drop-in file to override behaviour?
See https://coreos.com/os/docs/latest/using-systemd-drop-in-units.html
and https://github.com/camptocamp/puppet-systemd/blob/master/manifests/dropin_file.pp

[anarcat]

I'm not 100% against putting env_vars back in, but it didn't look like the postfix_exporter supported any.

I use it to pass the ARGS to the systemd file. I do this because, in #303, I patched this module to keep it from overwriting the systemd unit files everywhere, and instead use the ones from the Debian package.

@anarcat Could you share how you currently declare this class for Debian (I can include that in an @example).

It's tricky, because my usage depends on #303 and I'm not absolutely sure yet this is the right way to go. It's one of those sticky areas I'm having trouble merging my stuff in. :p

But here is our prometheus profile:

# add an exporter for the apache instance on this host
class profile::prometheus::postfix_exporter(
  Enum['present', 'absent'] $ensure = 'present',
) {
  $package_ensure = $ensure ? { 'present' => 'installed', 'absent' => 'absent' }
  $service_ensure = $ensure ? { 'present' => 'running', 'absent'  => 'stopped' }
  class { 'prometheus::postfix_exporter':
    package_ensure    => $package_ensure,
    service_ensure    => $service_ensure,
    export_scrape_job => true,
    # force debian package install parameters, further discussion in:
    # https://github.com/voxpupuli/puppet-prometheus/pull/303
    install_method    => 'package',
    user              => 'prometheus',
    group             => 'prometheus',
    package_name      => 'prometheus-postfix-exporter',
    service_name      => 'prometheus-postfix-exporter',
    # purge_config_dir => true,
    env_vars          => {
      'ARGS' => "[email protected] --postfix.logfile_path ''",
    },
  }

  # realize the allow rules defined on the prometheus server(s)
  Ferm::Rule <<| tag == 'profile::prometheus::server-postfix-exporter' |>>
}

Have you considered using a systemd drop-in file to override behaviour?

I have. The problem with that is that I don't really need to do that - the current flags passed by the postfix module are fine. What I want to avoid is completely overriding the Debian package's systemd files and using an override doesn't do that for me.

Basically, my problem is with this code in daemon.pp:

    'systemd' : {
      include 'systemd'
      systemd::unit_file {"${name}.service":
        content => template('prometheus/daemon.systemd.erb'),
        notify  => $notify_service,
      }
    }

I patched it to add a $manage_systemd_unit parameter to avoid this override if the Debian package is set, in #303:

modified   manifests/daemon.pp
@@ -82,6 +82,7 @@ define prometheus::daemon (
   String $service_ensure               = 'running',
   Boolean $service_enable              = true,
   Boolean $manage_service              = true,
+  Boolean $manage_systemd_unit         = $install_method != 'package',
   Hash[String, Scalar] $env_vars       = {},
   Optional[String] $env_file_path      = $prometheus::env_file_path,
   Optional[String[1]] $extract_command = $prometheus::extract_command,
@@ -184,10 +185,12 @@ define prometheus::daemon (
       }
     }
     'systemd' : {
-      include 'systemd'
-      systemd::unit_file {"${name}.service":
-        content => template('prometheus/daemon.systemd.erb'),
-        notify  => $notify_service,
+      if $manage_systemd_unit {
+        include 'systemd'
+        systemd::unit_file {"${name}.service":
+          content => template('prometheus/daemon.systemd.erb'),
+          notify  => $notify_service,
+        }
       }
     }
     # service_provider returns redhat on CentOS using sysv, https://tickets.puppetlabs.com/browse/PUP-5296

I know this isn't great. And I know I could just let it go and let the puppet module override the builtin systemd files shipped by the Debian package. but it just seems wrong to me: when I deploy a puppet module based on Debian packages, I try to reduce the diff that is deployed when puppet runs. This is one of them.

If you insist on removing env_vars, I would probably just patch it locally and keep that branch going in the short term. In the long term, I would need to figure out how to convert this module to "properly" use systemd with the Debian package, something it seems I was unable to do in #303 (or that might be better split out in a separate patchset?).

Anyways: I figured it would be much simpler for me to ask you to just keep this little env_vars hack of mine for the time being... ;)

[anarcat]

Several parameters are removed altogether; either because they did nothing (eg purge), or because they were essentially private and didn't need exposing in the class's API (eg os, arch).

After working on #399, I am not sure we should go with that approach here. I appreciate the idea of reducing the number of arguments in exporters - the current situation is kind of untenable, and it makes no sense to vary, say, $os or $init_style between two exporters on the same node anyways - but maybe it would be better to do this in a more coordinated way?

the way this is done now, only postfix would have this "new style" of parameters... shouldn't we do this uniformly everywhere?

if you will do the conversion in a later PR, I don't mind, but I think it's a problem if only postfix is like that...

furthermore, I am not sure i like the idea of moving those parameters out to the class anyways. here I had to do this to get the postfix exporter going in the end:

  class { 'prometheus':
    init_style => 'none',
  }
  class { 'prometheus::postfix_exporter':
    package_ensure    => $package_ensure,
    service_ensure    => $service_ensure,
    export_scrape_job => true,
    # force debian package install parameters, further discussion in:
    # https://github.com/voxpupuli/puppet-prometheus/pull/303
    install_method    => 'package',
    user              => 'prometheus',
    group             => 'prometheus',
    package_name      => 'prometheus-postfix-exporter',
    service_name      => 'prometheus-postfix-exporter',
    # purge_config_dir => true,
    env_vars          => {
      'ARGS' => "[email protected] --postfix.logfile_path ''",
    },
  }

this is getting very verbose! but maybe it's the right way to do this... i'm just not sure.

[anarcat]

i've rebased my master branch on this and things still work on my side. the only diff i have on the exporter is to re-enable the env_vars parameter that is present in other exporters but removed from this one, see 52cc6a0

[anarcat]

whoohoo! thanks!