etcdutil, leadership: use RequestProgress in watch loop #6961

lhy1024 · 2023-08-15T15:31:53Z

What problem does this PR solve?

Issue Number: Close #6889

What is changed and how does it work?

When connecting to multiple endpoints and injecting pd-leader-io-hang, the watch chan may get stuck, so I added a ticker to send request progress to ping whether the watch chan is working normally, and rebuild it if it doesn't receive messages from the watch chan for a long time.

And add CheckWatchChan to avoid block when creating a watch channel.

Use logFields to reduce code in log output

Check List

Tests

Unit test
Manual test

run tpcc in pd-leader-io-hang case twenty times
https://tcms.pingcap.net/dashboard/executions/plan/2130511
https://tcms.pingcap.net/dashboard/executions/plan/2100564

watch leader loop can exit normally
qps did not drop to zero and did not meet 9001 error

Release note

None.

ti-chi-bot · 2023-08-15T15:31:55Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
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.

ti-chi-bot · 2023-08-15T15:31:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-08-15T15:44:10Z

Codecov Report

Merging #6961 (03e95db) into master (c1d03ee) will decrease coverage by 0.05%.
The diff coverage is 86.02%.

❗ Current head 03e95db differs from pull request most recent head a3ed8e4. Consider uploading reports for the commit a3ed8e4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6961      +/-   ##
==========================================
- Coverage   74.20%   74.16%   -0.05%     
==========================================
  Files         433      433              
  Lines       46083    46133      +50     
==========================================
+ Hits        34196    34213      +17     
- Misses       8868     8893      +25     
- Partials     3019     3027       +8

Flag	Coverage Δ
unittests	`74.16% <86.02%> (-0.05%)`	⬇️

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

Signed-off-by: lhy1024 <[email protected]>

JmPotato · 2023-08-21T05:19:22Z

Is this PR reviewable?

lhy1024 · 2023-08-21T14:11:22Z

Is this PR reviewable?

No, it still be need to more test

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2023-08-22T10:46:34Z

/check-issue-triage-complete

Signed-off-by: lhy1024 <[email protected]>

rleungx · 2023-08-24T06:24:52Z

pkg/election/leadership.go

