Skip leadership check if the etcd instance is active processing heartbeats #18428

ahrtr · 2024-08-09T10:27:15Z

Will post benchmark result later.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

ahrtr · 2024-08-09T10:42:28Z

Summary

This PR resolves the performance issue #18069

Main branch

Summary:
  Total:	25.8324 secs.
  Slowest:	0.1114 secs.
  Fastest:	0.0001 secs.
  Average:	0.0024 secs.
  Stddev:	0.0030 secs.
  Requests/sec:	38711.1170

Response time histogram:
  0.0001 [1]	|
  0.0112 [986525]	|∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0223 [10104]	|
  0.0335 [1905]	|
  0.0446 [712]	|
  0.0557 [247]	|
  0.0669 [243]	|
  0.0780 [234]	|
  0.0891 [21]	|
  0.1003 [5]	|
  0.1114 [3]	|

Latency distribution:
  10% in 0.0008 secs.
  25% in 0.0012 secs.
  50% in 0.0017 secs.
  75% in 0.0027 secs.
  90% in 0.0043 secs.
  95% in 0.0060 secs.
  99% in 0.0132 secs.
  99.9% in 0.0406 secs.

This PR

Summary:
  Total:	14.8739 secs.
  Slowest:	0.0824 secs.
  Fastest:	0.0000 secs.
  Average:	0.0012 secs.
  Stddev:	0.0014 secs.
  Requests/sec:	67231.7847

Response time histogram:
  0.0000 [1]	|
  0.0083 [996078]	|∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0165 [2981]	|
  0.0247 [548]	|
  0.0330 [185]	|
  0.0412 [99]	|
  0.0494 [26]	|
  0.0577 [41]	|
  0.0659 [39]	|
  0.0741 [0]	|
  0.0824 [2]	|

Latency distribution:
  10% in 0.0004 secs.
  25% in 0.0006 secs.
  50% in 0.0009 secs.
  75% in 0.0014 secs.
  90% in 0.0021 secs.
  95% in 0.0028 secs.
  99% in 0.0063 secs.
  99.9% in 0.0160 secs.

Revert the #16822 (checked out `2f03bbc5dd34b4250c9baece58754e8180786bd4`)

Summary:
  Total:	14.6155 secs.
  Slowest:	0.0637 secs.
  Fastest:	0.0000 secs.
  Average:	0.0011 secs.
  Stddev:	0.0012 secs.
  Requests/sec:	68420.5991

Response time histogram:
  0.0000 [1]	|
  0.0064 [993920]	|∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0128 [4897]	|
  0.0191 [705]	|
  0.0255 [216]	|
  0.0319 [158]	|
  0.0383 [55]	|
  0.0446 [20]	|
  0.0510 [7]	|
  0.0574 [16]	|
  0.0637 [5]	|

Latency distribution:
  10% in 0.0004 secs.
  25% in 0.0006 secs.
  50% in 0.0009 secs.
  75% in 0.0013 secs.
  90% in 0.0019 secs.
  95% in 0.0025 secs.
  99% in 0.0054 secs.
  99.9% in 0.0136 secs.

ahrtr · 2024-08-09T10:45:29Z

cc @fuweid @jmhbnz @serathius @qixiaoyang0 @ranandfigma @caesarxuchao

This PR resolves the performance issue with a minimum change.

codecov-commenter · 2024-08-09T10:45:44Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.85%. Comparing base (df4e472) to head (3aa072a).
Report is 207 commits behind head on main.

❗ Current head 3aa072a differs from pull request most recent head b8b0cf8

Please upload reports for the commit b8b0cf8 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/raft.go	`87.16% <100.00%> (+0.42%)`	⬆️
server/etcdserver/server.go	`81.71% <100.00%> (+0.04%)`	⬆️

... and 23 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18428      +/-   ##
==========================================
+ Coverage   68.83%   68.85%   +0.02%     
==========================================
  Files         420      420              
  Lines       35475    35490      +15     
