search

saltstack-formulas / prometheus-formula

Manage a Prometheus installation
Other
27 stars 51 forks source link

Fix/environ correction #60

Closed B1ue-W01f closed 3 years ago

B1ue-W01f commented 3 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

59

Describe the changes you're proposing

Theres a few commits and some of them undo changes made that I later realised weren't neccesary, apologies. Primarily its regarding an update to the concat_environ used in the config.environ state. Comments have been added across the default/pillar.example files to guide users to the differences in approach between archive and repo install methods. Comments have been added to the default and repo pillar files to highlight the different intent. Tests added to ensure the environ args are being added to the required files for node_exporter and prometheus, along with pillar values being added to the repo pillar.

Pillar / config required to test the proposed changes

The salt/pillar/repo.sls is ultimately the only pillar file with pillar changes and includes those required to identify the correction.

Debug log showing how the proposed changes work

Tests added to the repo inspec check. Ive verified through the deployed prometheus server the log.level=debug is being passed to its command line flags.

Documentation checklist

Testing checklist

Additional context

None.

B1ue-W01f commented 3 years ago

Debian issue appears to be due to the --collector.systemd config argument being unavailable in the older packages.

The formula handles either scenario so switching it out from the test pillar for --log.level=debug instead

B1ue-W01f commented 3 years ago

If null args is passed then ARGS="" .. is that valid configuration?

        environ:
          args: {}

Yes, that's the configuration that ships with the package.

B1ue-W01f commented 3 years ago

@noelmcloughlin ah its not the required entry for oracle linux! It appears to be ALERTMANAGER_OPTS="" format for oracle linux, hence the failing tests

B1ue-W01f commented 3 years ago

Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar.

Leaving two approaches then: Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex. Exclude centos/oracle from deploying the args file and note the lack of feature support.

Thoughts or any better approach?

noelmcloughlin commented 3 years ago

I guess adding support involves creating a custom pillar file in tests/salt/pillars (or equivalent), updating kitchen.yaml with specific for each custom args needed. It's probably not major work, just something needed here. I don't understand why particular distros are changing format of configuration files, complicating things.

On Fri, Jun 25, 2021 at 1:36 PM B1ue-W01f @.***> wrote:

Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar.

Leaving two approaches then: Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex. Exclude centos/oracle from deploying the args file and note the lack of feature support.

Thoughts or any better approach?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/prometheus-formula/pull/60#issuecomment-868469107, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQSXCHU2P3IXXNTFFEDTURZ6JANCNFSM47JSWSSA .

B1ue-W01f commented 3 years ago

I guess adding support involves creating a custom pillar file in tests/salt/pillars (or equivalent), updating kitchen.yaml with specific for each custom args needed. It's probably not major work, just something needed here. I don't understand why particular distros are changing format of configuration files, complicating things. … On Fri, Jun 25, 2021 at 1:36 PM B1ue-W01f @.***> wrote: Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar. Leaving two approaches then: Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex. Exclude centos/oracle from deploying the args file and note the lack of feature support. Thoughts or any better approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQSXCHU2P3IXXNTFFEDTURZ6JANCNFSM47JSWSSA .

Yeah, unfortunately, its a different name for each file though i.e. ALERTMANAGER_OPTS, PROMETHEUS_OPTS etc which is going to have to be passed into the file.

Going to need something like: 

node_exporter:
  environ:
    environ_arg_name: NODE_EXPORTER_OPTS

for centos and:

node_exporter:
  environ:
    environ_arg_name: ARGS

for default, I guess, per component.

Pretty annoying. Ill take a look later on tonight.

B1ue-W01f commented 3 years ago

@noelmcloughlin let me know if you want anything changed but that's the handling for contos and oraclelinux added. Also part fix for arch linux failing previously - enough to get exporters going and be close to a fix for the main server. IMO there's a few issues with the source package and can see part is identified as a bug here so likely best to leave it as is for now.

B1ue-W01f commented 3 years ago

commit appears to fully comply with the failing lint error so intending to leave as is.

noelmcloughlin commented 3 years ago

CI is failing:

⧗   input: fix: rework to implement environment variables handling
⚠   footer must have leading blank line [footer-leading-blank]

What is the intention with storage.sls as its new file?

B1ue-W01f commented 3 years ago

CI is failing:

⧗   input: fix: rework to implement environment variables handling
⚠   footer must have leading blank line [footer-leading-blank]

What is the intention with storage.sls as its new file?

I cant identify the actual reason the CI failed there, the footer had a preceding blank line?

The storage.sls is to ensure correct user control of the data directory from storage.tsdb.path ; I don't believe the formula does this previously / couldn't find it, and the arch linux build partially fails for that reason currently. For the most part, it doesn't affect anything but it would avoid failures if the parameter is specified but ownership was not otherwise assigned.

noelmcloughlin commented 3 years ago

The commit probably has some sequence of chars which is breaking commit lint: https://github.com/conventional-changelog/commitlint/issues/896 Can you rebase and edit that commit, otherwise, it fails CI every time. Just simplify it if necessary

B1ue-W01f commented 3 years ago

@noelmcloughlin Hey, are you happy to accept? Or anything else I can do to improve?

noelmcloughlin commented 3 years ago

thanks @B1ue-W01f

saltstack-formulas-travis commented 3 years ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

myii commented 3 years ago

@B1ue-W01f I've got a question for you: is there a specific reason why you added .bundle/ to the ignores list in the .yamllint file?

https://github.com/saltstack-formulas/prometheus-formula/blob/2c572d0019fbbac51e2a5cf70b8928496be2cae9/.yamllint#L15

This file is managed across ~100 formulas in our organisation so it will end up being reset when the next standardisation takes place -- unless there's a good reason for it, of course. I'm not sure how YAML files will end up under .bundle/ in any situation. I would be interested to hear otherwise. Thanks.

B1ue-W01f commented 3 years ago

@myii Quite possibly not a good reason, first time working with your template.

But without that yamlint was going down into the bundle directory, such as:

image

Which does appear to contain .yml files across a large number of bundles.

myii commented 3 years ago

@myii Quite possibly not a good reason, first time working with your template.

But without that yamlint was going down into the bundle directory, such as:

image

Which does appear to contain .yml files across a large number of bundles.

@B1ue-W01f That's an excellent justification for it, I'll add that to the list of ignores across the org. Personally, I use rbenv to manage my gems (i.e. I don't install them locally) -- but some will do that, as you have, and our .yamllint configuration needs to handle that.

Appreciate your time taken to explain that!