search

saltstack-formulas / prometheus-formula

Manage a Prometheus installation
Other
27 stars 51 forks source link

FreeBSD support restored; textfile collectors; some bugfixes #20

Closed alxwr closed 5 years ago

alxwr commented 5 years ago

Tested with

myii commented 5 years ago

@alxwr Just a heads-up: we're now running yamllint on all of the formulas that have been converted to semantic-release. This PR has tripped some of those lint errors:

$ yamllint -s .
./pillar.example
  38:10     warning  missing starting space in comment  (comments)
  173:17    warning  truthy value should be one of [false, true]  (truthy)
  174:10    warning  missing starting space in comment  (comments)
  176:17    warning  truthy value should be one of [false, true]  (truthy)
  177:10    warning  missing starting space in comment  (comments)
  178:10    warning  missing starting space in comment  (comments)
  179:10    warning  missing starting space in comment  (comments)

./prometheus/defaults.yaml
  125:17    warning  truthy value should be one of [false, true]  (truthy)
  126:17    warning  truthy value should be one of [false, true]  (truthy)
  129:17    warning  truthy value should be one of [false, true]  (truthy)
  130:17    warning  truthy value should be one of [false, true]  (truthy)

./prometheus/osfamilymap.yaml
  39:11     warning  truthy value should be one of [false, true]  (truthy)
alxwr commented 5 years ago

@myii Thanks for the heads-up! @noelmcloughlin Thanks for refactoring the formula! I was a bit overwhelmed by the changes at first, but it's a lot DRYer now. :-) I just hope to have stayed within the concept when I added the textfile collectors.

In https://travis-ci.com/saltstack-formulas/prometheus-formula/jobs/241880163 I got this strange error with cron.present:

Reason: 'cron' __virtual__ returned False: cron module could not be loaded

Has anyone an idea how or why this fails on default-debian-9-2018-3-py2, but works in all other environments?

myii commented 5 years ago

Has anyone an idea how or why this fails on default-debian-9-2018-3-py2, but works in all other environments?

@alxwr The rest of the instances probably have the cron package installed by default. Since we've got debug logging enabled these days, thankfully it's a bit easier to track things down.

https://travis-ci.com/saltstack-formulas/prometheus-formula/jobs/241880163#L1842-L1846

       [DEBUG   ] Could not LazyLoad cron.present: 'cron' __virtual__ returned False: cron module could not be loaded
       [INFO    ] Running state [cd /var/tmp/node_exporter && LANG=C ipmitool sensor | /opt/prometheus/textfile_exporters/ipmitool > .ipmitool.prom$$; mv .ipmitool.prom$$ ipmitool.prom] at time 01:00:15.845535
       [DEBUG   ] LazyLoaded cron.present
       [ERROR   ] State 'cron.present' was not found in SLS 'prometheus.config.node_exporter.textfile_collectors.ipmitool'
       Reason: 'cron' __virtual__ returned False: cron module could not be loaded

https://travis-ci.com/saltstack-formulas/prometheus-formula/jobs/241880163#L1848-L1867

       [INFO    ] Running state [smartmontools] at time 01:00:15.846451
       [INFO    ] Executing state pkg.installed for [smartmontools]

       ...

       [INFO    ] Executing command [u'systemd-run', u'--scope', u'apt-get', u'-q', u'-y', u'-o', u'DPkg::Options::=--force-confold', u'-o', u'DPkg::Options::=--force-confdef', u'install', u'smartmontools'] in directory '/home/kitchen'

       ...

       [INFO    ] Made the following changes:

       ...

       'cron-daemon' changed from 'absent' to '1'

       ...

       'cron' changed from 'absent' to '3.0pl1-128+deb9u1'

