add Debian package configuration #303

anarcat · 2019-03-12T18:52:08Z

Pull Request (PR) description

This creates the necessary defaults on Debian to install Prometheus with the right settings from the Debian package.

The only major difference is the systemd unit file is kept in /etc/systemd/system: that is outside the scope of this change, as it's part of the systemd module.

This Pull Request (PR) fixes the following issues

Fixes: #32, #323

Remaining work

I was able to install the node-exporter with this configuration using
standard Debian packages. This is a Work In Progress (WIP) because I
haven't tested the other packages and especially the main prometheus
package. Hopefully it shouldn't be too much work to implement that as
well.

The change is mostly a noop when running on top of node-exporter already installed by hand using normal Debian packages.

Tests and documentation might also need an update.

bastelfreak · 2019-03-19T21:44:01Z

@anarcat can you please take a look at the failing travis jobs?

anarcat · 2019-03-19T23:14:31Z

it's partly stuff that has to do with the path changes, for example:

   expected that the catalogue would contain File[/usr/local/bin/node_exporter]

because it changes the defaults in the Debian environment, it affects basically the entire test suite. i'm not sure how to handle that...

anarcat · 2019-03-19T23:22:02Z

there's another thing i'm uncertain about: the Debian packages for the *-exporter stuff all use the same prometheus user: they don't create exporter-specific users the way the module does otherwise. this is what I tried to solve with:

prometheus::node_exporter::group: 'prometheus'
prometheus::node_exporter::user: 'prometheus'

... but I haven't done that with the apache exporter, for example. should we sync those up or does it matter?

similarly, the debian package already ship a .service file in /lib but the module installs a new one in /etc which shadows and overrides the one provided by the debian package. does that matter?

bastelfreak · 2019-03-20T23:03:18Z

service files that come from a package manager should go into /lib, custom services files should go into /etc. If the upstream service files work, we should probably use them and don't overwrite them.

anarcat · 2019-03-20T23:32:35Z

service files that come from a package manager should go into /lib, custom services files should go into /etc. If the upstream service files work, we should probably use them and don't overwrite them.

so how should we do this? the default path is set in the systemd module and we call into systemd::unit_file from prometheus::daemon.

what you seem to be saying is that we should just not write a .service file at all if we're installing through a package is that correct? but then a bunch of stuff could break if (for example) the username is changed.

right now, the user override I did for the node exporter was to avoid creating a user, but the apache exporter would have been broken if we didn't install our own .service file...

bastelfreak · 2019-03-21T00:53:39Z

I think that a parameter like manage_systemd_unit_file might be a good idea. This should default to false, if install_method is package. But I'm not sure how easy it is to workaround the different user/groupnames.

anarcat · 2019-03-21T15:47:52Z

if i would have a clue of how to do unit tests, i would design a unit test that would:

install everything through debian package (*::install_method: package)
run the puppet manifests and make sure systemd units, users and groups are not created
that everything still works:
- that the processes get started with the right flags
- that they run as the right user
- etc

This is partly why this is WIP: I have no idea how to do this. I've been learning some spec chops in #304 but i'm nowhere near the level where I could do this...

anarcat · 2019-03-21T19:45:23Z

i have updated the branch to just bite the bullet and disable the .service file when install_method is through a package. no unit tests, but we should make sure things work somehow, i don't know... I also had to cleanup the old files using such a resource in the parent profile:

  # cleanup old .service file previously deployed by prometheus module
  include systemd
  file {
    '/etc/systemd/system/prometheus.service':
      ensure => absent,
      notify => Class['systemd::systemctl::daemon_reload'],
  }

similar structures for the exporter, of course... that feels a little redundant - i wonder if we shouldn't enforce that in the class instead of wrapping the whole systemd thing inside an if. but then it would destroy local customizations people might have as well, so maybe it's better to let the users handle the transition since we don't support debian packages officially anyways right now...

bastelfreak · 2019-03-22T22:44:54Z

manifests/config.pp

