Include ring instance ID in InstanceDesc

[56quarters]

What this PR does:

This change adds the ID on an instance in a ring to its InstanceDesc object. This allows callers to access the ID of a member of a ring.

This change also modifies the PoolFactory type to accept an InstanceDesc object when creating a new client instead of only the address. This allows PoolFactory implementations to attach any extra metadata about the instance to metrics/logs/traces for the client.

Which issue(s) this PR fixes:

See grafana/mimir#6128

Checklist

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

[pstibrany]

This looks like a good start, but the change will only apply to new entries that get added to the ring. What about existing entries?

I wonder if we should add some "fixup" method to the *ring.Desc that would fill all ID fields based on instances map, and call that fixup method after reading ring from the KV store (in (*Ring).starting, (*Ring).loop, or GetOrCreateRingDesc). This would also fix writes back to KV, because writes always read from ring first, and write back afterwards.

[56quarters]

This looks like a good start, but the change will only apply to new entries that get added to the ring. What about existing entries?

I wonder if we should add some "fixup" method to the *ring.Desc that would fill all ID fields based on instances map, and call that fixup method after reading ring from the KV store (in (*Ring).starting, (*Ring).loop, or GetOrCreateRingDesc). This would also fix writes back to KV, because writes always read from ring first, and write back afterwards.

Sure, good idea. Is this the type of code that we'll need to keep around indefinitely or something to remove after a while?

[pstibrany]

Sure, good idea. Is this the type of code that we'll need to keep around indefinitely or something to remove after a while?

I don't see a problem with keeping this indefinitely.

[56quarters]

This looks like a good start, but the change will only apply to new entries that get added to the ring. What about existing entries?

Actually from what I can tell looking at the ring code, it seems like pods will add themselves to the ring on start up, updating their existing entry if required. If we make sure ID is set everywhere instances add themselves, do we still need to go through and update the ring.Desc? IDs for entries in the ring would become available after the first time they restart with this new code if I'm understanding correctly.

[56quarters]

I guess explicitly fixing up the ring.Desc means that we have a shorter window of time where InstanceDesc.Id is empty during the first rollout.

[pstibrany]

I guess explicitly fixing up the ring.Desc means that we have a shorter window of time where InstanceDesc.Id is empty during the first rollout.

It would mean that any code using updated dskit can already start using ID field, without relying on other components to re-register in the ring. For example in a mixed environment, where Mimir distributor is updated, but Mimir ingester is not, distributor can already use ID.

[56quarters]

I've tested setting instance IDs based on map keys locally with Mimir:

• Ingesters ran Mimir 2.10 which does not have this change
• All other components ran local image that included these changes

Everything worked as expected, distributors had access to the ingester IDs when creating clients for the ingesters.

I'm going to test in a dev cluster, only using this new code in distributors.

[pstibrany]

Thank you! The changes look good. I've verified that both lifecyclers update its instance's ID field in the ring.

BasicLifecycler makes sure to update ID of its entry in the ring in the registerInstance method, even if entry already exists.

Lifecycler uses initRing to add entry to the ring. But if the entry already exists and it's in the JOINING state, entry's ID field will not be updated. However it will be updated later during heartbeating.

I'd suggest to add changelog entry mentioning changes in the client package as well.