Add cluster-port support to redis-cli --cluster #10344

enjoy-binbin · 2022-02-25T13:24:38Z

In #9389, we add a new cluster-port config and make cluster bus port configurable,
and currently redis-cli --cluster create/add-node doesn't support with a configurable cluster-port instance.
Because redis-cli uses the old way (port + 10000) to send the CLUSTER MEET command.

Now we add this support on redis-cli --cluster, note we don't need to explicitly pass in the
cluster-port parameter, we can get the real cluster-port of the node in clusterManagerNodeLoadInfo,
so the --cluster create and --cluster add-node interfaces have not changed.

We will use the cluster-port when we are doing CLUSTER MEET, also note that CLUSTER MEET bus-port
parameter was added in 4.0, so if the bus_port (the one in redis-cli) is 0, or equal (port + 10000),
we just call CLUSTER MEET with 2 arguments, using the old form.

Fixes #10342. Fixes #10954

In redis#9389, we add a new cluster-port config and make cluster bus port configurable. Now add support for this parameter in redis-cli. Mainly for `--cluster create` and `--cluster add-node`, we can use `ip:port@bus_port` to pass the `cluster-port` parameter. The bus_port is optional, if not specified it is the same as before. Also add the `bus_port` field in `--cluster backup`, we also need to backup the `cluster-port`. Fixes redis#10342

hwware · 2022-02-25T15:57:09Z

I think code part is ok, but could you please add test in the same PR, then if the CI failed after this PR merged, we could easily locate where the issue probably comes from. Thanks

src/config.c

src/redis-cli.c

tests/unit/cluster.tcl

satheeshaGowda · 2022-03-09T18:47:50Z

Hello @enjoy-binbin if we are good with this approved PR (dont see any pending actions), could you please merge it? we are kind of blocked on this for now, thanks for understanding .

…er_bus_port

src/redis-cli.c

madolson

High level LGTM, just some minor comments.

src/server.c

src/redis-cli.c

tests/support/util.tcl

madolson · 2022-07-07T03:07:28Z

This seems like something we can merge back into 7.0, I see someone was blocked on it, and the actual major decision is small. @enjoy-binbin Do you have time to rebase and we can get it merged ASAP?

enjoy-binbin · 2022-07-07T03:14:07Z

@madolson yes, i was planning to do a rebase today too
i can take care of it, and then take care of the comments, thanks for the review

…er_bus_port

to be a bit cleaner Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

…fields

madolson

LGTM. @redis/core-team I'm not entirely sure if this is a major decision, I think we decided that tool API changes count. Please check the top comment and comment.

oranagra

