Ruler: Sync rules when ruler JOINING the ring instead of ACTIVE

[ying-jeanne]

What this PR does

The ruler becomes active in the ring after all its services are started. When the ruler becomes ready it automatically starts "owning" rule groups. So other rules in the ring stop evaluating these rule groups because they now shard to the new ruler.

After becoming ACTIVE, the ruler proceeds to sync the rule groups for all the tenants that it own; to do that it needs to iterate the bucket and fetch the rule groups for all tenants. For larger cells with many tenants the time to iterate the bucket can exceed a minute. And this causes ruler to miss one iteration as a consequence alerts are auto solved. When the second alert arrives another alert notification is sent.

To solve this, instead of sync the rules after ruler becoming ACTIVE in the Ring, sync the rules when ruler JOINING the Ring, and when the other tenant would drop the rule groups once new ruler become ACTIVE.

Which issue(s) this PR fixes or relates to

Fixes #4117

Checklist

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

[dimitarvdimitrov]

Nice work! I have a few minor comments.

I was surprised that there are already tests for a JOINING ruler in TestSharding. Thanks @pstibrany!

Also, I think there's a typo in the PR description

To solve this, instead of sync the rules after ruler becoming ACTIVE in the Ring, sync the rules when ruler JOINING the Ring, and when the other tenant would drop the rule groups once new ruler become ACTIVE.

Shouldn't it be the other rulers?

[56quarters]

This looks great to me but I'll let @dimitarvdimitrov 👍

[dimitarvdimitrov]

Nice work! I left some nitpicks about comments and what args are passed, but other than that LGTM.

I'll leave this for a review from someone in @grafana/mimir-ruler-and-alertmanager-maintainers as well before we merge it.

[dimitarvdimitrov]

Something which we talked about, but didn't write anywhere, is that after this change the likelihood of duplicate samples being written is higher.

The reason is that during a restart, after the new ruler is ACTIVE in the ring, the old rulers will detect a ring change and will start syncing their rules from the bucket. That bucket sync is likely to be slow. With the current implementation on main this was somewhat avoided because both new and old rulers sync the bucket concurrently, whereas with this change the new ruler would have already synced the bucket.

We can probably remediate that by first checking and stopping local rules after a ring change instead of directly syncing from the bucket, but for now I think it might not be a big problem.

[ying-jeanne]

I just reached out to alert, they are ok with the ruler changes, since we didn't change alert manager, this should be fine.

[dimitarvdimitrov]

LGTM, nicely done!

[dimitarvdimitrov]

@56quarters do you want to take another look at the PR before we merge it?

[56quarters]

LGTM