Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

try to perform a leadership transfer when leaving #11376

Merged
merged 2 commits into from
Oct 21, 2021
Merged

try to perform a leadership transfer when leaving #11376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b03eabe
try to perform a leadership transfer when leaving
dhiaayachi Oct 21, 2021
818c160
add a changelog
dhiaayachi Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/11376.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:enhancement
try to transfer leadership to another server when leaving.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

```
28 changes: 26 additions & 2 deletions agent/consul/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
"sync/atomic"
"time"

"github.com/hashicorp/go-version"

"github.com/armon/go-metrics"
connlimit "github.com/hashicorp/go-connlimit"
"github.com/hashicorp/go-hclog"
Expand Down Expand Up @@ -92,6 +94,8 @@ const (
// from Serf with the Catalog. If this is exhausted we will drop updates,
// and wait for a periodic reconcile.
reconcileChSize = 256

LeaderTransferMinVersion = "1.6.0"
)

const (
Expand Down Expand Up @@ -992,8 +996,28 @@ func (s *Server) Leave() error {
// removed for some reasonable period of time.
isLeader := s.IsLeader()
if isLeader && numPeers > 1 {
if err := s.autopilot.RemoveServer(raft.ServerID(s.config.NodeID)); err != nil {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

// 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)
}
Comment on lines +999 to +1020
Copy link
Contributor

@dnephin dnephin Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Two high level questions/suggestions:

  1. 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.
  2. Can we extract any of this to a function to remove the nested else statements, 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)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

Expand Down