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

fix skip label check #309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reyreaud-l
Copy link

Memberlist allows having label and to check them on receiving a message to ensure rings don't collide unexpectedly. The option to skip that checks currently doesn't work and will fail the message unless the labels are exactly the same.

This PR change that behaviour to ensure that the option to skip the label check does that.

I tried adding test to showcase that the behaviour is now working as expected. Let me know if you any changes would be necessary. This is my first time contributing to that repository, let me know if I missed any necessary steps <3

Copy link

hashicorp-cla-app bot commented Aug 29, 2024

CLA assistant check
All committers have signed the CLA.

Memberlist allows having label and to check them on receiving a message
to ensure rings don't collide unexpectedly. The option to skip that
checks currently doesn't work and will fail the message unless the
labels are exactly the same.

This PR change that behaviour to ensure that the option to skip the
label check does that.

Signed-off-by: Loic Reyreaud <loic@weaviate.io>
@rboyer
Copy link
Member

rboyer commented Sep 12, 2024

This is working as intended.

SkipInboundLabelCheck is about loosening the requirement that inbound traffic be labeled at all, not that it be labeled correctly. This is expected to be set when the label has been received and removed by a proxy of sorts, before arriving at the memberlist where SkipInboundLabelCheck is set to true. Internally that latter memberlist is configured to know its label and must ensure that the protocol was not violated and accidentally two labels were sent.

@reyreaud-l
Copy link
Author

Hey @rboyer thanks for the feedback. If that's not the use for SkipInboundLabelCheck, how would you recommend doing an HA rollout on a memberlist ring where one is introducing a label ?

For example you have a ring with 2 nodes and no label, you want to add a label to the ring to ensure that this ring will not collide with other rings unexpectedly. You need to do that while keeping at least one node online.

With my change (and the behaviour that I assumed of the flag). You could do

  1. Rollout to node one by one with SkipInboundLabelCheck to true
  2. Rollout with adding the label one by one
  3. Rollout with removing SkipInboundLabelCheck

But that's not working with the current memberlist impl. Would you have any pointers to do it ? Would you be open to adding a new flag that is doing what I'm describing (ie skip all label checks) ?

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.

2 participants