Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: publish refinery instance ID during pubsub peer comms #1417

Closed
wants to merge 5 commits into from

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Nov 8, 2024

Which problem is this PR solving?

When publishing peers via redis pubsub, we need a way to identify whether a node can receive keep and drop decision messages. This is important when performing migrations that update an older deployment that does not support keep/drop decisions to one that does.

Also, it would also be nice to not have to rely purely on a peer address as a means of testing whether the instance if the same for determining whether we need to re-send trace decisions. For example, it's possible the same IP address is used even though the refinery instance has restarted.

This PR updates refinery to generate a new instance ID on start up and then publish the refinery's instance ID as part of it's peer register/unregister commands. As the message format is changing, any messages that does not conform to the new format will not be added as valid peers.

Short description of the changes

  • Generate and inject an instance ID during process start up in main.go
  • Update redis pubsub to take the instance ID via injection
  • Update redis pubsub peer commands to also specify the instance ID
  • Update marshal and unmarshall funcs to handle and require instance ID
  • Update peers to be a set of peerRecord's that has both ID and address
  • Update pubsub tests

@MikeGoldsmith MikeGoldsmith self-assigned this Nov 8, 2024
@MikeGoldsmith MikeGoldsmith requested a review from a team as a code owner November 8, 2024 13:19
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The peer protocol was designed to be small because every pubsub message gets multiplied by N. Hence the 1-character command plus the address, which in most cases is going to be less than 20 characters. This adds 38 characters to it, which roughly triples the amount of data sent. I'm wondering if we could use, say, a 32-bit hash of the UUID instead so it only adds 8 characters?

The other problem, though, is that the new ID is only used as a flag to differentiate from the previous version of this protocol. It is not used to define uniqueness for the set of peers -- only the address does that. So once this rolls out, if a peer falls over and comes back up with the same address but a different ID, this does not do what it claims because the ID is not saved in the peer list.

I think to do what we want, we need to create a simple POD type like this:

type peerRecord struct {
  ID string
  Address string
}

And then the peers member becomes *generics.SetWithTTL[peerRecord].

@kentquirk
Copy link
Contributor

I apologize in advance -- I started playing with this on my own machine, and decided to try writing MapWithTTL, and I liked that better, so I ended up creating #1420 because it was going to be hard to do as a modification to this PR.

If you dislike it we can use this one instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants