Skip to content

Conversation

@enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Jun 10, 2025

Add SAFE option to SHUTDOWN. If we passed SAFE, the SHUTDOWN
will refuse to shutdown if it is not safe to shutdown. Like if
myself is a voting primary, it will refuse to shutdown. This
avoids the situation where a replica suddenly becomes the primary
when we shutting down the replica, or shutting down a primary node
by mistake and then causing the cluster to down.

Add SAFE option to SHUTDOWN command.
Add safe option to shutdown-on-sigint and shutdown-on-sigterm.

Note that SAFE cannot prevent FORCE, in the case of FORCE, SAFE
will print the relevant logs and do the FORCE shutdown, we allow
this combination.

Doc PR: valkey-io/valkey-doc#325

Add SAFE option to SHUTDOWN. If we passed SAFE, the SHUTDOWN
will refuse to shutdown if it is not safe to shutdown. Like if
myself is a voting primary, it will refuse to shutdown. This
avoids the situation where a replica suddenly becomes the primary
when we shutting down the replica, or shutting down a primary node
by mistake and then causing the cluster to down.

Add SAFE option to SHUTDOWN command.
Add safe option to shutdown-on-sigint and shutdown-on-sigterm.

Note that safe cannot prevent force, in the case of force, safe
will print the relevant logs and do the force shutdown.

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin added the major-decision-pending Major decision pending by TSC team label Jun 10, 2025
@codecov
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.35%. Comparing base (5cc2b25) to head (bd72a8d).
Report is 41 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2195      +/-   ##
============================================
- Coverage     71.50%   71.35%   -0.15%     
============================================
  Files           122      123       +1     
  Lines         66452    66936     +484     
============================================
+ Hits          47515    47764     +249     
- Misses        18937    19172     +235     
Files with missing lines Coverage Δ
src/cluster_legacy.c 86.84% <100.00%> (+0.02%) ⬆️
src/commands.def 100.00% <ø> (ø)
src/config.c 78.47% <ø> (ø)
src/db.c 90.11% <100.00%> (+0.06%) ⬆️
src/server.c 88.09% <100.00%> (+0.12%) ⬆️
src/server.h 100.00% <ø> (ø)

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@madolson
Copy link
Member

Didn't really read the code, but directionally I'm OK with adding a safe option to shutdown.

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now actually read the code and am OK with it.

@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Jun 16, 2025
@enjoy-binbin enjoy-binbin added release-notes This issue should get a line item in the release notes needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Jun 17, 2025
Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slightly more detailed reading of the text with an English lense.

@enjoy-binbin
Copy link
Member Author

thanks for the review and suggestions, sorry about that, i have always had this problem, chinese english as a Chinese, i am very bad at grammar.

Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Binbin <[email protected]>
@enjoy-binbin
Copy link
Member Author

@valkey-io/core-team Do any of you want to review (or ack) it before the final merge?

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Signed-off-by: Binbin <[email protected]>
@madolson madolson moved this to Todo in Valkey 9.0 Jun 30, 2025
@madolson madolson moved this from Todo to In Progress in Valkey 9.0 Jun 30, 2025
Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin merged commit 1e05724 into valkey-io:unstable Jul 1, 2025
53 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Valkey 9.0 Jul 1, 2025
@enjoy-binbin enjoy-binbin deleted the shutdown_safe branch July 1, 2025 03:52
zuiderkwast pushed a commit to valkey-io/valkey-doc that referenced this pull request Jul 1, 2025
@zuiderkwast zuiderkwast removed the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Jul 1, 2025
zuiderkwast added a commit that referenced this pull request Jul 22, 2025
In #1091, a new config `auto-failover-on-shutdown` was added. This PR
changes the config to make it unified with other shutdown related
options. This feature has not yet been released, so it's not a breaking
change.

The auto-failover-on-shutdown config is replaced by

* A new "failover" option to the existing configs `shutdown-on-sigterm`
and `shutdown-on-sigint`.
* A new FAILOVER option to the SHUTDOWN command.

Additionally, a history entry is added to the SHUTDOWN command which was
missing in #2195.

Follow-up of #1091.

Signed-off-by: Viktor Söderqvist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants