Add 4 commands for sentinel and update most test cases and json files #789

hwware · 2024-07-15T21:20:15Z

Add 4 new commands for Sentinel (reference #36)

Sentinel GET-PRIMARY-ADDR-BY-NAME
Sentinel PRIMARY
Sentinel PRIMARIES
Sentinel IS-PRIMARY-DOWN-BY-ADDR

and deprecate 4 old commands:

Sentinel GET-MASTER-ADDR-BY-NAME
Sentinel MASTER
Sentinel MASTERS
Sentinel IS-MASTER-DOWN-BY-ADDR

and all sentinel tests pass here https://github.com/hwware/valkey/actions/runs/9962102363/job/27525124583

Note:

runtest-sentinel pass all test cases
I finished a sentinel rolling upgrade test: 1 primary 2 replicas 3 sentinel
there are 4 steps in this test scenario:
step 1: all 3 sentinel nodes run old sentinel, shutdown primary, and then new primary can be voted successfully.
step 2: replace sentinel 1 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully
step 3: replace sentinel 2 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully
step 4: replace sentinel 3 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully

We can see, even mixed version sentinel running, whole system still works.

codecov · 2024-07-15T21:31:19Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.30%. Comparing base (6cb86ff) to head (87c9469).
Report is 65 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/sentinel.c	0.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #789      +/-   ##
============================================
- Coverage     70.33%   70.30%   -0.04%     
============================================
  Files           112      112              
  Lines         61489    61491       +2     
============================================
- Hits          43249    43230      -19     
- Misses        18240    18261      +21

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/sentinel.c	`0.00% <0.00%> (ø)`

... and 6 files with indirect coverage changes

enjoy-binbin · 2024-07-18T06:30:05Z

src/sentinel.c

@@ -4483,7 +4485,7 @@ void sentinelAskPrimaryStateToOtherSentinels(sentinelValkeyInstance *primary, in
        /* Ask */
        ll2string(port, sizeof(port), primary->addr->port);
        retval = redisAsyncCommand(
-            ri->link->cc, sentinelReceiveIsPrimaryDownReply, ri, "%s is-master-down-by-addr %s %s %llu %s",


the old sentinel does not recognize the new command, but i suppose it is ok.

yes, once we update this part, all sentinels should run on the same version.

is there a rolling upgrade scenario for sentinels where the user runs mixed versions of sentinels temporarily?

Yes, there is. For example, this Helm chart https://github.com/bitnami/charts/tree/main/bitnami/valkey sets up e.g. a k8s StatefulSet with three pods running a sentinel and a valkey container each. During a RollingUpdate of the StatefulSet, there will be mixed versions of sentinel and valkey temporarily. The sentinels will still need to find a quorum and initiate failovers during this time (if the primary gets updated)

make sense. @hwware I think we should leave this command as it is. Rolling upgrades can take time and we need to be resilient to any failures during this transitional period.

make sense. @hwware I think we should leave this command as it is. Rolling upgrades can take time and we need to be resilient to any failures during this transitional period.

Agree with you. Because @gmbnomis provides me a real world case, let us just leave these commands as they are, util I find a solution to solve the problem.

@gmbnomis @PingXie I just update the top comment, and describe a test scenario with mixed version sentinel. Please let me if you have more concern.

So, sentinel accepts the new command variants, but it still uses the old ones internally? (which is "IS-MASTER-DOWN-BY-ADDR" only, if I get this correctly)

That looks reasonable. We could get rid of the old forms in some later release when we can be sure that all sentinel versions that are supported during an upgrade understand the new form.

Thank you for explicitly testing the mixed sentinel version upgrade scenario. Highly appreciated!

So, sentinel accepts the new command variants, but it still uses the old ones internally? (which is "IS-MASTER-DOWN-BY-ADDR" only, if I get this correctly)

That looks reasonable. We could get rid of the old forms in some later release when we can be sure that all sentinel versions that are supported during an upgrade understand the new form.

Thank you for explicitly testing the mixed sentinel version upgrade scenario. Highly appreciated!

I keep the IS-MASTER-DOWN-BY-ADDR only internal, for user, we still encourage them to use new command format. Thanks

enjoy-binbin · 2024-07-18T06:31:15Z

tests/sentinel/tests/00-base.tcl

@@ -57,16 +57,16 @@ test "SENTINEL PENDING-SCRIPTS returns the information about pending scripts" {
 }

 test "SENTINEL PRIMARIES returns a list of monitored primaries" {
-    assert_match "*mymaster*" [S 0 SENTINEL MASTERS]


i guess we also need some dummy tests that can cover the old commands, otherwise the reply-schemas-validator CI will fail (and we also need it to make sure the old commands can work).

Please check https://github.com/hwware/valkey/actions/runs/9962102363/job/27525129293, it looks like reply-schemas-validator CI can accept all changes, and it passes. But adding some tests to cover old commands is fine.

Thanks BinBin，I find 5 sentinel deprecated commands are not hit as below

But I am curious why some many commands are not hit? it is normal?

that is because you just trigger a sentinel-test, it does not run the other tests, so those commands are not hit

enjoy-binbin · 2024-07-18T06:32:34Z

tests/sentinel/tests/00-base.tcl

@@ -200,7 +200,7 @@ test "SENTINEL RESET can resets the primary" {
    for {set j 0} {$j < 10} {incr j} {
        assert_equal 1 [S 0 SENTINEL RESET mymaster]
        set res1 [llength [S 0 SENTINEL SENTINELS mymaster]]
-        set res2 [llength [S 0 SENTINEL SLAVES mymaster]]


lets keep it, i believe i add this for the old name coverage

got it, I will update it and add the comment for old name coverage.

PingXie

LGTM overall with just one pending question.

PingXie · 2024-07-19T16:29:20Z

src/sentinel.c

@@ -4483,7 +4485,7 @@ void sentinelAskPrimaryStateToOtherSentinels(sentinelValkeyInstance *primary, in
        /* Ask */
        ll2string(port, sizeof(port), primary->addr->port);
        retval = redisAsyncCommand(
-            ri->link->cc, sentinelReceiveIsPrimaryDownReply, ri, "%s is-master-down-by-addr %s %s %llu %s",


is there a rolling upgrade scenario for sentinels where the user runs mixed versions of sentinels temporarily?

hwware · 2024-07-23T15:42:28Z

LGTM overall with just one pending question.

According to the latest modification, there is no rolling upgrade scenario because for the redisAsyncCommand in old version, it will send is-master-down-by-addr command, however for new version sentinel, it will send is-primary-down-by-addr.

In the case, if one sentinel node just upgrades, and it is "lucky" it detects the primary down, then it will send command "SENTINEL IS-PRIMARY-DOWN-BY-ADDR", however, old version sentinel can not recognize this command. Failover will not happen.
Thus, when clients decide to upgrade sentinel version, they should upgrade all bin files in all sentinel nodes in case the mentioned scenario happens. But from my personal experience, it happens with very low probability as the below reasons:

After sentinel bin file is replaced, sentinel starts with very short time, there is no data loaded.
The sentinel conf file no changes, and since the old version sentinel and new version sentinel keep high compatible, it does not need to check primary nodes and replicas node. It just need to send PING message as usual.

@PingXie @zuiderkwast

gmbnomis · 2024-07-30T22:45:20Z

In the case, if one sentinel node just upgrades, and it is "lucky" it detects the primary down, then it will send command "SENTINEL IS-PRIMARY-DOWN-BY-ADDR", however, old version sentinel can not recognize this command. Failover will not happen. ...

@hwware Please see my comment #789 (comment), which is a "real world" example of a rolling upgrade scenario which relies on failover to happen.

hwware · 2024-07-31T14:30:18Z

In the case, if one sentinel node just upgrades, and it is "lucky" it detects the primary down, then it will send command "SENTINEL IS-PRIMARY-DOWN-BY-ADDR", however, old version sentinel can not recognize this command. Failover will not happen. ...

@hwware Please see my comment #789 (comment), which is a "real world" example of a rolling upgrade scenario which relies on failover to happen.

Thanks for your information. Let us just leave these commands as they are now.

Signed-off-by: hwware <wen.hui.ware@gmail.com>

madolson · 2024-08-13T22:40:27Z

I assume the major decision is the deprecation and renaming, which I am OK with. So just indicating my +1 on that.

PingXie

LGTM. Thanks Wen!

zuiderkwast

Just to be sure I understand correctly:

The sentinels still use the MASTER commands (GET-MASTER-ADDR-BY-NAME, etc.) when they talk to each other, so we can support a rolling upgrade of sentinels.

In the future (Valkey 9?) the sentinels can use the PRIMARY variant of the commands instead. We still can't delete the MASTER commands, because we still want to support rolling upgrade of Sentinel from 8 to 9. Even later (Valkey 10?) we can delete the MASTER variants.

Correct?

hwware · 2024-08-16T13:46:07Z

Just to be sure I understand correctly:

The sentinels still use the MASTER commands (GET-MASTER-ADDR-BY-NAME, etc.) when they talk to each other, so we can support a rolling upgrade of sentinels.

Yes, when the communication between 2 sentinel nodes, the MASTER command is used.

In the future (Valkey 9?) the sentinels can use the PRIMARY variant of the commands instead. We still can't delete the MASTER commands, because we still want to support rolling upgrade of Sentinel from 8 to 9. Even later (Valkey 10?) we can delete the MASTER variants.

I will find a way to delete the master command ASAP, maybe in Valkey 8.2 version or Valkey 9. Valkey 10 is too long (I hope not).

Correct?

zuiderkwast · 2024-08-16T17:46:36Z

I see no hurry in deleting the deprecated commands, as long as there is a plan. I think we need to keep them as long as we support rolling upgrades from 8.0.

…valkey-io#789) Add 4 new commands for Sentinel (reference valkey-io#36) Sentinel GET-PRIMARY-ADDR-BY-NAME Sentinel PRIMARY Sentinel PRIMARIES Sentinel IS-PRIMARY-DOWN-BY-ADDR and deprecate 4 old commands: Sentinel GET-MASTER-ADDR-BY-NAME Sentinel MASTER Sentinel MASTERS Sentinel IS-MASTER-DOWN-BY-ADDR and all sentinel tests pass here https://github.com/hwware/valkey/actions/runs/9962102363/job/27525124583 Note: 1. runtest-sentinel pass all test cases 2. I finished a sentinel rolling upgrade test: 1 primary 2 replicas 3 sentinel there are 4 steps in this test scenario: step 1: all 3 sentinel nodes run old sentinel, shutdown primary, and then new primary can be voted successfully. step 2: replace sentinel 1 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully step 3: replace sentinel 2 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully step 4: replace sentinel 3 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully We can see, even mixed version sentinel running, whole system still works. --------- Signed-off-by: hwware <wen.hui.ware@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>

…#789) Add 4 new commands for Sentinel (reference #36) Sentinel GET-PRIMARY-ADDR-BY-NAME Sentinel PRIMARY Sentinel PRIMARIES Sentinel IS-PRIMARY-DOWN-BY-ADDR and deprecate 4 old commands: Sentinel GET-MASTER-ADDR-BY-NAME Sentinel MASTER Sentinel MASTERS Sentinel IS-MASTER-DOWN-BY-ADDR and all sentinel tests pass here https://github.com/hwware/valkey/actions/runs/9962102363/job/27525124583 Note: 1. runtest-sentinel pass all test cases 2. I finished a sentinel rolling upgrade test: 1 primary 2 replicas 3 sentinel there are 4 steps in this test scenario: step 1: all 3 sentinel nodes run old sentinel, shutdown primary, and then new primary can be voted successfully. step 2: replace sentinel 1 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully step 3: replace sentinel 2 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully step 4: replace sentinel 3 with new sentinel bin file, and then shutdown primary, and then another new primary can be voted successfully We can see, even mixed version sentinel running, whole system still works. --------- Signed-off-by: hwware <wen.hui.ware@gmail.com>

hwware changed the title ~~Update 4 commands json file and update most test cases~~ Add 4 commands for sentinel and update most test cases and json files Jul 15, 2024

hwware force-pushed the update-sentinel-commands-with-master-keyword branch from 142a1ad to 737cd6a Compare July 16, 2024 18:13

hwware marked this pull request as ready for review July 16, 2024 18:49

hwware requested review from PingXie and enjoy-binbin July 16, 2024 18:49

enjoy-binbin reviewed Jul 18, 2024

View reviewed changes

enjoy-binbin added the major-decision-pending Major decision pending by TSC team label Jul 18, 2024

hwware force-pushed the update-sentinel-commands-with-master-keyword branch 3 times, most recently from 969ecb1 to ef82e8e Compare July 18, 2024 20:49

PingXie reviewed Jul 19, 2024

View reviewed changes

hwware force-pushed the update-sentinel-commands-with-master-keyword branch from ef82e8e to 211d133 Compare August 12, 2024 20:01

hwware added 5 commits August 13, 2024 19:08

add 4 commands json file and update most test cases

c93e260

Signed-off-by: hwware <wen.hui.ware@gmail.com>

finish all update for these 4 commands

729d360

Signed-off-by: hwware <wen.hui.ware@gmail.com>

add test cases for all deprecated commands

2fd06d2

Signed-off-by: hwware <wen.hui.ware@gmail.com>

fix spelling issue

9a49f8c

Signed-off-by: hwware <wen.hui.ware@gmail.com>

keep is-master-down-by-addr only one place

87c9469

Signed-off-by: hwware <wen.hui.ware@gmail.com>

hwware force-pushed the update-sentinel-commands-with-master-keyword branch 2 times, most recently from ecd4b2b to 87c9469 Compare August 13, 2024 19:13

PingXie approved these changes Aug 16, 2024

View reviewed changes

zuiderkwast approved these changes Aug 16, 2024

View reviewed changes

hwware merged commit 33c7ca4 into valkey-io:unstable Aug 16, 2024
48 checks passed

enjoy-binbin added release-notes This issue should get a line item in the release notes and removed major-decision-pending Major decision pending by TSC team labels Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 4 commands for sentinel and update most test cases and json files #789

Add 4 commands for sentinel and update most test cases and json files #789

hwware commented Jul 15, 2024 •

edited

Loading

codecov bot commented Jul 15, 2024 •

edited

Loading

enjoy-binbin Jul 18, 2024

hwware Jul 18, 2024

PingXie Jul 19, 2024

gmbnomis Jul 30, 2024

PingXie Jul 31, 2024

hwware Jul 31, 2024

hwware Aug 13, 2024

gmbnomis Aug 13, 2024

hwware Aug 14, 2024

enjoy-binbin Jul 18, 2024

hwware Jul 18, 2024

enjoy-binbin Jul 18, 2024

hwware Jul 18, 2024 •

edited

Loading

enjoy-binbin Jul 18, 2024

enjoy-binbin Jul 18, 2024

hwware Jul 18, 2024

PingXie left a comment

PingXie Jul 19, 2024

hwware commented Jul 23, 2024

gmbnomis commented Jul 30, 2024

hwware commented Jul 31, 2024

madolson commented Aug 13, 2024

PingXie left a comment

zuiderkwast left a comment •

edited

Loading

hwware commented Aug 16, 2024

zuiderkwast commented Aug 16, 2024

Add 4 commands for sentinel and update most test cases and json files #789

Add 4 commands for sentinel and update most test cases and json files #789

Conversation

hwware commented Jul 15, 2024 • edited Loading

codecov bot commented Jul 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwware Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwware commented Jul 23, 2024

gmbnomis commented Jul 30, 2024

hwware commented Jul 31, 2024

madolson commented Aug 13, 2024

PingXie left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment • edited Loading

Choose a reason for hiding this comment

hwware commented Aug 16, 2024

zuiderkwast commented Aug 16, 2024

hwware commented Jul 15, 2024 •

edited

Loading

codecov bot commented Jul 15, 2024 •

edited

Loading

hwware Jul 18, 2024 •

edited

Loading

zuiderkwast left a comment •

edited

Loading