try to perform a leadership transfer when leaving #11376
Merged
Conversation
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
kyhavlov
approved these changes
Oct 21, 2021
dnephin
reviewed
Oct 21, 2021
| @@ -0,0 +1,3 @@ | |||
| ```release-note:enhancement | |||
| try to transfer leadership to another server when leaving. | |||
There was a problem hiding this comment.
What do you think about something like this?
Suggested change
| try to transfer leadership to another server when leaving. | |
| raft: Consul leaders will attempt to transfer leadership to another server as part of gracefully leaving the cluster. |
| s.logger.Error("failed to remove ourself as a Raft peer", "error", err) | ||
| leadershipTransferVersion := version.Must(version.NewVersion(LeaderTransferMinVersion)) | ||
| removeServer := false | ||
| if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, leadershipTransferVersion); !ok { |
There was a problem hiding this comment.
Shouldn't this be if ok ?
Suggested change
| if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, leadershipTransferVersion); !ok { | |
| if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, leadershipTransferVersion); ok { |
Comment on lines
+999
to
+1020
| leadershipTransferVersion := version.Must(version.NewVersion(LeaderTransferMinVersion)) | ||
| removeServer := false | ||
| if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, leadershipTransferVersion); !ok { | ||
| // Transfer leadership to another node then leave the cluster | ||
| future := s.raft.LeadershipTransfer() | ||
| if err := future.Error(); err != nil { | ||
| s.logger.Error("failed to transfer leadership, removing the server", "error", err) | ||
| // leadership transfer failed, fallback to removing the server from raft | ||
| removeServer = true | ||
| } else { | ||
| // we are not leader anymore, continue the flow to leave as follower | ||
| isLeader = false | ||
| } | ||
| } else { | ||
| // Leadership transfer is not available in the current version, fallback to removing the server from raft | ||
| removeServer = true | ||
| } | ||
| if removeServer { | ||
| future := s.raft.RemoveServer(raft.ServerID(s.config.NodeID), 0, 0) | ||
| if err := future.Error(); err != nil { | ||
| s.logger.Error("failed to remove ourself as raft peer", "error", err) | ||
| } |
There was a problem hiding this comment.
Two high level questions/suggestions:
- Do these comments really help readability? Generally I try to only write docstring comments unless there is something very unusual, and everything here looks pretty expected. Personally I think it reads better without the comments.
- Can we extract any of this to a function to remove the nested
elsestatements, and stateful variables?
func (s *Server) leadershipTransfer() (success bool) {
leadershipTransferVersion := version.Must(version.NewVersion(LeaderTransferMinVersion))
ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, leadershipTransferVersion)
if !ok {
return false
}
future := s.raft.LeadershipTransfer()
if err := future.Error(); err != nil {
s.logger.Error("failed to transfer leadership, removing the server", "error", err)
return false
}
return true
}Then in the caller:
if !s.leadershipTransfer() {
future := s.raft.RemoveServer(raft.ServerID(s.config.NodeID), 0, 0)
if err := future.Error(); err != nil {
s.logger.Error("failed to remove ourself as raft peer", "error", err)
}
}There was a problem hiding this comment.
The main reason of the comments is because I did not find a good way of removing the nested if else, I will attempt the extract in the new PR.
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/480806. |
|
🍒✅ Cherry pick of commit f424faf onto |
hc-github-team-consul-core
pushed a commit
that referenced
this pull request
Oct 21, 2021
* try to perform a leadership transfer when leaving * add a changelog
dhiaayachi
added a commit
that referenced
this pull request
Oct 21, 2021
This reverts commit f424faf.
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/481009. |
|
🍒✅ Cherry pick of commit f424faf onto |
hc-github-team-consul-core
pushed a commit
that referenced
this pull request
Oct 21, 2021
* try to perform a leadership transfer when leaving * add a changelog
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 is a port of #11242 (1.8.x) for main