-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
[NET-3865] [Supportability] Additional Information in the output of 'consul operator raft list-peers' #17582
Changes from 14 commits
2f94024
7626d09
79aabc9
44eee41
1390353
d118518
d46a074
539dbc7
604b616
3a63b3d
008946c
509062e
cf22ea5
eb69358
5c2a587
8e1f728
360ab9e
dc25344
c461f59
3631708
5d479e8
40dc1e1
e7afa34
8c1bab4
6b76d59
c001e59
64e3b0a
7f8d9d8
fdad3bb
1adcb15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
```release-note:feature | ||
improved consul operator raft list-peers command added CommitIndex as well as Trails Leader By in the output table | ||
Commit Index is the LastIndex from /v1/operator/autopilot/health and Trails Leader By is the difference from leader | ||
last index and followers last index | ||
``` | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,3 +122,20 @@ func (op *Operator) RaftRemovePeerByID(id string, q *WriteOptions) error { | |
} | ||
return nil | ||
} | ||
|
||
// GetAutoPilotHealth is used to query the autopilot health. | ||
func (op *Operator) GetAutoPilotHealth(q *QueryOptions) (*OperatorHealthReply, error) { | ||
r := op.c.newRequest("GET", "/v1/operator/autopilot/health") | ||
r.setQueryOptions(q) | ||
_, resp, err := op.c.doRequest(r) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer closeResponseBody(resp) | ||
|
||
var out OperatorHealthReply | ||
if err := decodeBody(resp, &out); err != nil { | ||
return nil, err | ||
} | ||
return &out, nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you're duplicating the code for AutoPilotServerHealth instead of calling it directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I did not see the code before. Thanks! |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,8 +70,29 @@ func raftListPeers(client *api.Client, stale bool) (string, error) { | |
return "", fmt.Errorf("Failed to retrieve raft configuration: %v", err) | ||
} | ||
|
||
autoPilotReply, err := client.Operator().GetAutoPilotHealth(q) | ||
if err != nil { | ||
return "", fmt.Errorf("Failed to retrieve autopilot health: %v", err) | ||
} | ||
|
||
serverHealthDataMap := make(map[string]api.ServerHealth) | ||
leaderLastCommitIndex := uint64(0) | ||
|
||
for _, serverHealthData := range autoPilotReply.Servers { | ||
serverHealthDataMap[serverHealthData.ID] = serverHealthData | ||
} | ||
|
||
for _, s := range reply.Servers { | ||
if s.Leader { | ||
serverHealthDataLeader, ok := serverHealthDataMap[s.ID] | ||
if ok { | ||
leaderLastCommitIndex = serverHealthDataLeader.LastIndex | ||
} | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of making a second API request to get a single value ( @dhiaayachi or @boxofrad Are there prior established patterns for this type of change that gets an additional piece of data or are multiple APIs calls the norm in the implemenation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @analogue extending the I don't think multiple API calls is a norm here and I would vote for having all the data needed included in a single API call as much as possible. It will also give the possibility to users to use the API to perform some automation, like watching the servers before going forward to the next step of a rolling upgrade. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
// Format it as a nice table. | ||
result := []string{"Node\x1fID\x1fAddress\x1fState\x1fVoter\x1fRaftProtocol"} | ||
result := []string{"Node\x1fID\x1fAddress\x1fState\x1fVoter\x1fRaftProtocol\x1fCommit Index\x1fTrails Leader By"} | ||
for _, s := range reply.Servers { | ||
raftProtocol := s.ProtocolVersion | ||
|
||
|
@@ -82,8 +103,22 @@ func raftListPeers(client *api.Client, stale bool) (string, error) { | |
if s.Leader { | ||
state = "leader" | ||
} | ||
result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s\x1f%v\x1f%s", | ||
s.Node, s.ID, s.Address, state, s.Voter, raftProtocol)) | ||
|
||
serverHealthData, ok := serverHealthDataMap[s.ID] | ||
if ok { | ||
trailsLeaderBy := leaderLastCommitIndex - serverHealthData.LastIndex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @analogue - Will leader last commit index be always greater than follower? or should I take abs(leaderLastCommitIndex - serverHealthData.LastIndex) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I believe the leader's last commit index should always be greater than or equal to the follower's last commit index. |
||
trailsLeaderByText := fmt.Sprintf("%d Commits", trailsLeaderBy) | ||
if s.Leader { | ||
trailsLeaderByText = "_" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer |
||
} else if trailsLeaderBy <= 1 { | ||
trailsLeaderByText = fmt.Sprintf("%d Commit", trailsLeaderBy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer lowercase |
||
} | ||
result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s\x1f%v\x1f%s\x1f%v\x1f%s", | ||
s.Node, s.ID, s.Address, state, s.Voter, raftProtocol, serverHealthData.LastIndex, trailsLeaderByText)) | ||
} else { | ||
result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s\x1f%v\x1f%s\x1f%v", | ||
s.Node, s.ID, s.Address, state, s.Voter, raftProtocol, "_")) | ||
} | ||
} | ||
|
||
return columnize.Format(result, &columnize.Config{Delim: string([]byte{0x1f})}), nil | ||
|
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.
Maybe short and sweet would be preferable here given the audience are customers interested in changes and how they benefit/affect them.
e.g.
consul operator raft list-peers
shows the number of commits each follower is trailing the leader by to aid in troubleshooting.