Fix port update not reflected in CLUSTER SLOTS #13932

StavRLevi · 2025-04-09T12:26:59Z

Close #13892
config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

… information about cluster slots and their associated nodes. the fix update this info when set cmd is done.

CLAassistant · 2025-04-09T12:27:07Z

All committers have signed the CLA.

sundb · 2025-04-09T12:33:46Z

please add a test to cover this bug.

src/config.c

…test

tests/unit/cluster/announced-endpoints.tcl

sundb · 2025-04-10T11:51:02Z

tests/unit/cluster/announced-endpoints.tcl

+        # Get the original port and change to new_port
+        set orig_port  [lindex [R 0 config get port] 1]
+        assert {$orig_port != ""}
+        set new_port [expr {$orig_port + 100}]


use find_available_port?

For my understanding, we use find_available_port when we need to dynamically allocate unused ports for new Redis instances.
But in this case — the test uses an already running Redis instance (R 0) and calls CONFIG SET port on it. So i think that lindex [R 0 config get port] 1 is valid here (In fact, using the live port is essential to reproduce the bug being tested) — we're not launching a new instance, just modifying the config of an existing one.
The point of this test is to verify that the updated port is properly reflected in CLUSTER SLOTS. So using the original running port ([config get port]) ensures we're updating the correct live node.

But $orig_port + 100 might not work, right? However, this is unlikely, because there are not many instances of the cluster, unless all 100 ports in the middle are used.
we can leave it as it is.

i think find_available_port is better, when we run ./runtest, we may create many instances, this test is not run by ./runtest-cluster, see also tests in #8510.

ok, got it. will fix.

ShooterIT · 2025-04-15T07:12:05Z

BTW, this title is too broad, let's be precise, how about

Fix port update not reflected in CLUSTER SLOTS

sundb · 2025-04-21T01:51:56Z

tests/unit/cluster/announced-endpoints.tcl

        assert_match "*@$base_bus_port *" [R 0 CLUSTER NODES]
    }