==========================================
+ Hits        24420    24438      +18     
+ Misses       9631     9621      -10     
- Partials     1424     1431       +7

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df4e472...b8b0cf8. Read the comment docs.

…beat Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ah8ad3

Thanks for implementation looks clean. LGTM
I have one question: Imagine a scenario where leadership is lost due to two candidates fighting over with similar terms and equal votes.
Does that mean after merging this the new vote will be start with more delay? And possibly the cluster will be down?

I know this scenario is hard to produce i'm asking to find out the behavior.

ahrtr · 2024-08-09T18:21:16Z

I have one question: Imagine a scenario where leadership is lost due to two candidates fighting over with similar terms and equal votes.
Does that mean after merging this the new vote will be start with more delay? And possibly the cluster will be down?

This PR has nothing to do with any raft's protocol. It just tries to avoid unnecessary ensureLeadership check, so as to avoid unnecessary performance reduce.

caesarxuchao

LGTM. Thanks for the quick fix!

server/etcdserver/server.go

jmhbnz

LGTM - Nice work @ahrtr

ivanvc

LGTM. Thanks, Benjamin.

k8s-ci-robot · 2024-08-14T00:18:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, caesarxuchao, ivanvc, jmhbnz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,jmhbnz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahrtr · 2024-08-14T14:58:33Z

Thanks all for the review.

serathius · 2024-08-15T11:39:22Z

server/etcdserver/server.go

@@ -904,10 +904,26 @@ func (s *EtcdServer) revokeExpiredLeases(leases []*lease.Lease) {
 	})
 }

+// isActive checks if the etcd instance is still actively processing the
+// heartbeat message (ticks). It returns false if no heartbeat has been
+// received within 3 * tickMs.


Why 3 election ticks? I think the correct value is somewhere between TickMs (default 100ms) and ElectionMs (default 1s), meaning the 3 makes sense for default config, but I would argue it can be problematic for cases where ElectionMS < 3* TicksMs

Why 3 election ticks?

Read #18428 (comment).

I would argue it can be problematic for cases where ElectionMS < 3* TicksMs

I am not worry about this.

checking activity should be only related to tickMs.

also if ElectionMS < 3* TicksMs, it means it's a very inappropriate election value.

also if ElectionMS < 3* TicksMs, it means it's a very inappropriate election value.

Then let's codify that is is inappropriate. Either working or failing validation.

serathius · 2024-08-15T11:41:58Z