https://travis-ci.com/saltstack-formulas/prometheus-formula/jobs/241880163#L1923-L1932

       [DEBUG   ] LazyLoaded cron.present
       [INFO    ] Running state [cd /var/tmp/node_exporter && LANG=C /opt/prometheus/textfile_exporters/smartmon.sh > .smartmon.prom$$ && mv .smartmon.prom$$ smartmon.prom] at time 01:00:22.033639
       [INFO    ] Executing state cron.present for [cd /var/tmp/node_exporter && LANG=C /opt/prometheus/textfile_exporters/smartmon.sh > .smartmon.prom$$ && mv .smartmon.prom$$ smartmon.prom]
       [INFO    ] Executing command 'crontab -l' as user 'root' in directory '/root'
       [DEBUG   ] stderr: no crontab for root

       [DEBUG   ] retcode: 1
       [INFO    ] Executing command 'crontab /tmp/__salt.tmp.AF29sk' as user 'root' in directory '/root'
       [INFO    ] {u'root': u'cd /var/tmp/node_exporter && LANG=C /opt/prometheus/textfile_exporters/smartmon.sh > .smartmon.prom$$ && mv .smartmon.prom$$ smartmon.prom'}
       [INFO    ] Completed state [cd /var/tmp/node_exporter && LANG=C /opt/prometheus/textfile_exporters/smartmon.sh > .smartmon.prom$$ && mv .smartmon.prom$$ smartmon.prom] at time 01:00:22.114639 (duration_in_ms=81.0)

Essentially, you're going to need to ensure that cron is available by this stage.

https://travis-ci.com/saltstack-formulas/prometheus-formula/jobs/241880163#L1277-L1284

       prometheus-exporters-node-textfile_collectors-ipmitool-cronjob:
         cron.present:
           - identifier: prometheus-exporters-node-textfile_collectors-ipmitool-cronjob
           - name: cd /var/tmp/node_exporter && LANG=C ipmitool sensor | /opt/prometheus/textfile_exporters/ipmitool > .ipmitool.prom$$; mv .ipmitool.prom$$ ipmitool.prom
           - minute: "*"
           - comment: Prometheus' node_exporter's ipmitool textfile collector
           - require:
             - file: prometheus-exporters-node-textfile_collectors-ipmitool-script
noelmcloughlin commented 5 years ago

I just hope to have stayed within the concept when I added the textfile collectors.

Hi @alxwr I hope you are okay with the refactoring. In next few months I'm planning on using this formula in some other projects so I needed to do some work on it in preparation. Let me check your PR.

noelmcloughlin commented 5 years ago

Hi @alxwr the __virtual__ returned False error means the underlying linux package is missing. I have seen it few times recently and had to ensure the package was installed.

alxwr commented 5 years ago

I just hope to have stayed within the concept when I added the textfile collectors.

Hi @alxwr I hope you are okay with the refactoring. In next few months I'm planning on using this formula in some other projects so I needed to do some work on it in preparation. Let me check your PR.

Hi @noelmcloughlin I needed some time to understand it. (Maybe that's an indicator both of us should write more comments declaring the intentions and goals of the implementation.) I like the fact that we're now generating our config from Pillar (or possibly any other data source). We'll need to accommodate some quirks of certain distributions tough. (#21)

I'll go through your proposed changes now. :-)

alxwr commented 5 years ago

This is impressive PR, neatly implemented, and nothing controversial ;-)

Thanks! I'm glad, you like it. :-)

I have one major comment that node_exporter dictionary must be moved to promethesu:config dict. Otherwise LGTM.

prometheus:config:node_exporter will be fed into prometheus/files/default/config.yml.jinja. I do want that, because it is confusing. Therefore I opened prometheus:node_exporter. I really don't care where it lies, I just don't want the somewhat ad-hoc implemented textfile exporters to have any side effects on the core of this formula.

alxwr commented 5 years ago

Has anyone an idea how or why this fails on default-debian-9-2018-3-py2, but works in all other environments?

@alxwr The rest of the instances probably have the cron package installed by default. Since we've got debug logging enabled these days, thankfully it's a bit easier to track things down.

@myii Thanks for the analysis! Finally the PR gets through the tests. :-)

alxwr commented 5 years ago

Rebased against master, because #21 was merged.

baby-gnu commented 5 years ago

I removed myself from reviewer since it's a little bit complicated for me as I'm not used to that software and I'm a bad linter ;-)

Regards.

alxwr commented 5 years ago

@noelmcloughlin Moved Pillar prometheus:node_exporter to prometheus:exporters:node_exporter.

alxwr commented 5 years ago

@noelmcloughlin Thanks for the review and feedback!

saltstack-formulas-travis commented 5 years ago

:tada: This PR is included in version 3.2.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: