etcdutil: consider the latency while patrolling the healthy endpoints #7737

JmPotato · 2024-01-19T07:02:09Z

What problem does this PR solve?

Issue Number: ref #7730, #7499.

What is changed and how does it work?

Consider the latency while patrolling the healthy endpoints to reduce the effect of slow nodes.
Now, there are the following strategies to select and remove unhealthy endpoints:

- Choose only the healthy endpoint within the lowest acceptable latency range.
- The evicted endpoint can only rejoin if it is selected again for three consecutive times.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Inject the etcd leader IO latency like:

The duration of QPS being affected is reduced, and it may not necessarily hit rock bottom completely. It will only be impacted by the switch of the etcd leader.

Before:

Etcd leader changing will be finished in just one term.

Before:

Endpoint updating will be stabilized and only occur during IO hang injection and recovery points.

Before:

Release note

Enhance PD availability in the event of IO hang causing node unavailability.

ti-chi-bot · 2024-01-19T07:02:12Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov · 2024-01-19T08:45:17Z

Codecov Report

Merging #7737 (ade43b6) into master (e8da033) will increase coverage by 0.02%.
The diff coverage is 95.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7737      +/-   ##
==========================================
+ Coverage   73.54%   73.57%   +0.02%     
==========================================
  Files         430      430              
  Lines       47645    47740      +95     
==========================================
+ Hits        35042    35126      +84     
- Misses       9605     9606       +1     
- Partials     2998     3008      +10

