Transfer leadership when establishLeadership fails #5247
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,10 @@ func (s *Server) monitorLeadership() { | |
| leaderLoop.Add(1) | ||
| go func(ch chan struct{}) { | ||
| defer leaderLoop.Done() | ||
| err := s.leaderLoop(ch) | ||
| if err != nil { | ||
| s.leadershipTransfer() | ||
| } | ||
| }(weAreLeaderCh) | ||
| s.logger.Printf("[INFO] consul: cluster leadership acquired") | ||
|
|
||
|
|
@@ -126,9 +129,23 @@ func (s *Server) monitorLeadership() { | |
| } | ||
| } | ||
|
|
||
| func (s *Server) leadershipTransfer() { | ||
| retryCount := 3 | ||
| for i := 0; i < retryCount; i++ { | ||
freddygv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| future := s.raft.LeadershipTransfer() | ||
| if err := future.Error(); err != nil { | ||
| s.logger.Printf("[ERR] consul: failed to transfer leadership attempt %d/%d: %v", i, retryCount, err) | ||
| } else { | ||
| s.logger.Printf("[ERR] consul: successfully transferred leadership attempt %d/%d", i, retryCount) | ||
| break | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| // leaderLoop runs as long as we are the leader to run various | ||
| // maintenance activities | ||
| func (s *Server) leaderLoop(stopCh chan struct{}) error { | ||
| // Fire a user event indicating a new leader | ||
| payload := []byte(s.config.NodeName) | ||
| for name, segment := range s.LANSegments() { | ||
|
|
@@ -178,7 +195,7 @@ RECONCILE: | |
| if err := s.revokeLeadership(); err != nil { | ||
| s.logger.Printf("[ERR] consul: failed to revoke leadership: %v", err) | ||
| } | ||
| return err | ||
|
There was a problem hiding this comment. This is the first usecase: we couldn't establish leadership and instead of retrying it after |
||
| } | ||
| establishedLeader = true | ||
| defer func() { | ||
|
|
@@ -204,7 +221,7 @@ WAIT: | |
| // down. | ||
| select { | ||
| case <-stopCh: | ||
| return nil | ||
| default: | ||
| } | ||
|
|
||
|
|
@@ -213,17 +230,21 @@ WAIT: | |
| for { | ||
| select { | ||
| case <-stopCh: | ||
| return nil | ||
| case <-s.shutdownCh: | ||
| return nil | ||
| case <-interval: | ||
| goto RECONCILE | ||
| case member := <-reconcileCh: | ||
| s.reconcileMember(member) | ||
| case index := <-s.tombstoneGC.ExpireCh(): | ||
| go s.reapTombstones(index) | ||
| case errCh := <-s.reassertLeaderCh: | ||
| err := reassert() | ||
| errCh <- err | ||
freddygv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if err != nil { | ||
| return err | ||
|
There was a problem hiding this comment. This is the second usecase: we couldn't reassert and instead of retrying it after There was a problem hiding this comment. Hmmm, when does this happen? I don't recall the specifics and wonder if by failing and stepping down we might end up causing new leadership stability issues? There was a problem hiding this comment. Lines 245 to 246 in 09a1a32 This happens after snapshot restore. Since the agent revokes leadership and immediately tries to establish it again, there is the possibility that it fails. When it does we are in the same situation as above - raft thinks this agent is a leader, but consul disagrees. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if leadership transfer fails? We exit the leader loop anyway and no longer act as the leader, but does raft still think we are the leader? That seems like a bad case to be in - roughly the same as the bug this is meant to be fixing although we at least made an attempt at telling raft we didn't want to be leader any more.
I can think of a couple of possible options here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a couple of changes to my PR, so that
leaderLoopis only left ifleadershipTransferwas successful. I think that mitigates the issues you are talking about.What happens now is that in case
leadershipTransferfails, the agent stays inleaderLoopand waits untilReconcileIntervalto retry toestablishLeadership(and totransferLeadershipin case it fails).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK although
ReconcileIntervalis 60 seconds which seems a lot to wait around for when we know the cluster is down. 🤔Should we instead consider looping indefinitely retrying every 5 seconds or something?
I think this is way better than before and eventually it should recover so it's good just wondering if it's easy to make that better quicker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reset the
intervalto 5 seconds when transfer leadership fails so that it retries establishLeadership again faster than before.