server/etcdserver/server.go

 // ensureLeadership checks whether current member is still the leader.
 func (s *EtcdServer) ensureLeadership() bool {
 	lg := s.Logger()

+	if s.isActive() {


This is incorrect assumption for 5 node clusters, getting a tick from 1 member is not effective way to confirm no other leader was elected. Imagine 3/2 network split with leader on side of 2. With your change the leader can continue think it's active by receiving ticks from 1 member, where on the other side of network 3 members have already elected new leader.

It has nothing to do with how many nodes the cluster has. The intention is to guard the case the node being stuck by itself, e.g stall write.

Imagine 3/2 network split with leader on side of 2. With your change the leader can continue think it's active by receiving ticks from 1 member

It's active, but it won't be a leader any more. It will step down automatically in such case due to losing quorum.

Note that the raft protocol handle the network partition case perfectly.

It's active, but it won't be a leader any more.

We are talking about 1 second before network partition, until heath probes fail for ElectionMs, the old leader will still think it's a leader before they resign.

serathius · 2024-08-15T11:43:40Z

The proposed solution is not compatible with etcd configuration options and doesn't work for 5 node clusters. Please consider reverting and redesign.

ahrtr · 2024-08-15T15:59:31Z

The proposed solution is not compatible with etcd configuration options and doesn't work for 5 node clusters. Please consider reverting and redesign.

Don't agree. Pls see my comments above. The only minor possible improvement is to guard the threshold < electionTimeout, but I am not worry about it as mentioned in #18428 (comment)

k8s-ci-robot added the approved label Aug 9, 2024

ahrtr added area/performance and removed approved labels Aug 9, 2024

k8s-ci-robot added the size/M label Aug 9, 2024

ahrtr mentioned this pull request Aug 9, 2024

Performance Issue: CPU usage of the leader node increased by 20% #18069

Closed

4 tasks

Skip leadership check if the etcd instance is active processing heart…

b8b0cf8

…beat Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ahrtr force-pushed the improve_leaseRenew_20240809 branch from 1da8809 to b8b0cf8 Compare August 9, 2024 16:02

k8s-ci-robot added the approved label Aug 9, 2024

ahrtr added backport/v3.4 backport/v3.5 labels Aug 9, 2024

ah8ad3 reviewed Aug 9, 2024

View reviewed changes

caesarxuchao approved these changes Aug 9, 2024

View reviewed changes

jmhbnz reviewed Aug 10, 2024

View reviewed changes

server/etcdserver/server.go Show resolved Hide resolved

jmhbnz approved these changes Aug 10, 2024

View reviewed changes

ivanvc approved these changes Aug 14, 2024

View reviewed changes

ahrtr merged commit a8b30b1 into etcd-io:main Aug 14, 2024
47 checks passed

ahrtr deleted the improve_leaseRenew_20240809 branch August 14, 2024 20:36

serathius reviewed Aug 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip leadership check if the etcd instance is active processing heartbeats #18428

Skip leadership check if the etcd instance is active processing heartbeats #18428

ahrtr commented Aug 9, 2024

ahrtr commented Aug 9, 2024

ahrtr commented Aug 9, 2024

codecov-commenter commented Aug 9, 2024 •

edited

Loading

ah8ad3 left a comment •

edited

Loading

ahrtr commented Aug 9, 2024

caesarxuchao left a comment

jmhbnz left a comment

ivanvc left a comment

k8s-ci-robot commented Aug 14, 2024

ahrtr commented Aug 14, 2024

serathius Aug 15, 2024

ahrtr Aug 15, 2024

serathius Aug 16, 2024

serathius Aug 15, 2024

ahrtr Aug 15, 2024

ahrtr Aug 15, 2024

serathius Aug 16, 2024

serathius commented Aug 15, 2024

ahrtr commented Aug 15, 2024 •

edited

Loading

Skip leadership check if the etcd instance is active processing heartbeats #18428

Skip leadership check if the etcd instance is active processing heartbeats #18428

Conversation

ahrtr commented Aug 9, 2024

ahrtr commented Aug 9, 2024

Summary

Main branch

This PR

Revert the #16822 (checked out 2f03bbc5dd34b4250c9baece58754e8180786bd4)

ahrtr commented Aug 9, 2024

codecov-commenter commented Aug 9, 2024 • edited Loading

Codecov Report

ah8ad3 left a comment • edited Loading

Choose a reason for hiding this comment

ahrtr commented Aug 9, 2024

caesarxuchao left a comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

ivanvc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 14, 2024

ahrtr commented Aug 14, 2024

serathius Aug 15, 2024

Choose a reason for hiding this comment

ahrtr Aug 15, 2024

Choose a reason for hiding this comment

serathius Aug 16, 2024

Choose a reason for hiding this comment

serathius Aug 15, 2024

Choose a reason for hiding this comment

ahrtr Aug 15, 2024

Choose a reason for hiding this comment

ahrtr Aug 15, 2024

Choose a reason for hiding this comment

serathius Aug 16, 2024

Choose a reason for hiding this comment

serathius commented Aug 15, 2024

ahrtr commented Aug 15, 2024 • edited Loading

Revert the #16822 (checked out `2f03bbc5dd34b4250c9baece58754e8180786bd4`)

codecov-commenter commented Aug 9, 2024 •

edited

Loading

ah8ad3 left a comment •

edited

Loading

ahrtr commented Aug 15, 2024 •

edited

Loading