+
+test "CONFIG SET port updates cluster-announced port" {


Suggested change

test "CONFIG SET port updates cluster-announced port" {

test "CONFIG SET port updates cluster-announced port" {

### Problem A previous PR (#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

Close redis#13892 config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

### Problem A previous PR (redis#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

Close redis#13892 config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

### Problem A previous PR (redis#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

Close #13892 config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

### Problem A previous PR (#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

Close #13892 config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

### Problem A previous PR (#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

Close redis#13892 config set port cmd updates server.port. cluster slot retrieves information about cluster slots and their associated nodes. the fix updates this info when config set port cmd is done, so cluster slots cmd returns the right value.

### Problem A previous PR (redis#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked. There is this comment in the `addNodeToNodeReply` function in the `cluster.c` file: ```c /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */ addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo())); addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN); ``` ### Fixed This PR fixes the TLS port issue and adds relevant tests.

RED-13892: config get cmd updates server.port. cluster slot retrieves…

a58ed74

… information about cluster slots and their associated nodes. the fix update this info when set cmd is done.

StavRLevi requested review from YaacovHazan and ronen-kalish April 9, 2025 12:27

sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Apr 9, 2025

StavRLevi changed the title ~~RED-13892: update the cluster info in config set cmd~~ update the cluster info in config set cmd Apr 9, 2025

StavRLevi changed the title ~~update the cluster info in config set cmd~~ https://github.com/redis/redis/issues/13892 update the cluster info in config set cmd Apr 9, 2025

sundb changed the title ~~https://github.com/redis/redis/issues/13892 update the cluster info in config set cmd~~ Update the cluster info in config set cmd Apr 9, 2025

sundb reviewed Apr 9, 2025

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

sundb changed the title ~~Update the cluster info in config set cmd~~ Fix missing cluster info update in CONFIG SET command Apr 9, 2025

fix update this cluster slots info when set port cmd is done + added …

4fdaa47

…test

sundb reviewed Apr 10, 2025

View reviewed changes

remove spaces

95e56de

sundb requested a review from ShooterIT April 14, 2025 09:56

sundb added this to Redis 7.4 Backport, Redis 8.2, Redis 6.2 Backport and Redis 7.2 Backport Apr 15, 2025

github-project-automation bot moved this to Todo in Redis 8.2 Apr 15, 2025

github-project-automation bot moved this to Todo in Redis 7.4 Backport Apr 15, 2025

github-project-automation bot moved this to To Do in Redis 6.2 Backport Apr 15, 2025

github-project-automation bot moved this to Todo in Redis 7.2 Backport Apr 15, 2025

StavRLevi changed the title ~~Fix missing cluster info update in CONFIG SET command~~ Fix port update not reflected in CLUSTER SLOTS Apr 16, 2025

change to newport to find_available_port from fixed number

7ad58ae

ShooterIT approved these changes Apr 17, 2025

View reviewed changes

sundb reviewed Apr 21, 2025

View reviewed changes

fix indentation

a9b0e00

sundb approved these changes Apr 21, 2025

View reviewed changes

sundb merged commit a257b6b into redis:unstable Apr 21, 2025
17 of 18 checks passed

github-project-automation bot moved this from Todo to Done in Redis 8.2 Apr 21, 2025

YaacovHazan moved this from Todo to pending in Redis 7.2 Backport Apr 22, 2025

YaacovHazan moved this from Todo to Pending in Redis 7.4 Backport Apr 22, 2025

vitahlin mentioned this pull request Apr 22, 2025

Fix tls port update not reflected in CLUSTER SLOTS #13966

Merged

YaacovHazan moved this from Pending to Todo in Redis 7.4 Backport May 25, 2025

YaacovHazan moved this from pending to Todo in Redis 7.2 Backport May 25, 2025

YaacovHazan moved this from To Do to In progress in Redis 6.2 Backport May 26, 2025

YaacovHazan moved this from In progress to To Do in Redis 6.2 Backport May 26, 2025

YaacovHazan moved this from To Do to Pending in Redis 6.2 Backport May 26, 2025

stondino00 mentioned this pull request Jun 6, 2025

Update Redis to 7.2.9 nextcloud-snap/nextcloud-snap#3186

Closed

YaacovHazan moved this from Todo to Done in Redis 7.2 Backport Jun 29, 2025

YaacovHazan moved this from Todo to Done in Redis 7.4 Backport Jun 29, 2025

sundb added this to Redis 8.0 Aug 15, 2025

sundb removed this from Redis 8.2 Aug 15, 2025

github-project-automation bot moved this to Todo in Redis 8.0 Aug 15, 2025

sundb moved this from Todo to Done in Redis 8.0 Aug 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix port update not reflected in CLUSTER SLOTS #13932

Fix port update not reflected in CLUSTER SLOTS #13932

Uh oh!

StavRLevi commented Apr 9, 2025 •

edited by sundb

Loading

Uh oh!

CLAassistant commented Apr 9, 2025 •

edited

Loading

Uh oh!

sundb commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sundb Apr 10, 2025

Uh oh!

StavRLevi Apr 14, 2025

Uh oh!

sundb Apr 14, 2025

Uh oh!

ShooterIT Apr 15, 2025

Uh oh!

StavRLevi Apr 16, 2025

Uh oh!

ShooterIT commented Apr 15, 2025

Uh oh!

sundb Apr 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	test "CONFIG SET port updates cluster-announced port" {
	test "CONFIG SET port updates cluster-announced port" {

Fix port update not reflected in CLUSTER SLOTS #13932

Fix port update not reflected in CLUSTER SLOTS #13932

Uh oh!

Conversation

StavRLevi commented Apr 9, 2025 • edited by sundb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sundb commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sundb Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

StavRLevi Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

sundb Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

ShooterIT Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

StavRLevi Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

ShooterIT commented Apr 15, 2025

Uh oh!

sundb Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

StavRLevi commented Apr 9, 2025 •

edited by sundb

Loading

CLAassistant commented Apr 9, 2025 •

edited

Loading