valkey
valkey copied to clipboard
valkey-io
Pass extensions to node if extension processing is handled by it
Ref: https://github.com/redis/redis/pull/12760
Description
Fixes compatibilty of PlaceholderKV cluster (7.2 - extensions enabled by default) with older Redis cluster (< 7.0 - extensions not handled) .
With some of the extensions enabled by default in 7.2 version, new nodes running 7.2 and above start sending out larger clusterbus message payload including the ping extensions. This caused an incompatibility with node running engine versions < 7.0. Old nodes (< 7.0) would receive the payload from new nodes (> 7.2) would observe a payload length (totlen) > (estlen) and would perform an early exit and won't process the message.
This fix introduces a flag extensions_supported on the clusterMsg indicating the sender node can handle extensions parsing. Once, a receiver nodes receives a message with this flag set to 1, it would update clusterNode new field extensions_supported and start sending out extensions if it has any.
This PR also introduces a DEBUG sub command to enable/disable cluster message extensions process-clustermsg-extensions feature.
Note: A successful PING/PONG is required as a sender for a given node to be marked as extensions_supported and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s).
Testing
TCL test verifying the cluster state is healthy irrespective of enabling/disabling cluster message extensions feature.
One of the test fails in few of the runs.
[err]: Verify the nodes configured with prefer hostname only show hostname for new nodes in tests/unit/cluster/hostnames.tcl
Expected '' to be equal to 'shard-1.com' (context: type eval line 60 cmd {assert_equal [lindex [get_slot_field $slot_result 0 2 3] 1] "shard-1.com"} proc ::test)
Taking a look.
Regarding the flaky test. There is a slight behavior change with this PR.
A successful PING/PONG is required as a sender for a given node to be marked as extensions_supported and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s). (Added it to the top level comment as well)
A successful PING/PONG is required as a sender for a given node to be marked as extensions_supported and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s). (Added it to the top level comment as well)
So this happens in two flows:
- The initial meet flow, where a new node to a cluster has to omit extensions since it doesn't know if the target supports them. After the target of the meet receives the message, it will show the node without extensions (not great) until it get's a followup pong message. We could solve this by omitting a node from cluster output (if it sent us a message with the extension bit set).
- When gossiping, other nodes will learn about the new node, and they also send it a message without extensions. This results as the same problem as one, but is easier to resolve since we can gossip if a node supports extensions. There ate 16 bits of flags (of which only 10 are being used) on the node flags that
So we really just need a goos solution for 1.
@PingXie @madolson I'm addressing the comments on the PR. Will get back in couple of days.
@zuiderkwast Unsure about what you meant by assigning it to Ping for this PR, did you meant it for review or something else ?
@zuiderkwast Unsure about what you meant by assigning it to Ping for this PR, did you meant it for review or something else ?
@hpatro please continue with this PR. I will be the "designated" reviewer :-)
Question: Shall we include this in our 7.2.4 release?
Although it improves compatibility with older Redis versions, it's a difference compared to Redis 7.2.4. We should mark this clearly in our release notes then.
Shall we include this in our 7.2.4 release?
I think we should try our best to make it happen. It is an important and fully compatible fix.
@hpatro please continue with this PR. I will be the "designated" reviewer :-)
Reminds me of the fact we should add code owner when we get this all sorted.
I think we should try our best to make it happen. It is an important and fully compatible fix.
Yeah, I was pushing for this because I think it's important for compatibility. I would be okay with this in like a 7.2.5 though, it's important to have eventually.
I think I can submit a PR today. And get it done by Monday if you all can help review it @madolson @PingXie . It would be nice to have this in our first release itself.
Changes done:
- Introduce new node flag
CLUSTER_NODE_EXTENSIONS_SUPPORTEDto mark if the node can handle extension(s) data based onCLUSTERMSG_FLAG0_EXT_DATAcluster message header flag. - Send extensions data only to the node which can handle extensions based on pt. 1.
CLUSTERMSG_FLAG0_EXT_DATAheader flag would be sent irrespective of extensions present or not. This helps indicate if the node supports extensions feature or not.- Introduce debug flag to enable/disable sending extensions data. Receiving extensions is still enabled.
@PingXie @madolson Let me know if you agree with this.
@madolson Regarding your concern of displaying IP address incorrectly for a period of time, I think we should decouple it from this issue. And I think that issue was valid in 7.0 itself. Can discuss further.
@PingXie Thanks for your feedback.
I initially started off this PR (in Redis) with similar setup you suggested (fixing the issue and manually tested it). The fake testing approach seems to bring in a bit of complexity and albeit it wasn't perfect (tried to get it out in a hurry). I've trimmed down the code, please have a look now.
Tested few things
Setup
- 2 cluster (1 primary/1 replica) each
- Shard 1: Engine Redis 6.2
- Shard 2: Engine Valkey latest with patch
Operations
- MEET between the hybrid cluster - Cluster was stable during the operation and slot information got propagated correctly between the clusters.
- Replaced primary of Redis 6.2 with Valkey latest, cluster/node remained healthy after the replacement.
- Added a Redis 7.2 node (without the fix), shows up as
failfor the Redis 6.2 and remains connected to the Valkey latest node.
@madolson Regarding your concern of displaying IP address incorrectly for a period of time, I think we should decouple it from this issue. And I think that issue was valid in 7.0 itself. Can discuss further.
Why do you think this? I think this is a regression if you have hostnames enabled and have a new node join a cluster, as for a brief period of time the nodes will incorrect show up as not support hostnames (even though they do). We've seen issues in the past where clients don't handle this correctly, and some clients see prolonged periods without being able to resolve it. My concern is that it affects new nodes added during steady state, and not just the upgrade case.
@madolson Regarding your concern of displaying IP address incorrectly for a period of time, I think we should decouple it from this issue. And I think that issue was valid in 7.0 itself. Can discuss further.
Why do you think this? I think this is a regression if you have hostnames enabled and have a new node join a cluster, as for a brief period of time the nodes will incorrect show up as not support hostnames (even though they do). We've seen issues in the past where clients don't handle this correctly, and some clients see prolonged periods without being able to resolve it. My concern is that it affects new nodes added during steady state, and not just the upgrade case.
Exactly, this issue exists for a new node being added and not related to the upgrade scenario. So, it could happen for any cluster running with hostnames feature. Hence, I don't want to couple with this particular PR.
Exactly, this issue exists for a new node being added and not related to the upgrade scenario. So, it could happen for any cluster running with hostnames feature.
Today, the hostname feature blindly sends the extension when it joins a new cluster, assuming it supports it. Now, with this change, we will wait until we have confirmation it supports the hostname. This is different behavior no?
Exactly, this issue exists for a new node being added and not related to the upgrade scenario. So, it could happen for any cluster running with hostnames feature.
Today, the hostname feature blindly sends the extension when it joins a new cluster, assuming it supports it. Now, with this change, we will wait until we have confirmation it supports the hostname. This is different behavior no?
Won't that node be added via gossip as well and in the gossip information, hostname won't be present. Am I missing something? 🙈
Won't that node be added via gossip as well and in the gossip information, hostname won't be present. Am I missing something? 🙈
That was one of the two cases we discussed, but the case I'm talking about is for meet messages. The first message a node sends to a new cluster will not include its hostname. (Maybe I'm indexing too much on this case though).
Exactly, this issue exists for a new node being added and not related to the upgrade scenario. So, it could happen for any cluster running with hostnames feature.
Today, the hostname feature blindly sends the extension when it joins a new cluster, assuming it supports it. Now, with this change, we will wait until we have confirmation it supports the hostname. This is different behavior no?
Why is this a problem? The cluster bus is an eventual consistency system, and the information broadcast via the cluster bus including "hostname" is inherently dynamic/non-constant. so conceptually speaking this behavior change is within the design premise.
The cluster bus is an eventual consistency system, and the information broadcast via the cluster bus including "hostname" is inherently dynamic/non-constant.
Although generally this is true, in practice today if you set all the hostnames at startup a node will never observe a node that doesn't have a hostname. This change introduces a small window where that isn't true, and it could show it to clients and a user might see errors.
I suppose I need to be prescriptive. It seems like everyone wants something simple, so I can lean into that. I would rather hostnames continue to work as they do, with nodes not checking to see if the other node supports it.
Besides my comment I was OK with the design, so would rather have this in the RC candidate. Folks can comment.
Will file an issue explaining the scenarios better and we can decide over there.
if you set all the hostnames at startup a node will never observe a node that doesn't have a hostname. This change introduces a small window where that isn't true, and it could show it to clients and a user might see errors.
First and foremost, I am not sure about the importance/value of this property of "a node will never observe a node that doesn't have a hostname". This already can happen during rolling upgrades from pre 7.0 builds to 7.2 and all nodes will have to handle it.
Furthermore, the propagation of many other node attributes don't have this property either today. Even within the same node, the cluster and the replication modules might have a different view on the "primary-replica" relationship too (for a very short period of time after cluster replicate).
My point is that, with the current cluster design, the operator would have to perform an explicit synchronization regardless (think of wait_for_cluster_propagation). So a one-off effort to provide this property (of "a node will never observe a node that doesn't have a hostname") is not going to change the landscape much but with the downside of the excessive complexity (to a part of the system where we already are grappling with reliability issues).
First and foremost, I am not sure about the importance/value of this property of "a node will never observe a node that doesn't have a hostname". This already can happen during rolling upgrades from pre 7.0 builds to 7.2 and all nodes will have to handle it.
The important point here is a node can't have a hostname pre 7.0, it was an opt-in feature.