Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use recommended instance label for Prometheus/Alertmanager resources #1520

Conversation

maxbrunet
Copy link
Contributor

@maxbrunet maxbrunet commented Nov 23, 2021

Description

This sets the recommmended app.kubernetes.io/instance label on all Prometheus/Alertmanager component resources, and removes the legacy prometheus/alertmanager ones.

Prometheus already had the instance label in selectorLabels, Alertmanager did not. Is there any concern about having it in commonLabels for both? I have tested these labels in my setup without issues.

For reference, the recommended labels were added in prometheus-operator/prometheus-operator#3841, this is in the continuity of #832

Type of change

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

- Use recommended instance label for Prometheus/Alertmanager resources

@maxbrunet maxbrunet force-pushed the feature/recommended-instance-label branch from bed87a8 to 5e375b3 Compare November 23, 2021 21:12
Copy link
Member

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Thanks for cleaning this up, now the code looks much cleaner!

One nit: let's mark this as a CHANGE in PR description as this is changing selectors and thus prevents clean upgrade.


Is there any concern about having it in commonLabels for both?

This label should be in commonLabels in every component (prometheus, alertmanager, node-exporter, etc.).

selectorLabels should contain the same labels as commonLabels minus ones that mutate between upgrades (like version)

@maxbrunet
Copy link
Contributor Author

One nit: let's mark this as a CHANGE in PR description as this is changing selectors and thus prevents clean upgrade.

I have updated the description, but this does not prevent clean upgrade as no label is added or removed from the pods and no immutable selector being updated here

@paulfantom
Copy link
Member

no label is added or removed from the pods and no immutable selector being updated here

commonLabels are passed into selectorLabels and only app.kubernetes.io/version is excluded. This means that by adding app.kubernetes.io/instance to commonLabels this PR is changing all selector entries in all prometheus objects. In the end better safe than sorry :)

@paulfantom paulfantom merged commit 974b37e into prometheus-operator:main Nov 25, 2021
@yaacine yaacine mentioned this pull request Nov 25, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants