etcdutil, leadership: make more high availability

[lhy1024]

What problem does this PR solve?

Issue Number: Close #6554 #6442

What is changed and how does it work?

leadership

1. make leadership exit watch loop as soon as possible when etcd is unavailable

2. add some other logic similar to loopwatcher, such as revision++, wresp.Err() != nil and WithRequireLeader

etcd client

Because the etcd client's keepalive is based on the grpc connection, it can only determine if the network is working properly.

And because etcd client's balancer only supports round-robin policy, it cannot be set to connect only to the leader.

So when the etcd client's original connection is not working for various reasons, such as network isolation, leader-io-hang, or tcp half-open connection, we need to switch the connection to an available node in a more timely manner.

It is easy to think that we can create additional goroutines to check the health of the connections, so for each etcd client, we will create two goroutines, one goroutine to check the health of the node and another goroutine to synchronize the members.

And grpc substream retry policy is exponential rather than linear, which also needs to change it.

Check List

Tests

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

1. pd-leader-io-hang

   
   only update pd etcd client, there are two 8027 error
   
   update pd and tidb etcd client at the same, there is no any 8027 error

2. pd-leader-to-all-network-partition

   
   only update pd etcd client, there is a warning with 21.7% drop.
   
   update pd and tidb etcd client at the same, there are some warnings, but all warnings are just a little over 20% and below 25%

3. Manual stop and random restart api server, observe when tso server watch primary
   etcdutil, leadership: make more high availability #6577 (comment)
   
   

4. Test 30 times

   Step: kubectl delete pod serverless-cluster-pd-0 serverless-cluster-pd-1 serverless-cluster-pd-2 -n tidb-serverless --force --grace-period=0
   note: replace tso server with etcdutil, leadership: make more high availability #6577 use pd-tso-bench tools: add keepalive for pd-tso-bench #6699
   Result: can recovery in 2 min

Release note

None.

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• binshi-bing
• 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]

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

[rleungx]

Any progress on this PR?

[lhy1024]

Any progress on this PR?

In addition to trying to test with iptable and tcpkill, I tried to start the server locally and do a kill test.

Test step:

1. ./pd-server services api ./pd-server services tso --listen-addr="http://127.0.0.1:3379/" --backendendpoints="http://127.0.0.1:2379/" ./pd-server services tso --listen-addr="http://127.0.0.1:3380/" --backend-endpoints="http://127.0.0.1:2379/"

2. kill or kill -9 with api server

3. wait random time to restart api server

4. observe when tso server finish election or watch leader.

5. I test with pd-cse and this pr. For one api server, this pr is not better than pd-cse with keep-alive

client, err := clientv3.New(clientv3.Config{
        Endpoints:            []string{acURL.String()},
        DialTimeout:          defaultEtcdClientTimeout,
        TLS:                  tlsConfig,
        LogConfig:            &lgc,
        DialKeepAliveTime:    defaultDialKeepAliveTime,
        DialKeepAliveTimeout: defaultDialKeepAliveTimeout,
    })

2. I try use session to keep alive, but when I kill or kill -9 api server, there is no any session log, which means the code doesn't touch session.Done.

func (ls *Leadership) Watch(serverCtx context.Context, revision int64) {
    if ls == nil {
        return
    }
    session, err := concurrency.NewSession(
        ls.client, concurrency.WithTTL(180),
    ) // 3 minutes
    if err != nil {
        return
    }
   ...
   for {
   ....
       case <-session.Done():
            // session closed, return
            log.Info("leadership session is done", zap.String("leader-key", ls.leaderKey),
                zap.String("purpose", ls.purpose))
            return
    ....
   }
}

[lhy1024]

Similarly, I test other ways

3. detect unhealthy etcd cluster and exit watch loop
4. pick some etcd pr
5. use the new client healthy strategy, it is valid for the current test

When creating the client, create two new goroutines, one goroutine synchronizes the endpoint and stores it in a sync.map and periodically checks if it is expired, the other goroutine establishes n connections to check the health of the nodes, the advantage of this is to avoid the failure of other nodes to affect the overall latency, but also to avoid the delay when synchronizing This has the advantage of avoiding the overall latency caused by the failure of other nodes and the latency caused by the lag in synchronization.
If there is no node available, try to reset endpoints to refresh the connection

[codecov]

Codecov Report

Merging #6577 (e1e7e08) into master (40eaa35) will increase coverage by 0.13%.
The diff coverage is 89.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6577      +/-   ##
==========================================
+ Coverage   74.15%   74.29%   +0.13%     
==========================================
  Files         413      414       +1     
  Lines       43303    43445     +142     
==========================================
+ Hits        32112    32276     +164     
+ Misses       8324     8311      -13     
+ Partials     2867     2858       -9     

Flag	Coverage Δ
unittests	74.29% <89.73%> (+0.13%)	⬆️

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

[rleungx]

Is it ready to be reviewed?

[binshi-bing]

left few comments. no complete yet.

[binshi-bing]

How about adding log when we exits the get leader loop?

Suggested change

	defer watcher.Close()
	defer watcher.Close()
	defer log.Info("exit ...")

[binshi-bing]

Is this etcd standard heartbeat or health check API?

[lhy1024]

yes, etcd also use this key to check health https://github.com/etcd-io/etcd/blob/85b640cee793e25f3837c47200089d14a8392dc7/etcdctl/ctlv3/command/ep_command.go#L121

[binshi-bing]

left a few comments. The rest looks good to me

[binshi-bing]

Agree with rleungx. Probably just use something like //nolint to avoid pointer.
But I have no strong preference, either way looks good to me.

[binshi-bing]

Regarding #6554 "TSO primary election took 14 minutes after PD(API) Pods being deleted at the same time and PD(API) leader being re-elected", I have two questions:

1. If watch can't exit when etcd isn't available is the main reason, it only explains the seconday, which watches the leader here, can't be re-elected as the new primary, and it can't explain why the old primary can't be re-elected as the new primary. What happened on the old primary? Did the old primary also get stuck in some watch loop?
2. It seems to me what we're thinking is, during the long duration between PD(API) Pods being deleted at the same time and PD(API) leader being re-elected, the watch here won't exit, and even after PD (API) leader is back, the watch here somehow still can't catch the lease lost/expired event. Is my understanding correct? If yes, how long is the unsafe duration of PD(API) leader being offline (once this unsafe duration is exceeded, the watcher here still can't catch the lease lost/expired event)? And the setting value of "unhealthyTimeout" needs to be less than this unsafe duration.

[binshi-bing]

I do think the current PR can help to mitigate the situation even without perfect solution or answers to 1 and 2, because with this pr, it seems that the secondary can be elected as the new primary which addressed the question 1 above and the question 2 is a parameter tuning problem.

[lhy1024]

It's not sure because the watch loop can't exit that it takes fifteen minutes to re-elect, there could be some reasons, stuck on grpc retry, stuck on tcp retry, or watch stuck on etcd watch.

[binshi-bing]

'continue' goes to the beginning of the for loop, correct? Do we need to reset lastHealthyTime here?

[lhy1024]

Yes, we need to recreate watch chan with new revision

[JmPotato]

LGTM. Please fix make check first.

[rleungx]

Suggested change

	watchChanCtx, cancel := context.WithCancel(clientv3.WithRequireLeader(serverCtx))
	watchChanCancel = cancel
	watchChanCtx, watchChanCancel := context.WithCancel(clientv3.WithRequireLeader(serverCtx))

[lhy1024]

In previously written, it seems to work without adding the nolint comment.

[rleungx]

Is it necessary now?

[JmPotato]

/merge

[ti-chi-bot]

@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]

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

Commit hash: e1e7e08