Handle promotion failures, deadlocks, and state transitions #656
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the uRaft module's reliability by addressing promotion deadlocks, improving state transition handling, and adding comprehensive observability through structured logging. The changes ensure proper recovery from failed promotions, prevent concurrent state transitions, and improve cluster stability during quorum loss scenarios.
Key Changes:
- Implemented
handlePromotionFailure()andcleanupDirtyPromotion()to recover from failed/interrupted promotions - Added deferred promotion/demotion mechanism using
force_promote_flag to handle overlapping state transitions - Enhanced logging throughout the codebase with structured syslog messages and a new
nodeToString()helper - Fixed election deadlock by resetting
current_termandleader_idwhen quorum is lost
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/uraft/uraftcontroller.h | Added new cleanup methods and force_promote_ flag for managing state transitions |
| src/uraft/uraftcontroller.cc | Implemented promotion failure handling, deferred transitions, enhanced logging, and improved timeout handling |
| src/uraft/uraft.h | Added nodeToString() helper method declaration |
| src/uraft/uraft.cc | Implemented nodeToString(), added syslog messages for election events and state transitions, fixed quorum loss deadlock |
| src/uraft/saunafs-uraft-helper.in | Added saunafs_cleanup_dirty_state() function and enhanced saunafs_drop_ip() with better error handling and logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ralcolea
force-pushed
the
fix-issue-576
branch
from
09ecc8c to
81c811b
Compare
ralcolea
force-pushed
the
fix-issue-576
branch
from
81c811b to
1cf746e
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces reliability and observability improvements to the uRaft module, addressing promotion/demotion handling, deadlock prevention, and cluster recovery. The changes include improved handling of promotion timeouts and failures, removal of redundant set_block_promotion() calls, enhanced logging and observability, fixes for election deadlocks, and deferred promotions during active demotion. The review focuses on identifying potential issues related to correctness and maintainability, particularly in the areas of error handling, logging consistency, and code clarity, with severity levels of medium, high, and critical.
ralcolea
requested review from
antuan96314,
dmga44,
ictus4u,
lgsilva3087,
rolysr and
uristdwarf
ralcolea
force-pushed
the
fix-issue-576
branch
from
1cf746e to
446922c
Compare
rolysr
reviewed
Oct 29, 2025
lgsilva3087
reviewed
Oct 29, 2025
ralcolea
force-pushed
the
fix-issue-576
branch
from
446922c to
ab1280a
Compare
lgsilva3087
approved these changes
Oct 30, 2025
ralcolea
force-pushed
the
fix-issue-576
branch
from
ab1280a to
115eced
Compare
rolysr
reviewed
Oct 31, 2025
rolysr
reviewed
Oct 31, 2025
This commit applies the following changes intended to fix issue #576: - Improve states transition during promotion by ensuring that after starting one demotion/promotion operation, it is not interrupted by other demotion/promotion operation. - Trigger cleanup after promotion timeout to ensure a temporary master can be restarted as a shadow after an interrupted metadata dump or a failed promotion that creates a remanent metadata.sfs.tmp file that prevent a master restart. - Fix Floating IP timing to ensure the manager is only started after a successful promotion. Signed-off-by: Crash <[email protected]>
Previously, certain execution paths could permanently block elections by calling `set_block_promotion(true)`, leading to a deadlock scenario where no Leader could be elected. Some of these calls were unnecessary, as the uRaft mechanism based on the loyalty agreement is already designed to prevent election loops correctly. This commit addresses issue #576 by: - Removing redundant calls to `set_block_promotion()` to avoid the deadlock scenario described above. - Improving logging in the `saunafs_drop_ip` function to provide more detailed information about the status of the floating IP removal. Signed-off-by: Crash <[email protected]>
This commit adds detailed logs for promotion and demotion events in uRaft that were not previously recorded. These logs improve visibility into node state transitions and help identify issues during debugging. Signed-off-by: Crash <[email protected]>
ralcolea
force-pushed
the
fix-issue-576
branch
from
115eced to
4218896
Compare
Previously, if a promotion was requested while a demotion was still in progress, the request was ignored. This could leave the node stuck in a follower state even when a promotion was desired. This commit adds a `force_promote_` flag to defer the promotion until the current demotion completes. Once demotion finishes, the controller automatically triggers a forced promotion and logs the transition. This ensures consistent role transitions and prevents promotion loss during overlapping commands. Signed-off-by: Crash <[email protected]>
ralcolea
force-pushed
the
fix-issue-576
branch
from
4218896 to
c0c20d5
Compare
This commit introduces the following review suggestions: - Updated the `cleanup` command in saunafs-uraft-helper to remove the temporary metadata.sfs.tmp file only if it is not holded by any process, and removed redundant logic for deleting `.sfsmaster.lock`. - Refactor `saunafs_drop_ip` command in saunafs-uraft-helper to make the code more readable. - Renamed `force_demote_` and `force_promote_` to `is_demote_pending_` and `is_promote_pending_` for clearer intent and consistency with their behavior. - Added documentation for newly introduced functions and attributes to improve code readability and maintenance. Co-authored-by: guillex <[email protected]> Co-authored-by: Rolando Sánchez Ramos <[email protected]> Signed-off-by: Crash <[email protected]>
ralcolea
force-pushed
the
fix-issue-576
branch
from
c0c20d5 to
7ad2beb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces several reliability and observability improvements to uRaft module, addressing multiple edge cases around promotion/demotion handling, deadlock prevention, and cluster recovery.
The main motivation was caused by issue #576 (promotion deadlocks), but there were introduced other improvements in the high-availability mechanism like logging and fixes when handling overlapping transitions or loss of quorum.
Key Changes
Improved handling of promotion timeouts and failures
handlePromotionFailure()andcleanupDirtyPromotion()inuRaftController.metadata.sfs.tmp).Removed redundant
set_block_promotion()callsLogging and Observability
Added structured syslog messages for:
Introduced
nodeToString()helper for consistent and readable node identifiers in logs.Enhanced logs in
saunafs_drop_ip()for clarity during IP removal and state cleanup.Deferred promotions during active demotion
force_promote_flag to defer a promotion request until demotion completes.Testing
The following Molecule scenarios were added and refactored to test the new changes:
uraft_kill_sfsmaster:This scenario was refactored to restart uRaft in the server where thesfsmasterprocess is killed, ensuring the cluster finishes in a stable state.uraft_loss_quorum:This scenario was modified to simulate a quorum loss and validate that the cluster can continue serving I/O operations once the quorum is restored.uraft_deadlock_multiple_elections:This is a new scenario introduced to validate that the uRaft cluster remains stable and free of deadlocks during consecutive election cycles.uraft_failed_promotion:This is a new scenario introduced to verify that the uRaft cluster can gracefully recover when a promotion attempt fails. It also ensures thathandlePromotionFailure()works as expected in case of failed promotions.For more details, see
fix-saunafs-issue-576branch in deployment (Ansible) repository.