Allow disabling of ring heartbeats by setting relevant options to zero.

[stevesg]

What this PR does:
Allow disabling of ring heartbeats by setting relevant options to zero.

Depends on #4342

Checklist

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

[pracucci]

The solution you've picked should work.

Initially, I thought that we should not set Timestamp when the instance is registered in the ring and we should not update it when an InstanceDesc is updated, but then I've realised memberlist actually requires that Timestamp change each time we do update InstanceDesc so we have to keep all that logic and just disabled the hearbeating part.

However, I would update the ring web page to clearly state when heartbeating is disabled.

[tomwilkie]

LGTM;

+1 to Marco's suggestion on including the heartbeat value / disabled on the ring info page.

Not related to this change, but why is there so much duplication of ring config now?

[stevesg]

Initially, I thought that we should not set Timestamp when the instance is registered in the ring and we should not update it when an InstanceDesc is updated, but then I've realised memberlist actually requires that Timestamp change each time we do update InstanceDesc so we have to keep all that logic and just disabled the hearbeating part.

I had the same worry that memberlist relies on the Timestamp to resolve conflicts but it looks that the Timestamp is updated whenever the ring state is updated (example). This makes sense as in my testing with very long heartbeats, I didn't encounter any issues.

Will update the status page

[stevesg]

It doesn't seem trivial to add the heartbeat period to the status page, as the configuration actually exists in the lifecycler :/ Any suggestions?

[pracucci]

It doesn't seem trivial to add the heartbeat period to the status page, as the configuration actually exists in the lifecycler :/ Any suggestions?

The easiest (but probably) hackiest one is to add the heartbeat period to ring.Config too, but not registered via a CLI flag and not exposed via YAML. Then, in the code, we do set it based on the setting coming from the lifecycler.

[bboreham]

Not related to this change, but why is there so much duplication of ring config now?

Can't speak to the history but I agree there is a lot of duplication?
Maybe because the first variant didn't want the "lifecycler" behaviour?

[pracucci]

Not related to this change, but why is there so much duplication of ring config now?

Can't speak to the history but I agree there is a lot of duplication?
Maybe because the first variant didn't want the "lifecycler" behaviour?

I'm the guilty one. Long time ago (when we built the blocks storage) we duplicated (once) the ring config to slim it down. The ring.Config has a bunch of options we didn't need for the compactor so to make the config more clear (instead of reusing the same config and having many options not working for compactor) we duplicated it. The same pattern than has been applied for other rings. It's something we can definitely cleanup.

[pracucci]

The improvement to the HTTP ring page can be done in a follow up PR.

[edma2]

I'd love to learn more about the motivation behind this PR (and #4342). I couldn't find much docs on this feature. Does this improve availability by ignoring heartbeat failures?

[bboreham]

Updating a KV store every few seconds is difficult to scale past 100-200 ingesters.
As of #741 we have a heartbeat directly from distributors to ingesters, so the one via the ring should not be necessary.

Some of the story is here: https://grafana.com/blog/2020/02/11/how-were-abusing-hashicorps-consul-at-grafana-labs/