Flag	Coverage Δ
unittests	`73.57% <95.27%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

pkg/utils/etcdutil/health_checker.go

HuSharp

Do we need to add some metrics to indicate healthy status?

JmPotato · 2024-01-22T12:26:45Z

Do we need to add some metrics to indicate healthy status?

Added. PTAL.

HuSharp · 2024-01-23T02:07:54Z

Do we need to add some metrics to indicate healthy status?

Added. PTAL.

can you paste the metric pic in the pr description :)

metrics/grafana/pd.json

HuSharp

rest LGTM!

pkg/utils/etcdutil/health_checker.go

JmPotato · 2024-01-29T06:36:21Z

pkg/utils/etcdutil/health_checker.go

+	var (
+		lastEps   = checker.client.Endpoints()
+		pickedEps = checker.pickEps(probeCh)
+	)
+	if len(pickedEps) > 0 {
+		checker.updateEvictedEps(lastEps, pickedEps)
+		pickedEps = checker.filterEps(pickedEps)
+	}
+	return lastEps, pickedEps, !typeutil.AreStringSlicesEquivalent(lastEps, pickedEps)


Key modifications. You can specifically review this section.

rleungx · 2024-01-29T09:26:24Z

pkg/utils/etcdutil/health_checker.go

+	pickedEps := make([]string, 0, len(eps))
+	for _, ep := range eps {
+		if count, ok := checker.evictedEps.Load(ep); ok {
+			if count.(int) < pickedCountThreshold {


What if all the eps are less than pickedCountThreshold?

If the final picked endpoint is empty, we will not use it to replace the last:

https://github.com/JmPotato/pd/blob/17097f35dc235c94f63ff395956e7e5bad1da80b/pkg/utils/etcdutil/health_checker.go#L118-L132

Then, we might need to reset all, right?

Do you mean we should reset all endpoints when there are no available endpoints to improve availability, considering the situation can't get any worse?

pkg/utils/etcdutil/health_checker.go

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato · 2024-01-30T08:18:20Z

/merge

ti-chi-bot · 2024-01-30T08:18:22Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2024-01-30T08:18:23Z

This pull request has been accepted and is ready to merge.

Commit hash: ade43b6

close #7251 Cherry-pick the etcd client health checker improvements from #7725. #7727, #7743, #7737 and #7779. Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato requested review from lhy1024, rleungx and HuSharp January 19, 2024 07:02

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2024

JmPotato force-pushed the cal branch 3 times, most recently from 06f8682 to 862d2af Compare January 22, 2024 03:49

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2024

JmPotato force-pushed the cal branch 2 times, most recently from 8ff4354 to 41f7bd7 Compare January 22, 2024 03:51

lhy1024 reviewed Jan 22, 2024

View reviewed changes

pkg/utils/etcdutil/health_checker.go Outdated Show resolved Hide resolved

pkg/utils/etcdutil/health_checker.go Show resolved Hide resolved

HuSharp reviewed Jan 22, 2024

View reviewed changes

pkg/utils/etcdutil/health_checker.go Outdated Show resolved Hide resolved

HuSharp reviewed Jan 22, 2024

View reviewed changes

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 22, 2024

JmPotato requested review from lhy1024 and HuSharp January 22, 2024 12:27

JmPotato force-pushed the cal branch from fc7d8b8 to 5ec2127 Compare January 22, 2024 13:35

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 23, 2024

HuSharp reviewed Jan 23, 2024

View reviewed changes

metrics/grafana/pd.json Outdated Show resolved Hide resolved

JmPotato force-pushed the cal branch from d1ea8d2 to e8e104e Compare January 23, 2024 04:36

JmPotato requested a review from HuSharp January 23, 2024 05:42

HuSharp approved these changes Jan 23, 2024

View reviewed changes

pkg/utils/etcdutil/health_checker.go Show resolved Hide resolved

JmPotato commented Jan 29, 2024

View reviewed changes

JmPotato force-pushed the cal branch from 408eb88 to 17097f3 Compare January 29, 2024 07:22

rleungx reviewed Jan 29, 2024

View reviewed changes

pkg/utils/etcdutil/health_checker.go Show resolved Hide resolved

JmPotato added 8 commits January 30, 2024 12:09

Consider the latency while patrolling the healthy endpoints

01cffe3

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Add more comments

cdabf67

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Refine the code and metrics

5266eaf

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Refine the metrics

f9271cd

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Add source to the logs

7915b88

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Optimize the performance of updateEvictedEps

5a8dc54

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Refine TestRandomKillEtcd

335d272

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Fix the channle consumed bug

ade43b6

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato force-pushed the cal branch from 17097f3 to ade43b6 Compare January 30, 2024 04:09

rleungx approved these changes Jan 30, 2024

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 30, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 30, 2024

ti-chi-bot bot merged commit 1c54865 into tikv:master Jan 30, 2024
25 of 26 checks passed

JmPotato deleted the cal branch January 30, 2024 08:22

This was referenced Jan 30, 2024

pd can not select new leader which lead qps drop to zero when inject pdleader io delay 500ms last for 5mins #7251

Closed

qps drop 70% during pd leader io hang with error code 8027 #6291

Closed

This was referenced Feb 1, 2024

server/*: add trend to check etcd healthy #7682

Closed

lease: check etcd leader healthy by KeepAliveOnce #7670

Closed

JmPotato mentioned this pull request Feb 2, 2024

*: cherry-pick the etcd client health checker improvements #7793

Merged

This was referenced Feb 6, 2024

Enhance the detection mechanism for the unhealthy etcd node #7730

Closed

Reduce the impact of etcd leader on the availability of PD leader #7499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdutil: consider the latency while patrolling the healthy endpoints #7737

etcdutil: consider the latency while patrolling the healthy endpoints #7737

JmPotato commented Jan 19, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

HuSharp left a comment

JmPotato commented Jan 22, 2024

HuSharp commented Jan 23, 2024

HuSharp left a comment

JmPotato Jan 29, 2024

rleungx Jan 29, 2024 •

edited

Loading

JmPotato Jan 29, 2024

rleungx Jan 29, 2024

JmPotato Jan 30, 2024

JmPotato commented Jan 30, 2024

ti-chi-bot bot commented Jan 30, 2024

ti-chi-bot bot commented Jan 30, 2024

etcdutil: consider the latency while patrolling the healthy endpoints #7737

etcdutil: consider the latency while patrolling the healthy endpoints #7737

Conversation

JmPotato commented Jan 19, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jan 19, 2024 • edited Loading

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

HuSharp left a comment

Choose a reason for hiding this comment

JmPotato commented Jan 22, 2024

HuSharp commented Jan 23, 2024

HuSharp left a comment

Choose a reason for hiding this comment

JmPotato Jan 29, 2024

Choose a reason for hiding this comment

rleungx Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

JmPotato Jan 29, 2024

Choose a reason for hiding this comment

rleungx Jan 29, 2024

Choose a reason for hiding this comment

JmPotato Jan 30, 2024

Choose a reason for hiding this comment

JmPotato commented Jan 30, 2024

ti-chi-bot bot commented Jan 30, 2024

ti-chi-bot bot commented Jan 30, 2024

JmPotato commented Jan 19, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

rleungx Jan 29, 2024 •

edited

Loading