@@ -256,15 +260,15 @@ func (ls *Leadership) Watch(serverCtx context.Context, revision int64) {
 			// When etcd is not available, the watcher.RequestProgress will block,
 			// so we check the etcd availability first.
 			if !etcdutil.IsHealthy(serverCtx, ls.client) {
-				log.Error("the connection of the leadership watcher is unhealthy",
+				log.Warn("the connection of the leadership watcher is unhealthy, retry to watch later",


I think it's better to make the log different.

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2023-08-25T02:30:55Z

Each plan run 10 network-partition and 10 pd-leader-io-hang with 03e95db
https://tcms.pingcap.net/dashboard/executions/plan/2101385
https://tcms.pingcap.net/dashboard/executions/plan/2101384

two follower exit leader loop normally
no qps drop and no 9001 error
no 9001 error in these two plans, only one execute failed

lhy1024 · 2023-08-25T02:32:21Z

test has updated, ptal @JmPotato @rleungx

pkg/election/leadership.go

rleungx · 2023-08-25T06:31:35Z

pkg/utils/etcdutil/etcdutil.go

-		opts := append(lw.opts, clientv3.WithRev(revision))
-		watchChan := watcher.Watch(watchChanCtx, lw.key, opts...)
-	WatchChanLoop:
+		watcherCtx, cancel := context.WithCancel(clientv3.WithRequireLeader(ctx))


Suggested change

watcherCtx, cancel := context.WithCancel(clientv3.WithRequireLeader(ctx))

watcherCtx, watchChanCancel = context.WithCancel(clientv3.WithRequireLeader(ctx))

#6577 (comment)

rleungx · 2023-08-25T06:37:11Z

pkg/utils/etcdutil/etcdutil.go

@@ -840,3 +894,18 @@ func (lw *LoopWatcher) SetLoadTimeout(timeout time.Duration) {
 func (lw *LoopWatcher) SetLoadBatchSize(size int64) {
 	lw.loadBatchSize = size
 }
+
+// CheckWatchChan checks whether the watch channel is blocked for a long time while creating a new watch channel.
+func CheckWatchChan(ctx context.Context, cancel context.CancelFunc, done chan struct{}) {


What is the difference between checkStream?

no difference? only avoid cycle import, how about moving checkStream out from tsoutil? Which path is suitable?

Maybe we can reuse it?

move it to grpcutil

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2023-08-25T07:45:00Z

/merge

ti-chi-bot · 2023-08-25T07:45:01Z

@lhy1024: 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 · 2023-08-25T07:45:03Z

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

Commit hash: a3ed8e4

* pd-ctl: make `TestShowKeyspaceGroupPrimary` stable (tikv#6903) close tikv#6783 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> * pd-ctl: fix keyspace group split hang when pd is in pd-mode (tikv#6907) close tikv#6906 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> * etcdutil, leadership: use RequestProgress in watch loop (tikv#6961) close tikv#6889 Signed-off-by: lhy1024 <[email protected]> --------- Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

* pd-ctl: make `TestShowKeyspaceGroupPrimary` stable (tikv#6903) close tikv#6783 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> * pd-ctl: fix keyspace group split hang when pd is in pd-mode (tikv#6907) close tikv#6906 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Signed-off-by: lhy1024 <[email protected]> * mcs/tso: add log to show the progress of group cleaning up (tikv#6971) ref tikv#6589 Add log to show the progress of group cleaning up. Signed-off-by: JmPotato <[email protected]> * etcdutil, leadership: use RequestProgress in watch loop (tikv#6961) close tikv#6889 Signed-off-by: lhy1024 <[email protected]> --------- Signed-off-by: lhy1024 <[email protected]> Signed-off-by: JmPotato <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Co-authored-by: JmPotato <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 15, 2023

ti-chi-bot bot requested review from JmPotato and rleungx August 15, 2023 15:32

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2023

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Aug 15, 2023

lhy1024 force-pushed the io-hang-watch branch from a20bdbe to 385310c Compare August 16, 2023 10:06

lhy1024 changed the title ~~[DNM]add ticker~~ etcdutil, leadership: use RequestProgress in watch loop Aug 16, 2023

etcdutil, leadership: use RequestProgress in watch loop

e2c17b2

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the io-hang-watch branch from 385310c to e2c17b2 Compare August 16, 2023 10:20

add test

f8ae8e9

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 17, 2023

use the same ctx with watcher

f856b56

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the io-hang-watch branch from 05c7477 to f856b56 Compare August 21, 2023 15:00

lhy1024 added 3 commits August 21, 2023 23:17

fix test

1402111

Signed-off-by: lhy1024 <[email protected]>

add watcher close

1e1e7de

Signed-off-by: lhy1024 <[email protected]>

add unit test

bdb3f47

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the io-hang-watch branch from 2024c3d to bdb3f47 Compare August 22, 2023 10:41

lhy1024 marked this pull request as ready for review August 22, 2023 10:45

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2023

lhy1024 removed the do-not-merge/needs-triage-completed label Aug 22, 2023

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 23, 2023

lhy1024 added 3 commits August 23, 2023 17:25

address comments

4acee01

Signed-off-by: lhy1024 <[email protected]>

adjust log

5a695c2

Signed-off-by: lhy1024 <[email protected]>

add CheckWatchChan

1d79ece

Signed-off-by: lhy1024 <[email protected]>

rleungx reviewed Aug 24, 2023

View reviewed changes

lhy1024 added 2 commits August 24, 2023 17:25

adjust log

70d4e78

Signed-off-by: lhy1024 <[email protected]>

make test stable

03e95db

Signed-off-by: lhy1024 <[email protected]>

JmPotato approved these changes Aug 25, 2023

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 Aug 25, 2023

rleungx reviewed Aug 25, 2023

View reviewed changes

lhy1024 added 2 commits August 25, 2023 15:20

address comments and revert logFields

2b8aa43

Signed-off-by: lhy1024 <[email protected]>

move CheckStream to grpcutil

fbfae85

Signed-off-by: lhy1024 <[email protected]>

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 Aug 25, 2023

lhy1024 added 2 commits August 25, 2023 15:35

Merge branch 'master' of github.com:tikv/pd into io-hang-watch

1ea51ba

fix lint

a3ed8e4

Signed-off-by: lhy1024 <[email protected]>

rleungx approved these changes Aug 25, 2023

View reviewed changes

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 25, 2023

ti-chi-bot bot merged commit 50368e5 into tikv:master Aug 25, 2023

lhy1024 deleted the io-hang-watch branch August 25, 2023 07:51

lhy1024 mentioned this pull request Aug 29, 2023

TestExitWatch is unstable #6936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdutil, leadership: use RequestProgress in watch loop #6961

etcdutil, leadership: use RequestProgress in watch loop #6961

lhy1024 commented Aug 15, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 15, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 15, 2023

codecov bot commented Aug 15, 2023 •

edited

Loading

JmPotato commented Aug 21, 2023

lhy1024 commented Aug 21, 2023

lhy1024 commented Aug 22, 2023

rleungx Aug 24, 2023

lhy1024 commented Aug 25, 2023 •

edited

Loading

lhy1024 commented Aug 25, 2023

rleungx Aug 25, 2023

lhy1024 Aug 25, 2023

rleungx Aug 25, 2023

lhy1024 Aug 25, 2023

rleungx Aug 25, 2023

lhy1024 Aug 25, 2023

lhy1024 commented Aug 25, 2023

ti-chi-bot bot commented Aug 25, 2023

ti-chi-bot bot commented Aug 25, 2023

	watcherCtx, cancel := context.WithCancel(clientv3.WithRequireLeader(ctx))
	watcherCtx, watchChanCancel = context.WithCancel(clientv3.WithRequireLeader(ctx))

etcdutil, leadership: use RequestProgress in watch loop #6961

etcdutil, leadership: use RequestProgress in watch loop #6961

Conversation

lhy1024 commented Aug 15, 2023 • 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 Aug 15, 2023 • edited Loading

ti-chi-bot bot commented Aug 15, 2023

codecov bot commented Aug 15, 2023 • edited Loading

Codecov Report

JmPotato commented Aug 21, 2023

lhy1024 commented Aug 21, 2023

lhy1024 commented Aug 22, 2023

rleungx Aug 24, 2023

Choose a reason for hiding this comment

lhy1024 commented Aug 25, 2023 • edited Loading

lhy1024 commented Aug 25, 2023

rleungx Aug 25, 2023

Choose a reason for hiding this comment

lhy1024 Aug 25, 2023

Choose a reason for hiding this comment

rleungx Aug 25, 2023

Choose a reason for hiding this comment

lhy1024 Aug 25, 2023

Choose a reason for hiding this comment

rleungx Aug 25, 2023

Choose a reason for hiding this comment

lhy1024 Aug 25, 2023

Choose a reason for hiding this comment

lhy1024 commented Aug 25, 2023

ti-chi-bot bot commented Aug 25, 2023

ti-chi-bot bot commented Aug 25, 2023

lhy1024 commented Aug 15, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

lhy1024 commented Aug 25, 2023 •

edited

Loading