Unverified Commit 6f0ddc9d authored by debing.sun's avatar debing.sun Committed by GitHub
Browse files

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

This PR is based on the commits from PR
https://github.com/valkey-io/valkey/pull/52.
Ref: https://github.com/redis/redis/pull/12760
Close https://github.com/redis/redis/issues/13401
This PR will replace https://github.com/redis/redis/pull/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: default avatarHarkrishn Patro <harkrisp@amazon.com>
Co-authored-by: default avatarHarkrishn Patro <harkrisp@amazon.com>

---------
Signed-off-by: default avatarHarkrishn Patro <harkrisp@amazon.com>
Co-authored-by: default avatarHarkrishn Patro <harkrisp@amazon.com>
parent 731f2dc5
......@@ -2,8 +2,13 @@
* Copyright (c) 2009-Present, Redis Ltd.
* All rights reserved.
*
* Copyright (c) 2024-present, Valkey contributors.
* All rights reserved.
*
* Licensed under your choice of the Redis Source Available License 2.0
* (RSALv2) or the Server Side Public License v1 (SSPLv1).
*
* Portions of this file are available under BSD3 terms; see REDISCONTRIBUTIONS for more information.
*/
/*
......@@ -2579,9 +2584,6 @@ uint32_t writePingExt(clusterMsg *hdr, int gossipcount) {
extensions++;
if (hdr != NULL) {
if (extensions != 0) {
hdr->mflags[0] |= CLUSTERMSG_FLAG0_EXT_DATA;
}
hdr->extensions = htons(extensions);
}
......@@ -2771,6 +2773,9 @@ int clusterProcessPacket(clusterLink *link) {
}
sender = getNodeFromLinkAndMsg(link, hdr);
if (sender && (hdr->mflags[0] & CLUSTERMSG_FLAG0_EXT_DATA)) {
sender->flags |= CLUSTER_NODE_EXTENSIONS_SUPPORTED;
}
/* Update the last time we saw any data from this node. We
* use this in order to avoid detecting a timeout from a node that
......@@ -3536,6 +3541,8 @@ static void clusterBuildMessageHdr(clusterMsg *hdr, int type, size_t msglen) {
/* Set the message flags. */
if (clusterNodeIsMaster(myself) && server.cluster->mf_end)
hdr->mflags[0] |= CLUSTERMSG_FLAG0_PAUSED;
hdr->mflags[0] |= CLUSTERMSG_FLAG0_EXT_DATA; /* Always make other nodes know that
* this node supports extension data. */
hdr->totlen = htonl(msglen);
}
......@@ -3614,7 +3621,9 @@ void clusterSendPing(clusterLink *link, int type) {
* to put inside the packet. */
estlen = sizeof(clusterMsg) - sizeof(union clusterMsgData);
estlen += (sizeof(clusterMsgDataGossip)*(wanted + pfail_wanted));
estlen += writePingExt(NULL, 0);
if (link->node && nodeSupportsExtensions(link->node)) {
estlen += writePingExt(NULL, 0);
}
/* Note: clusterBuildMessageHdr() expects the buffer to be always at least
* sizeof(clusterMsg) or more. */
if (estlen < (int)sizeof(clusterMsg)) estlen = sizeof(clusterMsg);
......@@ -3684,7 +3693,9 @@ void clusterSendPing(clusterLink *link, int type) {
/* Compute the actual total length and send! */
uint32_t totlen = 0;
totlen += writePingExt(hdr, gossipcount);
if (link->node && nodeSupportsExtensions(link->node)) {
totlen += writePingExt(hdr, gossipcount);
}
totlen += sizeof(clusterMsg)-sizeof(union clusterMsgData);
totlen += (sizeof(clusterMsgDataGossip)*gossipcount);
serverAssert(gossipcount < USHRT_MAX);
......
/*
* Copyright (c) 2009-Present, Redis Ltd.
* All rights reserved.
*
* Copyright (c) 2024-present, Valkey contributors.
* All rights reserved.
*
* Licensed under your choice of the Redis Source Available License 2.0
* (RSALv2) or the Server Side Public License v1 (SSPLv1).
*
* Portions of this file are available under BSD3 terms; see REDISCONTRIBUTIONS for more information.
*/
#ifndef CLUSTER_LEGACY_H
#define CLUSTER_LEGACY_H
......@@ -51,6 +64,7 @@ typedef struct clusterLink {
#define CLUSTER_NODE_MEET 128 /* Send a MEET message to this node */
#define CLUSTER_NODE_MIGRATE_TO 256 /* Master eligible for replica migration. */
#define CLUSTER_NODE_NOFAILOVER 512 /* Slave will not try to failover. */
#define CLUSTER_NODE_EXTENSIONS_SUPPORTED 1024 /* This node supports extensions. */
#define CLUSTER_NODE_NULL_NAME "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
#define nodeIsSlave(n) ((n)->flags & CLUSTER_NODE_SLAVE)
......@@ -59,6 +73,7 @@ typedef struct clusterLink {
#define nodeTimedOut(n) ((n)->flags & CLUSTER_NODE_PFAIL)
#define nodeFailed(n) ((n)->flags & CLUSTER_NODE_FAIL)
#define nodeCantFailover(n) ((n)->flags & CLUSTER_NODE_NOFAILOVER)
#define nodeSupportsExtensions(n) ((n)->flags & CLUSTER_NODE_EXTENSIONS_SUPPORTED)
/* This structure represent elements of node->fail_reports. */
typedef struct clusterNodeFailReport {
......
#
# Copyright (c) 2009-Present, Redis Ltd.
# All rights reserved.
#
# Copyright (c) 2024-present, Valkey contributors.
# All rights reserved.
#
# Licensed under your choice of the Redis Source Available License 2.0
# (RSALv2) or the Server Side Public License v1 (SSPLv1).
#
# Portions of this file are available under BSD3 terms; see REDISCONTRIBUTIONS for more information.
#
proc get_slot_field {slot_output shard_id node_id attrib_id} {
return [lindex [lindex [lindex $slot_output $shard_id] $node_id] $attrib_id]
}
......@@ -116,10 +129,11 @@ test "Verify the nodes configured with prefer hostname only show hostname for ne
# Have everyone forget node 6 and isolate it from the cluster.
isolate_node 6
# Set hostnames for the masters, now that the node is isolated
R 0 config set cluster-announce-hostname "shard-1.com"
R 1 config set cluster-announce-hostname "shard-2.com"
R 2 config set cluster-announce-hostname "shard-3.com"
set primaries 3
for {set j 0} {$j < $primaries} {incr j} {
# Set hostnames for the masters, now that the node is isolated
R $j config set cluster-announce-hostname "shard-$j.com"
}
# Prevent Node 0 and Node 6 from properly meeting,
# they'll hang in the handshake phase. This allows us to
......@@ -149,9 +163,17 @@ test "Verify the nodes configured with prefer hostname only show hostname for ne
} else {
fail "Node did not learn about the 2 shards it can talk to"
}
set slot_result [R 6 CLUSTER SLOTS]
assert_equal [lindex [get_slot_field $slot_result 0 2 3] 1] "shard-2.com"
assert_equal [lindex [get_slot_field $slot_result 1 2 3] 1] "shard-3.com"
wait_for_condition 50 100 {
[lindex [get_slot_field [R 6 CLUSTER SLOTS] 0 2 3] 1] eq "shard-1.com"
} else {
fail "hostname for shard-1 didn't reach node 6"
}
wait_for_condition 50 100 {
[lindex [get_slot_field [R 6 CLUSTER SLOTS] 1 2 3] 1] eq "shard-2.com"
} else {
fail "hostname for shard-2 didn't reach node 6"
}
# Also make sure we know about the isolated master, we
# just can't reach it.
......@@ -170,10 +192,14 @@ test "Verify the nodes configured with prefer hostname only show hostname for ne
} else {
fail "Node did not learn about the 2 shards it can talk to"
}
set slot_result [R 6 CLUSTER SLOTS]
assert_equal [lindex [get_slot_field $slot_result 0 2 3] 1] "shard-1.com"
assert_equal [lindex [get_slot_field $slot_result 1 2 3] 1] "shard-2.com"
assert_equal [lindex [get_slot_field $slot_result 2 2 3] 1] "shard-3.com"
for {set j 0} {$j < $primaries} {incr j} {
wait_for_condition 50 100 {
[lindex [get_slot_field [R 6 CLUSTER SLOTS] $j 2 3] 1] eq "shard-$j.com"
} else {
fail "hostname information for shard-$j didn't reach node 6"
}
}
}
test "Test restart will keep hostname information" {
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment