search

canonical / prometheus-k8s-operator

https://charmhub.io/prometheus-k8s
Apache License 2.0
21 stars 35 forks source link

Add leader guards for the aggregator #464

Closed rbarry82 closed 1 year ago

rbarry82 commented 1 year ago

Issue

Set leader guards in MetricsEndpointAggregator so the proxy can scale.

benhoyt commented 1 year ago

@PietroPasotti Yeah, that approach seems simpler to me too: only instantiate/set up if it's the leader in the first place to avoid the proliferation of is_leader checks. But then again, if it's a library, that might make usage of the library more error-prone: you have to remind people to only set it up in an is_leader check. Whereas if the library does that itself they can just always set it up (which is presumably what's happening here). Hmm, it's not obvious to me which is a better approach.

rbarry82 commented 1 year ago

I definitely agree that it's a nicer model in principle for leader-only apps. This library/class is a particularly terrible use case for that, due to the amount of methods in the library which are called into to twiddle data as a "fan-in" and conversion. Even a leader guard on the appropriate method wouldn't quite work, because the per-unit lookaside CSV to keep track of the "real" names of {some_ip_address} NRPE endpoints wouldn't get recorded, and a change in leadership would lead to a potential drop.

There are definitely cases where that could work. It's a little hard to recommend it as a pattern, since new authors may not be aware of when it is/is not a good idea for their library until they get to a v1 refactor without having very complete knowledge of charm workflows/lifecycles. The "per-method guard" as an iterative practice is a safer recommendation, even if not necessarily a better one.