Pass extensions to node if extension processing is handled by it #52

hpatro · 2024-03-27T19:08:19Z

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.

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.

hpatro · 2024-03-27T21:55:42Z

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.

hpatro · 2024-03-27T22:37:11Z

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)

Signed-off-by: Harkrishn Patro <[email protected]>

madolson · 2024-03-31T04:30:03Z

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.

src/cluster_legacy.c

src/cluster_legacy.h

src/cluster_legacy.c

src/cluster_legacy.h

src/cluster_legacy.c

src/server.h

hpatro · 2024-04-04T21:42:08Z

@PingXie @madolson I'm addressing the comments on the PR. Will get back in couple of days.

hpatro · 2024-04-04T21:43:11Z

@zuiderkwast Unsure about what you meant by assigning it to Ping for this PR, did you meant it for review or something else ?

PingXie · 2024-04-05T01:12:26Z

@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 :-)

zuiderkwast · 2024-04-05T11:41:17Z

Yeah, I meant as the reviewer. Sorry.

zuiderkwast · 2024-04-05T11:44:12Z

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.

PingXie · 2024-04-05T13:14:31Z

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.

madolson · 2024-04-05T20:03:17Z

@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.

madolson · 2024-04-05T20:04:03Z

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.

hpatro · 2024-04-05T20:25:24Z

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.

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro · 2024-04-05T22:55:33Z

Changes done:

Introduce new node flag CLUSTER_NODE_EXTENSIONS_SUPPORTED to mark if the node can handle extension(s) data based on CLUSTERMSG_FLAG0_EXT_DATA cluster message header flag.
Send extensions data only to the node which can handle extensions based on pt. 1.
CLUSTERMSG_FLAG0_EXT_DATA header 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.

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro · 2024-04-06T03:45:26Z

@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.

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro · 2024-04-06T04:03:47Z

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 fail for the Redis 6.2 and remains connected to the Valkey latest node.

Signed-off-by: Harkrishn Patro <[email protected]>

PingXie

This looks a lot cleaner and maintainable to me. Thanks a lot for simplifying the fix, @hpatro!

The server code LGTM but I think there is room for improvement in the test code. Can you please take another look?

tests/unit/cluster/hostnames.tcl

Signed-off-by: Harkrishn Patro <[email protected]>

PingXie

LGTM

tests/unit/cluster/hostnames.tcl

madolson · 2024-04-07T02:33:42Z

@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.

hpatro · 2024-04-07T05:07:48Z

@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.

madolson · 2024-04-07T05:12:03Z

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?

hpatro · 2024-04-07T05:21:36Z

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? 🙈

madolson · 2024-04-07T05:26:40Z

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).

PingXie · 2024-04-07T06:11:11Z

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.

madolson · 2024-04-07T06:45:26Z

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.

madolson

Besides my comment I was OK with the design, so would rather have this in the RC candidate. Folks can comment.

…key-io#52) Ref: redis/redis#12760 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). TCL test verifying the cluster state is healthy irrespective of enabling/disabling cluster message extensions feature. --------- Signed-off-by: Harkrishn Patro <[email protected]>

hpatro · 2024-04-09T16:04:57Z

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.

PingXie · 2024-04-11T15:27:07Z

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).

madolson · 2024-04-11T23:40:31Z

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.

…key-io#52) Ref: redis/redis#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. --------- Signed-off-by: Harkrishn Patro <[email protected]>

) This PR is based on the commits from PR valkey-io/valkey#52. Ref: #12760 Close #13401 This PR will replace #13449 Fixes compatibilty of Redis 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 does the following things: 1. Always set `CLUSTERMSG_FLAG0_EXT_DATA`, because during the meet phase, we do not know whether the connected node supports ext data, we need to make sure that it knows and send back its ext data if it has. 2. If another node does not support ext data, we will not send it ext data to avoid the handshake failure due to the incorrect payload length. Note: A successful `PING`/`PONG` is required as a sender for a given node to be marked as `CLUSTERMSG_FLAG0_EXT_DATA` and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s). --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]> --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]>

…is#13465) This PR is based on the commits from PR valkey-io/valkey#52. Ref: redis#12760 Close redis#13401 This PR will replace redis#13449 Fixes compatibilty of Redis 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 does the following things: 1. Always set `CLUSTERMSG_FLAG0_EXT_DATA`, because during the meet phase, we do not know whether the connected node supports ext data, we need to make sure that it knows and send back its ext data if it has. 2. If another node does not support ext data, we will not send it ext data to avoid the handshake failure due to the incorrect payload length. Note: A successful `PING`/`PONG` is required as a sender for a given node to be marked as `CLUSTERMSG_FLAG0_EXT_DATA` and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s). --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]> --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]>

) This PR is based on the commits from PR valkey-io/valkey#52. Ref: #12760 Close #13401 This PR will replace #13449 Fixes compatibilty of Redis 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 does the following things: 1. Always set `CLUSTERMSG_FLAG0_EXT_DATA`, because during the meet phase, we do not know whether the connected node supports ext data, we need to make sure that it knows and send back its ext data if it has. 2. If another node does not support ext data, we will not send it ext data to avoid the handshake failure due to the incorrect payload length. Note: A successful `PING`/`PONG` is required as a sender for a given node to be marked as `CLUSTERMSG_FLAG0_EXT_DATA` and then extensions message will be sent to it. This could cause a slight delay in receiving the extensions message(s). --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]> --------- Signed-off-by: Harkrishn Patro <[email protected]> Co-authored-by: Harkrishn Patro <[email protected]>

hpatro requested a review from a team March 27, 2024 19:08

hpatro force-pushed the cluster_upgrade branch from 43d6c6e to 4b622a3 Compare March 27, 2024 19:10

hpatro requested a review from roshkhatri March 27, 2024 22:37

hpatro added 2 commits March 27, 2024 23:03

Pass extensions to node if extension processing is handled by it

b0576d0

Signed-off-by: Harkrishn Patro <[email protected]>

Fix flaky test

9466e67

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro force-pushed the cluster_upgrade branch from adf561f to 9466e67 Compare March 27, 2024 23:04

hpatro mentioned this pull request Mar 29, 2024

[NEW] Introduce automated cross version and cross fork testing infrastructure #76

Open