LGTM (didn't look deeply into the code).
We do consider interface changes in tools as major decisions, but this PR doesn't have an interface change.

oranagra · 2022-07-11T06:09:01Z

Ohh, i was looking for the interface change in redis (remembered the one that was removed), but actually the remaining interface changes are described in the top comment, in redis-cli --cluster info and redis-cli --cluster backup.
i think the one in --cluster backup is harmless, but the one in --cluster info could cause some script that parses that to fail.

As far as i can tell these changes are not required for the fix in this ticket and are not strictly required for the new cluster-port feature to be useful, so maybe we should remove them from this PR, and introduce them in 7.2 (to avoid risk of braking something in a patch-level release)?

@enjoy-binbin @madolson WDYT?

enjoy-binbin · 2022-07-11T06:28:26Z

As far as i can tell these changes are not required for the fix in this ticket and are not strictly required for the new cluster-port feature to be useful

yes, after i refreshed this PR, realize the change is not that important, i have a thought to remove it, #10344 (comment)

so maybe we should remove them from this PR, and introduce them in 7.2 (to avoid risk of braking something in a patch-level release)?

i agree, for using cluster-port, that info change is not necessary, and it's also more safe, i am going to remove the info change. and i think madolson would agree too. i can do it in advance

oranagra

i'm not sure we want / need to add the cluster bus port to the backup json
i.e. it probably can't do any harm. but maybe such a backup doesn't have to contain that detail.
we do have a way to identify the source.. and it's not as if it holds the entire configuration so we can use that backup to restore the config too..

i suggest to drop that too, and re-consider for 7.2

src/redis-cli.c

enjoy-binbin · 2022-07-11T07:18:27Z

yes i agree, i drop the info and backup changes for now, let's re-consider for 7.2 if it needed

for the record, the --cluster backup was added in #5888, and artix75 said it will be a restore command in the future, but...
I looked at this json file and it's more like logging stuff than for restoring the cluster.
I modified it at the time for completeness, no a strong will, we can re-think it in 7.2 if we need that --cluster restore command

oranagra · 2022-07-11T11:14:24Z

for the record, before merging i triggered both cluster tests and the unit/cluster one:
https://github.com/redis/redis/actions/runs/2648128963
https://github.com/redis/redis/actions/runs/2648135573

This bug was introduced in redis#10344 (7.0.3), and it breaks the redis-cli --cluster create usage in redis#10436 (7.0 RC3). At the same time, the cluster-port support introduced in redis#10344 cannot use the DNS lookup brought by redis#10436.

…node (#11151) This bug was introduced in #10344 (7.0.3), and it breaks the redis-cli --cluster create usage in #10436 (7.0 RC3). At the same time, the cluster-port support introduced in #10344 cannot use the DNS lookup brought by #10436.

…node (#11151) This bug was introduced in #10344 (7.0.3), and it breaks the redis-cli --cluster create usage in #10436 (7.0 RC3). At the same time, the cluster-port support introduced in #10344 cannot use the DNS lookup brought by #10436. (cherry picked from commit c2b0c13)

…node (redis#11151) This bug was introduced in redis#10344 (7.0.3), and it breaks the redis-cli --cluster create usage in redis#10436 (7.0 RC3). At the same time, the cluster-port support introduced in redis#10344 cannot use the DNS lookup brought by redis#10436.

In redis#9389, we add a new `cluster-port` config and make cluster bus port configurable, and currently redis-cli --cluster create/add-node doesn't support with a configurable `cluster-port` instance. Because redis-cli uses the old way (port + 10000) to send the `CLUSTER MEET` command. Now we add this support on redis-cli `--cluster`, note we don't need to explicitly pass in the `cluster-port` parameter, we can get the real `cluster-port` of the node in `clusterManagerNodeLoadInfo`, so the `--cluster create` and `--cluster add-node` interfaces have not changed. We will use the `cluster-port` when we are doing `CLUSTER MEET`, also note that `CLUSTER MEET` bus-port parameter was added in 4.0, so if the bus_port (the one in redis-cli) is 0, or equal (port + 10000), we just call `CLUSTER MEET` with 2 arguments, using the old form. Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

…node (redis#11151) This bug was introduced in redis#10344 (7.0.3), and it breaks the redis-cli --cluster create usage in redis#10436 (7.0 RC3). At the same time, the cluster-port support introduced in redis#10344 cannot use the DNS lookup brought by redis#10436.

satheeshaGowda mentioned this pull request Feb 25, 2022

redis-cli --cluster create , failing to create cluster. #10342

Closed

enjoy-binbin added 4 commits February 26, 2022 19:16

Add cluster_port in INFO CLUSTER

faf37aa

cluster-port is mutable -> MODIFIABLE_CONFIG

093801b

Add --cluster create / add-node tests

06d35bc

Fix Nodes don't agree about configuration

5c2a2a9

PingXie reviewed Feb 27, 2022

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

src/redis-cli.c Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Show resolved Hide resolved

enjoy-binbin added 2 commits February 27, 2022 13:06

log / help message cleanups

f01a772

revert cluster-port back to immutable

fb32c21

PingXie reviewed Mar 2, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

tests/unit/cluster.tcl Outdated Show resolved Hide resolved

CR from PingXie: fix getInfoProperty with ^

db352e6

PingXie approved these changes Mar 2, 2022

View reviewed changes

enjoy-binbin requested a review from madolson March 10, 2022 04:03

enjoy-binbin added 4 commits March 28, 2022 18:55

Temporarily delete the test and merge the lastest unstable

11269b1

Merge remote-tracking branch 'upstream/unstable' into redis_cli_clust…

c8f228d

…er_bus_port

add the test back

b753ca6

remove clusterManagerCommandHelp stdout cleanup - reduce diff

6ab3625

enjoy-binbin requested a review from zuiderkwast March 28, 2022 11:11

zuiderkwast approved these changes Mar 28, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

madolson reviewed Jul 7, 2022

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

tests/support/util.tcl Show resolved Hide resolved

enjoy-binbin and others added 4 commits July 7, 2022 11:27

Merge remote-tracking branch 'upstream/unstable' into redis_cli_clust…

23f5443

…er_bus_port

Update tests/support/util.tcl

447ffa2

to be a bit cleaner Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

remove cluster_port in INFO, generally don't add configs to the info …

ad60fcb

…fields

CLUSTER MEET: maintain the compatibility

1493f61

enjoy-binbin marked this pull request as draft July 7, 2022 06:39

we don't need to pass the bus_port anymore

354445b

enjoy-binbin added 3 commits July 7, 2022 17:07

revert the change in cluster.c

c8a7f3f

revert the change in config.c

2172c2b

cluster backup: bus_port->cluster_bus_port

d9bab48

enjoy-binbin mentioned this pull request Jul 8, 2022

Redis cluster bus port #10954

Closed

madolson approved these changes Jul 10, 2022

View reviewed changes

madolson added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus approval-needed Waiting for core team approval to be merged labels Jul 10, 2022

oranagra approved these changes Jul 10, 2022

View reviewed changes

oranagra reviewed Jul 11, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits July 11, 2022 15:13

drop info and backup changes for now

5fc6f45

cluster help: take oran's suggestion

b9e5bc7

oranagra approved these changes Jul 11, 2022

View reviewed changes

oranagra merged commit 35e8ae3 into redis:unstable Jul 11, 2022

enjoy-binbin deleted the redis_cli_cluster_bus_port branch July 11, 2022 08:23

oranagra mentioned this pull request Jul 11, 2022

Release 7.0.3 #10962

Merged

enjoy-binbin mentioned this pull request Jul 28, 2022

fix redis-cli cluster create use cluster-announce-bus-port option #6442

Open

enjoy-binbin mentioned this pull request Aug 19, 2022

Fix Invalid node address specified in redis-cli --cluster create/add-node #11151

Merged

Add cluster-port support to redis-cli --cluster #10344

Add cluster-port support to redis-cli --cluster #10344

Uh oh!

Conversation

enjoy-binbin commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwware commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

satheeshaGowda commented Mar 9, 2022

Uh oh!

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Jul 7, 2022

Uh oh!

enjoy-binbin commented Jul 7, 2022

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Jul 11, 2022

Uh oh!

enjoy-binbin commented Jul 11, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enjoy-binbin commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jul 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

enjoy-binbin commented Feb 25, 2022 •

edited

Loading

hwware commented Feb 25, 2022 •

edited

Loading

enjoy-binbin commented Jul 11, 2022 •

edited

Loading