ctrl:rule: Add ability to configure with PrometheusRule CRDs #167
Conversation
saswatamcode
force-pushed
the
prom-rule
branch
from
c212e5d to
6e31754
Compare
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
saswatamcode
force-pushed
the
prom-rule
branch
from
6e31754 to
6666c72
Compare
Signed-off-by: Saswata Mukherjee <[email protected]>
saswatamcode
force-pushed
the
prom-rule
branch
from
aca53ff to
79e214b
Compare
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
|
This looks good overall. I wonder should we add it to the feature gates though? Even if we want to enable it by default that would be fine, what do you think? The main issue I have is that we have introduced a client for a GVK that may not be present in the cluster. See https://github.com/thanos-community/thanos-operator/blob/main/cmd/main.go#L87 where we have added that feature as opt-out, perhaps we could even lump the prom operator apis under a single flag? |
|
That's a good point, let me try single flag approach |
…ator CRD Signed-off-by: Saswata Mukherjee <[email protected]>
| flag.BoolVar(&featureGatePrometheusOperator, "feature-gate.enable-service-monitors", true, | ||
| "If set, the operator will manage ServiceMonitors for Prometheus Operator") | ||
| flag.BoolVar(&featureGatePrometheusOperator, "feature-gate.enable-prometheus-operator-crds", true, | ||
| "If set, the operator will manage ServiceMonitors for components it deploys, and discover PrometheusRule objects to set on Thanos Ruler, from Prometheus Operator.") |
There was a problem hiding this comment.
this is good for now, but probably want to move the more detailed description of behaviour into docs at some point and we can keep this brief!
| // Once detected, these rules are made into configmaps and added to the Ruler. | ||
| // +kubebuilder:default:={matchLabels:{"operator.thanos.io/prometheus-rule": "true"}} | ||
| // +kubebuilder:validation:Required | ||
| PrometheusRuleSelector metav1.LabelSelector `json:"prometheusRuleSelector,omitempty"` |
There was a problem hiding this comment.
somethings here seem conflicting
- If not specified, no PrometheusRules will be loaded.
- +kubebuilder:validation:Required
I think it should be made optional and turned into a pointer?
There was a problem hiding this comment.
Right that line shouldn't be there.
The behavior for this, is that will always look for prometheusrule objects and is set to our label by default
But you can customize this label (which is something we would need, as it might be hard for users to update all their existing prometheusrule objects with a specific label.
There was a problem hiding this comment.
Added a rule to validate minimum of one label
| "github.com/thanos-community/thanos-operator/test/utils" | ||
|
|
||
| monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" |
There was a problem hiding this comment.
There was a problem hiding this comment.
I added a -local flag to goimport to prevent this. Hopefully this can be done automatically now :)
|
@saswatamcode looking good generally, just some small comments/nits |
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
|
@philipgough I'm going to merge this as CI is green, comments are addressed and I would like to build a few things on top of this. But feel free to add more comments, will address in future PR! |
This PR adds the ability to detect PrometheusRule CRD and generate configmaps to mount to Thanos Ruler pods via ThanosRuler CRD.
PrometheusRule objects will take priority over ConfigMaps.
For now, rules are limited by size restriction of ConfigMaps (will address in future PR):
A ConfigMap is not designed to hold large chunks of data. The data stored in a ConfigMap cannot exceed 1 MiB.