Use standard label selectors in target allocator config #2564
Conversation
dd73dd6 to
941ac15
Compare
| @@ -68,10 +68,20 @@ func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) { | |||
| prometheusCRConfig["scrape_interval"] = params.OtelCol.Spec.TargetAllocator.PrometheusCR.ScrapeInterval.Duration | |||
| } | |||
|
|
|||
| prometheusCRConfig["service_monitor_selector"] = &metav1.LabelSelector{ | |||
There was a problem hiding this comment.
just to confirm my understanding here... by not having the nil check we follow what prometheus expects i.e. when service/pod monitor selector is nil nothing will be selected? whereas before we just didn't set it and assumed everything?
There was a problem hiding this comment.
do we have a test in the configmap_test.go for this new nil behavior?
There was a problem hiding this comment.
just to confirm my understanding here... by not having the nil check we follow what prometheus expects i.e. when service/pod monitor selector is nil nothing will be selected? whereas before we just didn't set it and assumed everything?
Prometheus' (and LabelSelector in general) behaviour is that nil == nothing and empty == everything. We on the other hand, took the map being empty or nil to mean everything, and there wasn't any way to set it to be nothing.
I've modified the configmap tests (and the builder and reconciliation tests) to show the empty label selectors. E2E tests check that this works as well - I know because I had a bug here earlier, and they caught it.
There was a problem hiding this comment.
In a sense, I'm moving the logic from target allocator here. If you look at the target allocator changes, it used to have this for the Prometheus Spec:
ServiceMonitorSelector: &metav1.LabelSelector{
MatchLabels: cfg.ServiceMonitorSelector,
},Now we have this here, accomplishing the same thing.
There was a problem hiding this comment.
gotcha, thanks for explaining.
…ry#2564) * Use standard label selectors in target allocator config * Move all CR selectors in TA config to the right section
Description:
Use standard label selectors in target allocator config instead of plain maps. This is a step towards resolving #1907 in
v1alpha2CRs, by using these selectors in the CRDs themselves as well.I'd wanted to keep backwards compatibility with older target allocator versions to make upgrades easier, so I couldn't simply change the types of existing config fields. Instead, I decided to move all of these fields under the
prometheus_crsection in the configuration, which imo is the more intuitive location for it anyway.There's also a difference in semantics between our current
v1alpha1CRD and standard behaviour for these selectors that I needed to handle. For selectors:nilselector means "select nothing", which is also the default in prometheus-operator CRs{}selector means "select everything"However, for the label maps in our
v1alpha1CRD, an empty or nil map means "select everything". As a result, I needed to make sure we put an empty selector in the configuration in that case. This is the reason they appear in the modified test code. Inv1alpha2, we can change this behaviour to match prometheus-operator.Link to tracking Issue: #1907
Testing:
Modified existing unit tests. Existing E2E tests cover the functionality.