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

feat(TargetAllocator): allow configuration of PrometheusCR namespaceSelectors #3573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dexter0195
Copy link
Contributor

Description:
Adding the possibility to set values for PrometheusCR:

  • probeNamespaceSelector
  • serviceMonitorNamespaceSelector
  • scrapeConfigNamespaceSelector
  • probeNamespaceSelector

Link to tracking Issue(s): #3086

Testing: Added tests for configmap generation

Documentation: Updated README.md

@dexter0195 dexter0195 requested a review from a team as a code owner December 23, 2024 16:37
Copy link

linux-foundation-easycla bot commented Dec 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dexter0195 dexter0195 force-pushed the feat/3086-allow-configuration-prometheus-cr branch from a40ec60 to 8721b68 Compare December 23, 2024 16:43
@dexter0195 dexter0195 changed the title Feat/3086 allow configuration prometheus cr feat(PrometheusCR): allow configuration of namespaceSelectors Dec 23, 2024
@dexter0195 dexter0195 changed the title feat(PrometheusCR): allow configuration of namespaceSelectors feat(TargetAllocator): allow configuration of PrometheusCR namespaceSelectors Dec 23, 2024
@dexter0195 dexter0195 force-pushed the feat/3086-allow-configuration-prometheus-cr branch from 712162d to a539185 Compare December 23, 2024 18:50
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

Change looks good overall, just one thought on how to prevent it from breaking. cc @swiatekm

Comment on lines 108 to 123
prometheusCRConfig["service_monitor_selector"] = taSpec.PrometheusCR.ServiceMonitorSelector

prometheusCRConfig["service_monitor_namespace_selector"] = taSpec.PrometheusCR.ServiceMonitorNamespaceSelector

prometheusCRConfig["pod_monitor_selector"] = taSpec.PrometheusCR.PodMonitorSelector

prometheusCRConfig["pod_monitor_namespace_selector"] = taSpec.PrometheusCR.PodMonitorNamespaceSelector

prometheusCRConfig["scrape_config_selector"] = taSpec.PrometheusCR.ScrapeConfigSelector

prometheusCRConfig["scrape_config_namespace_selector"] = taSpec.PrometheusCR.ScrapeConfigNamespaceSelector

prometheusCRConfig["probe_selector"] = taSpec.PrometheusCR.ProbeSelector

prometheusCRConfig["probe_namespace_selector"] = taSpec.PrometheusCR.ProbeNamespaceSelector

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: we can remove the excess spacing here, it's superfluous IMO.

// matchExpressions are ANDed. An empty label selector matches all objects. A null
// label selector matches no objects.
// +optional
PodMonitorNamespaceSelector *metav1.LabelSelector `json:"podMonitorNamespaceSelector,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should also do some defaulting here to prevent an upgrade from breaking existing installations? As the doc string states, a null selector matches nothing, upgrading with this on will mean that existing installations (who do not set this currently) would not have it, resulting in a null selector, aka 💥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point 👍 , should we implement that for the PodMonitorNamespaceSelector or for all the selectors (including the already existing ones) ? What do you recommend ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should set the default to the empty selector for all selectors you're adding here. The end result will be that resource selectors target nothing by default, while namespace selectors target everything, which is also what prometheus-operator does.

I'm not sure if you can set these defaults via a kubebuilder marker. If not, then you'll have to do it in the webhook.

Copy link
Contributor Author

@dexter0195 dexter0195 Dec 28, 2024

Choose a reason for hiding this comment

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

Hey @swiatekm , thank you for the heads up for the kubebuilder 😃 I indeed found a way to set a default and I added it to the new fields like so

// +kubebuilder:default:={}

which resulted in a generated crd like this:

...
                      podMonitorNamespaceSelector:
                        default: {}
                        properties:
                          matchExpressions:
...

Let me know if that looks good for you 🙏

@dexter0195 dexter0195 force-pushed the feat/3086-allow-configuration-prometheus-cr branch from e2dbc42 to 4f9c31c Compare December 28, 2024 16:27
@dexter0195 dexter0195 force-pushed the feat/3086-allow-configuration-prometheus-cr branch from 4f9c31c to db97808 Compare December 28, 2024 16:30
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.

Unable to run the targetAllocator in namespace mode
3 participants