-          content => template('prometheus/prometheus.systemd.erb'),
+        if $prometheus::server::manage_systemd_unit {
+          include 'systemd'
+          systemd::unit_file {'prometheus.service':


instead of defining the systemd::unit_file resoure here, should we only set $ensure = 'present and pass that to the resource? And if $prometheus::server::manage_systemd_unit isn't true, set $ensure = 'absent'.

that's what i said earlier:

i wonder if we shouldn't enforce that in the class instead of wrapping the whole systemd thing inside an if. but then it would destroy local customizations people might have as well, so maybe it's better to let the users handle the transition since we don't support debian packages officially anyways right now...

i don't mind either way...

anarcat · 2019-04-03T15:00:22Z

what should be my next step here?

anarcat · 2019-04-08T15:41:55Z

I think that a parameter like manage_systemd_unit_file might be a good idea. This should default to false, if install_method is package. But I'm not sure how easy it is to workaround the different user/groupnames.

Another problem has come up with this in #310: the way parameters are passed in the Debian service file is through the $ARGS variable set in the EnvironmentFile (/etc/default/prometheus-postfix-exporter). The .service file deployed by the puppet modules doesn't use that mechanism to pass flags which makes it difficult to have a coherent approach for both.

For now I've used the env_vars parameter for the postfix service, but it would be nice to have a more uniform way to pass options around between the two ways of deploying exporters.

dhoppe · 2019-06-05T08:48:37Z

@anarcat Could you please do a rebase?

anarcat · 2019-06-05T13:28:26Z

@dhoppe done, but i'd still need help and guidance on the next steps to move forward here. there are unresolved conversations and questions above I would love to have feedback on. :)

data/Debian.yaml

data/defaults.yaml

manifests/config.pp

manifests/server.pp

anarcat · 2019-06-14T17:11:38Z

I have greatly simplified this by ripping out the permissions tweak in a separate PR (#323) since that is probably relevant to all installs, not just Debian.

I have also removed the Hiera customization that is associated with the Debian package setup. That is trickier, because it's hard to get right for users. To enable the Debian package, they would need to do this chunk:

prometheus::bin_dir: '/usr/bin'
prometheus::shared_dir: '/usr/share/prometheus'
prometheus::install_method: 'package'
prometheus::node_exporter::package_ensure: 'latest'
prometheus::node_exporter::package_name: 'prometheus-node-exporter'
prometheus::node_exporter::service_name: 'prometheus-node-exporter'
prometheus::node_exporter::group: 'prometheus'
prometheus::node_exporter::user: 'prometheus'
prometheus::apache_exporter::package_name: 'prometheus-apache-exporter'
prometheus::apache_exporter::service_name: 'prometheus-apache-exporter'
prometheus::apache_exporter::user: 'prometheus'
prometheus::apache_exporter::group: 'prometheus'
prometheus::configname: 'prometheus.yml'
prometheus::server::user: 'root'
prometheus::server::group: 'root'
prometheus::postgres_exporter::package_name: 'prometheus-postgres-exporter'
prometheus::postgres_exporter::service_name: 'prometheus-postgres-exporter'
prometheus::postgres_exporter::user: 'prometheus'
prometheus::postgres_exporter::group: 'prometheus'

... I think you'll agree this is preeeetty annoying and not very practical. I am not sure how best to do this cleanly without flipping the default install method on Debian, to be honest, since we can't have conditionals in YAML (or can we?).

Would love to have a better way to do this, but failing that, this will have to do for us for now, because i'd really like to see at least some of this get through.

bastelfreak · 2020-01-31T21:22:03Z

and some more:

diff --git a/manifests/config.pp b/manifests/config.pp
index 8129672..bbbbe1f 100644
--- a/manifests/config.pp
+++ b/manifests/config.pp
@@ -164,7 +164,7 @@ class prometheus::config {
       mode    => '0644',
       owner   => 'root',
       group   => '0', # Darwin uses wheel
-      content => "ARGS='${join($daemon_flags, ' ')}'\n",
+      content => "ARGS='${join(sort($daemon_flags), ' ')}'\n",
     }
   }
 
@@ -192,7 +192,7 @@ class prometheus::config {
         content => epp("${module_name}/prometheus.systemd.epp", {
           'user'           => $prometheus::server::user,
           'group'          => $prometheus::server::group,
-          'daemon_flags'   => $daemon_flags,
+          'daemon_flags'   => $daemon_flags.sort,
           'max_open_files' => $max_open_files,
           'bin_dir'        => $prometheus::server::bin_dir,
           'env_file_path'  => $prometheus::server::env_file_path,
diff --git a/spec/fixtures/files/prometheus.default b/spec/fixtures/files/prometheus.default
index c2f631e..e68d2f6 100644
--- a/spec/fixtures/files/prometheus.default
+++ b/spec/fixtures/files/prometheus.default
@@ -1 +1 @@
-ARGS='--config.file=/etc/prometheus/prometheus.yaml --storage.tsdb.path=/var/lib/prometheus --storage.tsdb.retention=360h --web.console.templates=/usr/local/share/prometheus/consoles --web.console.libraries=/usr/local/share/prometheus/console_libraries'
+ARGS='--config.file=/etc/prometheus/prometheus.yaml --storage.tsdb.path=/var/lib/prometheus --storage.tsdb.retention=360h --web.console.libraries=/usr/local/share/prometheus/console_libraries --web.console.templates=/usr/local/share/prometheus/consoles'

manifests/config.pp

anarcat · 2020-01-31T21:34:58Z

this fixes most of the tests:

committed.

and some more:

committed.

vox-pupuli-tasks · 2020-08-01T14:56:31Z

Dear @anarcat, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

Most of the other exporters have such a setting. It's important to make this work in Debian, where the service name is `prometheus-apache-exporter` and not `apache_exporter`. This was extracted from the larger Debian PR in voxpupuli#303.

Without this change, when the install_method is set to `package`, the various `prometheus::server` settings will never do anything because we stop touching the systemd config file. By default, the Debian systemd.service file has this entry: EnvironmentFile=/etc/default/prometheus ExecStart=/usr/bin/prometheus $ARGS ... which makes it use the ARGS define in the "default" config file. We harmonize our systemd config and init.d files to sync up with that file instead of rewriting the startup file. Closes: voxpupuli#323.

We still need it in our debian voodoo stuff.

Without this change, when the install_method is set to `package` and init_style is set to `none`, the various `prometheus::server` settings will never do anything because we stop touching the systemd config file. By deploying the env_file_path on the server, we allow the "Debian package" setup (install_method=package and init_style=none) to propagate the daemon_flags correctly. By default, the Debian systemd.service file has this entry: EnvironmentFile=/etc/default/prometheus ExecStart=/usr/bin/prometheus $ARGS ... which makes it use the ARGS define in the "default" config file. This is a subset of voxpupuli#303, which should hopefully pass tests and be more manageable. Closes: voxpupuli#323

We need the env_vars for the Debian package to work properly. In Debian-land, the systemd `.service` file gets its parameters from the EnvironmentFile, which we deploy through the `env_vars` here. Yes, it's backwards, and no, it's not great because it doesn't actually use the `options` stuff, but at least it works. This is necessary to have those exporters be able to accept options when using init_style=none and install_method=package. This is a split off of voxpupuli#303 which should at least pass tests (/probably/ because the `install_method=package` code path is basically untested).

anarcat · 2021-02-23T16:34:21Z

alright, i split this up in two smaller PRs: #527 and #528. Neither touches the test suite, which means the tests will probably pass, but that's kind of a little white lie: the code they affect (install_method=package and init_style=none) is basically untested anyways.

but at least it should be mergeable, and should form a better basis to fix #32 in the long run than this too old and complicated PR. i'll regroup on those and summarize progress in #32 instead of keeping this old thing alive.

Most of the other exporters have such a setting. It's important to make this work in Debian, where the service name is `prometheus-apache-exporter` and not `apache_exporter`. This was extracted from the larger Debian PR in voxpupuli#303.

Without this change, when the install_method is set to `package` and init_style is set to `none`, the various `prometheus::server` settings will never do anything because we stop touching the systemd config file. By deploying the env_file_path on the server, we allow the "Debian package" setup (install_method=package and init_style=none) to propagate the daemon_flags correctly. By default, the Debian systemd.service file has this entry: EnvironmentFile=/etc/default/prometheus ExecStart=/usr/bin/prometheus $ARGS ... which makes it use the ARGS define in the "default" config file. This is a subset of voxpupuli#303, which should hopefully pass tests and be more manageable. Closes: voxpupuli#323

This was referenced Mar 12, 2019

Support Debian packages #32

Closed

add ability to export/collect scrape_jobs #304

Merged

anarcat force-pushed the debian-package branch from 423d5da to b8de306 Compare March 19, 2019 21:01

bastelfreak reviewed Mar 22, 2019

View reviewed changes

anarcat mentioned this pull request Apr 8, 2019

postfix exporter support #310

Closed

anarcat mentioned this pull request Apr 11, 2019

Add Bind exporter #312

Merged

bastelfreak added enhancement New feature or request needs-work not ready to merge just yet tests-fail labels Apr 12, 2019

bastelfreak added the needs-rebase label May 30, 2019

anarcat force-pushed the debian-package branch from 113566e to 40d696d Compare June 5, 2019 13:27

dhoppe reviewed Jun 6, 2019

View reviewed changes

data/Debian.yaml Outdated Show resolved Hide resolved

data/defaults.yaml Outdated Show resolved Hide resolved

manifests/config.pp Outdated Show resolved Hide resolved

manifests/server.pp Outdated Show resolved Hide resolved

anarcat force-pushed the debian-package branch from e19b6e0 to 8865d04 Compare June 13, 2019 16:08

This was referenced Jun 14, 2019

env_file_path has no effect on prometheus::server #323

Closed

make config files readonly to daemons #324

Merged

anarcat force-pushed the debian-package branch 2 times, most recently from 683107c to 4146844 Compare June 14, 2019 17:08

anarcat force-pushed the debian-package branch 2 times, most recently from 1978fb8 to fe13476 Compare January 31, 2020 21:19

bastelfreak reviewed Jan 31, 2020

View reviewed changes

manifests/config.pp Outdated Show resolved Hide resolved

anarcat force-pushed the debian-package branch from fe13476 to a1d8415 Compare January 31, 2020 21:29

vox-pupuli-tasks bot added the merge-conflicts label Aug 1, 2020

alexjfisher mentioned this pull request Sep 26, 2020

Daemon: create env files only if required #493

Merged

anarcat force-pushed the debian-package branch 2 times, most recently from dbb8f79 to f850f89 Compare February 22, 2021 21:51

anarcat removed merge-conflicts needs-rebase labels Feb 22, 2021

anarcat and others added 6 commits February 23, 2021 09:48

fix tests for split config setup

b6c4149

revert the env_vars removal

f9403a9

We still need it in our debian voodoo stuff.

Add env_vars to blackbox_exporter to pass to the daemon

81d3378

shorten truth check

79d00bb

sort the daemon flags

8c2adf9

anarcat force-pushed the debian-package branch from f850f89 to 8c2adf9 Compare February 23, 2021 15:07

anarcat mentioned this pull request Feb 23, 2021

deploy env_file_path on server #527

Merged

anarcat mentioned this pull request Feb 23, 2021

revert the env_vars removal for two exporters #528

Closed

anarcat closed this Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Debian package configuration #303

add Debian package configuration #303

anarcat commented Mar 12, 2019 •

edited

Loading

bastelfreak commented Mar 19, 2019

anarcat commented Mar 19, 2019

anarcat commented Mar 19, 2019

bastelfreak commented Mar 20, 2019

anarcat commented Mar 20, 2019

bastelfreak commented Mar 21, 2019

anarcat commented Mar 21, 2019

anarcat commented Mar 21, 2019 •

edited

Loading

bastelfreak Mar 22, 2019 •

edited

Loading

anarcat Mar 22, 2019

anarcat commented Apr 3, 2019

anarcat commented Apr 8, 2019

dhoppe commented Jun 5, 2019

anarcat commented Jun 5, 2019

anarcat commented Jun 14, 2019

bastelfreak commented Jan 31, 2020

anarcat commented Jan 31, 2020

vox-pupuli-tasks bot commented Aug 1, 2020

anarcat commented Feb 23, 2021

add Debian package configuration #303

add Debian package configuration #303

Conversation

anarcat commented Mar 12, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Remaining work

bastelfreak commented Mar 19, 2019

anarcat commented Mar 19, 2019

anarcat commented Mar 19, 2019

bastelfreak commented Mar 20, 2019

anarcat commented Mar 20, 2019

bastelfreak commented Mar 21, 2019

anarcat commented Mar 21, 2019

anarcat commented Mar 21, 2019 • edited Loading

bastelfreak Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

anarcat Mar 22, 2019

Choose a reason for hiding this comment

anarcat commented Apr 3, 2019

anarcat commented Apr 8, 2019

dhoppe commented Jun 5, 2019

anarcat commented Jun 5, 2019

anarcat commented Jun 14, 2019

bastelfreak commented Jan 31, 2020

anarcat commented Jan 31, 2020

vox-pupuli-tasks bot commented Aug 1, 2020

anarcat commented Feb 23, 2021

anarcat commented Mar 12, 2019 •

edited

Loading

anarcat commented Mar 21, 2019 •

edited

Loading

bastelfreak Mar 22, 2019 •

edited

Loading