Receive: Allow setting hashing algorithm per tenant in hashrings config

[haanhvu]

Signed-off-by: haanhvu [email protected]

fixes #5567

• [ ] I added CHANGELOG entry for this change.
• [ ] Change is not relevant to the end user.

Changes

Receive: Allow setting hashing algorithm per tenant in hashrings config

Verification

[haanhvu]

@fpetkovski could you pls review if this is what you mean in the issue?

[haanhvu]

Just thought of this case, currently if users specify the algorithm as Ketama, not ketama, we would consider it invalid and fall back to hashmod? https://github.com/thanos-io/thanos/blob/fb11fc7fcf7178f0564db5b73af595d54a8757f8/pkg/receive/hashring.go#L26-L27

@matej-g @fpetkovski @yeya24

[matej-g]

Just thought of this case, currently if users specify the algorithm as Ketama, not ketama, we would consider it invalid and fall back to hashmod?

Preferably we should convert the string to lowercase (if this is not already done by the library we use for flags?) and accept it, more user friendly.

[fpetkovski]

I am not sure I like where this rabbit hole is taking us :)

What about adding validation here so that we can take the guesswork out: https://github.com/thanos-io/thanos/blob/main/pkg/receive/hashring.go#L289? That should resolve @yeya24's comment if I'm not mistaken.

[haanhvu]

I made all the required changes. There're still some failed checks because I'm having a problem with Makefile (go version problem). Will look into it tomorrow.

[matej-g]

@haanhvu any updates on this one?

[haanhvu]

@matej-g There're some small fixes left to do (mostly the log as @douglascamata suggested). But I need to finish #5777 first before getting back to this. Can't be in two rabbit holes at the same time...

[matej-g]

No worries @haanhvu, I was just sifting through PRs and wanted to know latest status, carry on and thank you :blush:

[haanhvu]

If an invalid algorithm is specified, do we just fallback to the hashmod algorithm? I feel a better way is to either: Return non-exist error and let users know this is wrong. Fall back to the default one. But have a log to tell that the specified algorithm is wrong. Those 2 options are better than ignoring the error.

Can we log more information about the hashring to help debugging & problem solving? For example, the hashring name and tenants could be useful.

@matej-g @douglascamata @yeya24 Hey about the log, I'm not sure where to put it for now. In #5777 we edited and deleted a lot of functions in hashring.go. If these changes are merged (which will be merged soon, I think), maybe I'll need to put the hashring algorithm checking log to receive cmd or config.go, instead of hashring.go like currently.

So I think we can add the log later, after #5777 merged. Is that ok?

[douglascamata]

Sounds good to me. Not sure what the maintainers think.

[haanhvu]

I made all the required changes. Please help review!

[haanhvu]

I completely forgot. Do we need to add test? Like a simple test that shows the algorithm in hashring config will overwrite the global algorithm?

[douglascamata]

@haanhvu tests are always welcome! :D