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

Add pod monitors and remove old service based monitoring #246

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

uvegla
Copy link
Contributor

@uvegla uvegla commented Feb 12, 2024

No description provided.

@uvegla uvegla marked this pull request as ready for review February 12, 2024 14:28
@uvegla uvegla requested a review from a team as a code owner February 12, 2024 14:28
@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why is false by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure, if it is guaranteed that every cluster you install the chart to has pod monitors. If so, I am happy to make it true by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes we have them on all clusters greater than 18.2

Choose a reason for hiding this comment

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

Well, most of them have pod monitors. It appeared around release 18 IIRC.
So I'd set this value as true by default, with the possibility of disabling for old releases.

But if you want to keep compatibility with old clusters, you may want to keep the service then, rather than completely disabling metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workload clusters included? (As customers use the chart as well) Changing to default true then. 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KVM is frozen on an old MCB and therefor Flux release, so probably not an issue for really old ones.

Choose a reason for hiding this comment

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

Yes, WCs > 18.2 have the prometheus-agent running, and support podmons.

Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

If you've tested that then LGTM :)

@uvegla uvegla merged commit 7869e7f into master Feb 13, 2024
4 checks passed
@uvegla uvegla deleted the add-pod-monitors branch February 13, 2024 09:12
@uvegla uvegla changed the title Add pod monitors Add pod monitors and remove old service based monitoring Feb 13, 2024
Copy link
Contributor

@mproffitt mproffitt left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants