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

hpatro · 2023-11-13T23:47:21Z

Description

With some of the extensions enabled by default in 7.2, 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 do 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.

Testing

Manually setup nodes running with 6.0 in cluster-enabled and did a rolling upgrade to 7.2 and throughout the upgrades the cluster was healthy. After the upgrade completed, verified cluster extensions parsing was ongoing via gdb and CLUSTER MYSHARDID was a valid value on both sender and receiver.

hpatro · 2023-11-13T23:48:28Z

Note: Our current testing infrastructure doesn't support testing engine upgrade scenario, we can invest on building it separately into Redis.

madolson

Looks ok, I think this generally makes sense.

madolson · 2023-12-26T02:42:37Z

src/cluster.h

    char myip[NET_IP_STR_LEN];    /* Sender IP, if not all zeroed. */
    uint16_t extensions; /* Number of extensions sent along with this packet. */
-    char notused1[30];   /* 30 bytes reserved for future usage. */
+    uint16_t extensions_supported; /* Does the node support parsing ping extensions. */


This is fine, except it maybe should be like mflags2 or something. I would use 8 bits and not 16.

Didn't intend for this variable to be used for any other purpose. Do we want it to renamed to mflags2 ?

Modified it to 8 bits.

Oops, there is some issue with macos on modifying it to 8 bits.

The API(s) are the following:

extern uint16_t ntohs (uint16_t __netshort) __THROW __attribute__ ((__const__)); extern uint16_t htons (uint16_t __hostshort) __THROW __attribute__ ((__const__));

I would rather keep the variable as uint16_t to avoid any issue during conversion. WDYT ?

src/cluster_legacy.h

stevelipinski · 2024-02-28T15:42:08Z

We were hoping to gain the ability to upgrade from a 6.2 cluster to 7.2 and are blocked by this.
Is this proposed code change sufficient to fix issue #12685?
What is required to get this reviewed and merged?
We may be able to try to test this if there's a need for additional validation of this change.

madolson · 2024-03-11T20:10:25Z

src/cluster_legacy.h

    clusterLink *link;          /* TCP/IP link established toward this node */
    clusterLink *inbound_link;  /* TCP/IP link accepted from this node */
    list *fail_reports;         /* List of nodes signaling this as failing */
+    uint16_t extensions_supported; /* Does the node support parsing ping extensions. */


Can we add this into flags instead of a separate 16 bit field?

madolson · 2024-03-11T20:15:22Z

src/cluster_legacy.h

 static_assert(offsetof(clusterMsg, extensions) == 2214, "unexpected field offset");
-static_assert(offsetof(clusterMsg, notused1) == 2216, "unexpected field offset");
+static_assert(offsetof(clusterMsg, mflags2) == 2216, "unexpected field offset");
+static_assert(offsetof(clusterMsg, notused1) == 2217, "unexpected field offset");


Sorry, I just realized that we have 3 bytes of mflags, not 3 bits, so we still have 21 bits left to use. So we don't need to introduce mflags 2 here.

CLAassistant · 2024-03-24T23:05:02Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

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 <harkrisp@amazon.com>

…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 <harkrisp@amazon.com>

…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 <harkrisp@amazon.com>

Ref: 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 <harkrisp@amazon.com>

) 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 <harkrisp@amazon.com> Co-authored-by: Harkrishn Patro <harkrisp@amazon.com> --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com> Co-authored-by: Harkrishn Patro <harkrisp@amazon.com>

…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 <harkrisp@amazon.com> Co-authored-by: Harkrishn Patro <harkrisp@amazon.com> --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com> Co-authored-by: Harkrishn Patro <harkrisp@amazon.com>

Pass extensions to node if extension processing is handled by it

256a94c

hpatro requested a review from madolson November 13, 2023 23:47

hpatro marked this pull request as draft November 13, 2023 23:51

madolson reviewed Dec 26, 2023

View reviewed changes

hpatro added 4 commits January 12, 2024 18:27

Use uint8_t type for extensions_supported

6237d1d

Merge branch 'unstable' into cluster_upgrade_fix

ecea655

cast to uint16_t for htons parameter

5b76b99

Explicit conversion of htons response

dc047ba

madolson reviewed Jan 12, 2024

View reviewed changes

src/cluster_legacy.h Outdated Show resolved Hide resolved

madolson reviewed Jan 12, 2024

View reviewed changes

src/cluster_legacy.h Outdated Show resolved Hide resolved

hpatro added 2 commits January 12, 2024 22:44

Revert to 16 bytes to be compatible with htons/ntohs api

538551d

Convert to unsigned char to avoid memory wastage

3cc0893

hpatro marked this pull request as ready for review January 19, 2024 18:44

madolson reviewed Mar 11, 2024

View reviewed changes

hpatro mentioned this pull request Mar 27, 2024

Pass extensions to node if extension processing is handled by it valkey-io/valkey#52

Merged

hpatro closed this Apr 10, 2024

sundb mentioned this pull request Aug 6, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

hpatro commented Nov 13, 2023

Uh oh!

hpatro commented Nov 13, 2023

Uh oh!

madolson left a comment

Uh oh!

madolson Dec 26, 2023

Uh oh!

hpatro Jan 12, 2024

Uh oh!

hpatro Jan 12, 2024 •

edited

Loading

Uh oh!

hpatro Jan 12, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

stevelipinski commented Feb 28, 2024

Uh oh!

madolson Mar 11, 2024

Uh oh!

madolson Mar 11, 2024

Uh oh!

CLAassistant commented Mar 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

Uh oh!

Conversation

hpatro commented Nov 13, 2023

Description

Testing

Uh oh!

hpatro commented Nov 13, 2023

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

madolson Dec 26, 2023

Choose a reason for hiding this comment

Uh oh!

hpatro Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

hpatro Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hpatro Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevelipinski commented Feb 28, 2024

Uh oh!

madolson Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

madolson Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Mar 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hpatro Jan 12, 2024 •

edited

Loading

hpatro Jan 12, 2024 •

edited

Loading