Log leadership changes at manager level #2542
Conversation
manager/manager.go
Outdated
| // node is not a member of the raft quorum. this won't look very pretty | ||
| // in logs ("leadership changed from aslkdjfa to ErrNoRaftMember") but | ||
| // it also won't be very common | ||
| return "ErrNoRaftMember" |
There was a problem hiding this comment.
Change to "not yet part of a raft cluster"
| default: | ||
| id, err := m.raftNode.GetNodeIDByRaftID(leader) | ||
| // the only possible error here is "ErrMemberUnknown" | ||
| if err != nil { |
There was a problem hiding this comment.
Change to err != ErrMemberUnknown ?
There was a problem hiding this comment.
care to elaborate ? @dperny
| id, err := m.raftNode.GetNodeIDByRaftID(leader) | ||
| // the only possible error here is "ErrMemberUnknown" | ||
| if err != nil { | ||
| return "an unknown node" |
There was a problem hiding this comment.
This is not very useful in the logs either. Any way to make this somewhat less confusing ?
There was a problem hiding this comment.
probably not. it's an unlikely case that this happens, and if it does happen, you'll need to look at the logs output by the raft library.
There was a problem hiding this comment.
Let me clarify: my suggestion is to find out what is the scenario when we can get a ErrMemberUnknown and try to return a string that explains that scenario. @dperny
dperny
force-pushed
the
info-level-leader-elections
branch
2 times, most recently
from
5747e31
to
abfd94a
Compare
|
@anshulpundir changed to "not yet part of a raft cluster" but i disagree with the other two suggestions. |
dperny
force-pushed
the
info-level-leader-elections
branch
from
abfd94a
to
29abbd4
Compare
|
CI is still not green @dperny 🐳 misspell |
Updates (*Manager).handleLeadershipEvent to include a log message explaining where the leadership changed, using swarmkit node IDs instead of the raft node ids, at the Info level. Signed-off-by: Drew Erny <drew.erny@docker.com>
dperny
force-pushed
the
info-level-leader-elections
branch
from
29abbd4
to
9a54111
Compare
Codecov Report
@@ Coverage Diff @@
## master #2542 +/- ##
=========================================
+ Coverage 61.45% 61.5% +0.05%
=========================================
Files 133 133
Lines 21746 21755 +9
=========================================
+ Hits 13363 13380 +17
- Misses 6937 6941 +4
+ Partials 1446 1434 -12 |
There was a problem hiding this comment.
Spoke with @dperny offline and resolved the comments. I'm fine with the changes.
Updates (*Manager).handleLeadershipEvent to include a log message explaining where the leadership changed, using swarmkit node IDs instead of the raft node ids, at the Info level.