ruler: ignore rulers in non-operation states when getting and syncing rules

[fionaliao]

What this PR does

Currently, forEachRulerInTheRing() fails if there are rulers in the ring in states other than the states in the operation argument. This is because forEachRulerInTheRing() calls GetReplicationSetForOperation(). Any ruler in a non-operation state is seen as unhealthy by GetReplicationSetForOperation(). As RF = 1, the number of healthy instances must match the total number of instances in the ring. Therefore GetReplicationSetForOperation() errors and that's propagated to forEachRulerInTheRing().

However, it's possible for all rules to be processed (evaluated or synced) correctly even when there are "unhealthy" instances. This is because rules are only processed on healthy instances and are moved around rulers depending on the rulers that are healthy. This causes some endpoints to return errors more often than necessary.

As a concrete example, api/v1/rules and /api/v1/alerts will return errors if there are any JOINING or LEAVING rulers in the ring. Rulers can be in these state during rollouts, or scaleups and scaledowns. These endpoints use GetRules() which calls forEachRulerInTheRing() with RuleEvalRingOp. RuleEvalRingOp only considers ACTIVE rulers as healthy.

Instead of erroring when there are non-ACTIVE rulers in the ring, it's safe to ignore non-ACTIVE rulers in this case:

• A ruler does not start evaluation rules until it's transitioned from JOINING to ACTIVE. And ACTIVE rulers ignore JOINING rulers when deciding which rules to execute. So JOINING rulers can be ignored when querying for rules.
• When a ruler is stopped, it stops all rule evaluations before it transitions to the LEAVING state - as per this code, the rules manager is stopped and then the subservices (including the lifecycler) are stopped. This means that if a ruler's in the LEAVING state it can't be evaluating rules, so it can safely ignored when getting rules.
• It can take other rules a while to detect a ruler is LEAVING or has left the ring, so there can be a pause in rule evaluation for some rules in this time. This happens with the current implementation anyway; filtering for only ACTIVE instances won't make this worse.

The ring can be eventually consistent (e.g. if using memberlist). Therefore rulers could transition between states and redistribute rules during the process of getting rules (e.g. ruler is JOINING but transitions to ACTIVE and rulers are redistributed). So it's possible to get a partial set of results if rulers in other states are ignored. But there are already similar cases happening now, like if a ruler is not present in the ring when we get the list of rulers to get rules from, but joins the ring and becomes active while getting rules.

To implement this, I've introduced a new ring function, GetSubringForOperationStates() in grafana/dskit#699. This is used to filter out the instances in the non-operation states (for the GetRules() case, this will filter out all the non-ACTIVE instances) before GetReplicationSetForOperation() is called. The effect is that GetReplicationSetForOperation()` will only error if there are instances in the operation states that have timed out heartbeats.

I also considered using GetAllHealthy(), rather than introducing a new function. However, GetAllHealthy() ignores out rulers that are in the requested states but have timed out heartbeats rather than than erroring. While rulers being in JOINING and LEAVING states are expected from time to time due to rollouts and scaling, if everything is working as expected, active rulers should not end up with heartbeat timeouts.

Another alternative was to add retries to forEachRulerInTheRing() - we don't expect the ring to always have rulers in the non-operation states. However, in the case of GetRules(), rulers can take a long time to transition from JOINING to ACTIVE if there are many rules to load, so retries would either be ineffective or have high latency.

As well as GetRules(), forEachRulerInTheRing() is used by notifySyncRules(), which notifies all the JOINING and ACTIVE rulers to sync rules. Previously this would fail if any rulers were in the LEAVING state before notifying any ruler to sync rules. It's safe to not notify LEAVING rulers as they do not pick up new rules. This new behaviour ends up being an improvement as other rulers will still be notified when there are LEAVING rulers.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[fionaliao]

Wanted to check if the result of this test case is expected - when getting the shuffle sharded subring for a user, non-ACTIVE rulers are not filtered out. Rules do end up being evaluated on the ACTIVE rulers in the subring though.

The issue I see is that it's possible during rollout or scaling for all the rulers in a shuffle sharded subring to be in a non-ACTIVE state, in which case all rules will not be evaluated for a user.

This was happening before my PR as well. This could be fixed (if it is an issue) by moving GetSubringForOperationStates(op) before calling ShuffleShard() on the ring, so the shuffle sharded ring is made from only the instances in the requested state. Would need to be careful to update the logic in all the places that gets the shuffled sharded subring.

[pracucci]

I was looking exactly at this test and then I noticed your comment. I'm glad we both put our 👀 on this.

I agree this behaviour in related from your PR. Is it desired? It depends. Generally speaking, this is the expected behaviour of shuffle sharding in ingesters (where shuffle sharding has been introduced at the beginning): the goal is to not reshard series among different ingesters during a rolling update. I also think it's a desired property for the store-gateways shuffle sharding.

Is it desired for rulers? I don't know right now (I need to think about it more). Synching rules take time. Given rulers unregister from the ring during a restart, changing this logic would realistically just cover the time it takes for the initial sync (when the ruler is in JOINING state). Moving rule groups to the remaining ACTIVE ruler in the shard (if any) or to another ruler outside the original tenant's shard may not make much difference. The only real difference may be in case the shard size = 1 (if the shard is composed by the JOINING ruler, then no rules would be evaluated while JOINING). However, synching rules on other rules will take time too, so not sure would be a significant improvement changing this logic.

[pracucci]

I love 1 line logic changes. Thanks!