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

Change SM and PM label selector #18

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

QuentinBisson
Copy link
Contributor

This PR:

  • uses the application.giantswarm.io/team label to discover Service|PodMonitors.

Testing

Tested on a GCP Workload Cluster

Checklist

  • Update changelog in CHANGELOG.md.
  • Make sure values.yaml and values.schema.json are valid.

Signed-off-by: QuentinBisson <[email protected]>
@QuentinBisson QuentinBisson self-assigned this Dec 5, 2022
@QuentinBisson QuentinBisson marked this pull request as ready for review December 5, 2022 21:43
@QuentinBisson QuentinBisson requested a review from a team as a code owner December 5, 2022 21:43
@QuentinBisson
Copy link
Contributor Author

Tested on gohan

podMonitorNamespaceSelector:
matchLabels:
name: kube-system
serviceMonitorNamespaceSelector: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed namespace selector! But I wonder about capi management cluster. The Agent will be able to scrape all SMs

Copy link
Contributor

Choose a reason for hiding this comment

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

All SMs and PMs that have defined application.giantswarm.io/team.
Meaning it should exclude customer-defined SMs and PMs.
Clever! I like it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hervenicol yes exactly. That way we will pick up managed apps service monitors but not the customer workload :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will also help us enforce that this label is set :)

Copy link
Contributor

@hervenicol hervenicol left a comment

Choose a reason for hiding this comment

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

LGTM

@marieroque marieroque self-requested a review December 6, 2022 08:10
@QuentinBisson QuentinBisson merged commit 67ecc43 into main Dec 6, 2022
@QuentinBisson QuentinBisson deleted the configure-servicemonitor-discovery branch December 6, 2022 10:51
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.

5 participants