Trigger a save of the cluster configuration file before shutting down #822

enjoy-binbin · 2024-07-24T03:22:34Z

The cluster configuration file is the metadata "database" for the
cluster. It is best to trigger a save when shutdown the server, to
avoid inconsistent content that is not refreshed.

We save the nodes.conf whenever something that affects the nodes.conf
has changed. But we are saving nodes.conf in clusterBeforeSleep, and
some events may save it without a fsync, there is a time gap.

And shutdown has its own save seems good to me, it doesn't need to
care about the others.

At the same time, a comment is added in unlock nodes.conf to explain
why we actively unlock when shutdown.

The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. Signed-off-by: Binbin <[email protected]>

codecov · 2024-07-24T03:35:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.28%. Comparing base (0053429) to head (f69f803).
Report is 56 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #822      +/-   ##
============================================
- Coverage     70.64%   70.28%   -0.37%     
============================================
  Files           112      113       +1     
  Lines         61531    61732     +201     
============================================
- Hits          43467    43386      -81     
- Misses        18064    18346     +282

Files with missing lines	Coverage Δ
src/cluster_legacy.c	`85.39% <100.00%> (-0.11%)`	⬇️
src/server.c	`88.56% <100.00%> (-0.01%)`	⬇️

... and 19 files with indirect coverage changes

src/server.c

Signed-off-by: Binbin <[email protected]>

src/cluster.h

src/cluster_legacy.c

Co-authored-by: Harkrishn Patro <[email protected]> Signed-off-by: Binbin <[email protected]>

hpatro · 2024-07-26T17:23:43Z

Test failure on the PR seems unrelated. We should put effort on stabilizing these tests. :|

*** [err]: Empty-shard migration target is auto-updated after failover in target shard in tests/unit/cluster/slot-migration.tcl
incorrect slot state on R 0: expected [609->-7163d8afa4ecd66656c474a3d4547ad1d35f6716]; got [609->-b5854de4879d64bdbafb24cf90e81fdd04a8b9e8]
*** [err]: Empty-shard migration source is auto-updated after failover in source shard in tests/unit/cluster/slot-migration.tcl
incorrect slot state on R 0: expected [609->-7163d8afa4ecd66656c474a3d4547ad1d35f6716]; got [609->-b5854de4879d64bdbafb24cf90e81fdd04a8b9e8]
Cleanup: may take some time... OK

…nfig Signed-off-by: Binbin <[email protected]>

zuiderkwast

LGTM,

but we save the nodes.conf whenever something that affects the nodes.conf has changed. Why do we need to do it at shutdown too?

enjoy-binbin · 2024-08-27T02:30:38Z

we are saving nodes.conf in beforeSleep, and event save it without fsync, there is a time gap i think, and shutdown has its own save seems good to me (it doesn't need to care about the others.)

zuiderkwast

OK

PingXie

LGTM with one comment.

src/cluster_legacy.c

Signed-off-by: Binbin <[email protected]>

…valkey-io#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]>

…valkey-io#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]> Signed-off-by: Ping Xie <[email protected]>

…valkey-io#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]>

…valkey-io#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]> Signed-off-by: Ping Xie <[email protected]>

…#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]> Signed-off-by: Ping Xie <[email protected]>

…valkey-io#822) The cluster configuration file is the metadata "database" for the cluster. It is best to trigger a save when shutdown the server, to avoid inconsistent content that is not refreshed. We save the nodes.conf whenever something that affects the nodes.conf has changed. But we are saving nodes.conf in clusterBeforeSleep, and some events may save it without a fsync, there is a time gap. And shutdown has its own save seems good to me, it doesn't need to care about the others. At the same time, a comment is added in unlock nodes.conf to explain why we actively unlock when shutdown. Signed-off-by: Binbin <[email protected]> Signed-off-by: naglera <[email protected]>

enjoy-binbin requested a review from PingXie July 24, 2024 03:22

hpatro reviewed Jul 24, 2024

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

adding a new clusterHandleServerShutdown

7691559

Signed-off-by: Binbin <[email protected]>

enjoy-binbin commented Jul 25, 2024

View reviewed changes

src/cluster.h Show resolved Hide resolved

hpatro approved these changes Jul 26, 2024

View reviewed changes

src/cluster_legacy.c Outdated Show resolved Hide resolved

Update src/cluster_legacy.c

dcac0d2

Co-authored-by: Harkrishn Patro <[email protected]> Signed-off-by: Binbin <[email protected]>

enjoy-binbin requested a review from zuiderkwast August 24, 2024 10:01

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

9a89d0b

…nfig Signed-off-by: Binbin <[email protected]>

enjoy-binbin requested a review from madolson August 24, 2024 10:02

zuiderkwast reviewed Aug 26, 2024

View reviewed changes

zuiderkwast approved these changes Aug 27, 2024

View reviewed changes

PingXie reviewed Aug 27, 2024

View reviewed changes

src/cluster_legacy.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits August 27, 2024 11:55

code review: holding the lock until the very end

9849a77

Signed-off-by: Binbin <[email protected]>

Add comment about the unlock

f69f803

Signed-off-by: Binbin <[email protected]>

PingXie approved these changes Aug 27, 2024

View reviewed changes

enjoy-binbin added the release-notes This issue should get a line item in the release notes label Sep 4, 2024

enjoy-binbin merged commit 38457b7 into valkey-io:unstable Sep 12, 2024
47 checks passed

enjoy-binbin deleted the save_cluster_config branch September 12, 2024 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger a save of the cluster configuration file before shutting down #822

Trigger a save of the cluster configuration file before shutting down #822

enjoy-binbin commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading

hpatro commented Jul 26, 2024

zuiderkwast left a comment

enjoy-binbin commented Aug 27, 2024

zuiderkwast left a comment

PingXie left a comment

Trigger a save of the cluster configuration file before shutting down #822

Trigger a save of the cluster configuration file before shutting down #822

Conversation

enjoy-binbin commented Jul 24, 2024 • edited Loading

codecov bot commented Jul 24, 2024 • edited Loading

Codecov Report

hpatro commented Jul 26, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

enjoy-binbin commented Aug 27, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

enjoy-binbin commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading