sync etcd endpoints immediately after initializing the client

[vldmit]

Close #1574
Problem Statement

etcd client would only run Sync after AutoSyncInterval (30 seconds), which makes tikv client vulnerable to the failure of endpoints provided in addrs before the first sync happens. Specific failure scenario:

1. tidb is initialized with a --path=endpoint
2. tidb successfully established connection to the endpoint
3. n < 30 seconds after, endpoint fails (e.g. k8s control plane is upgrading the pod)
4. etcd client is no longer connected
5. Safe checkpoint expires and CheckVisibility in KVStore start to error out

Fix

We explicitly synchronize client endpoints with endpoints from etcd membership during client initialization phase. etcd client would continue to do periodic sync, we just force first sync to happen in the init phase.

[ti-chi-bot]

Welcome @vldmit!

It looks like this is your first PR to tikv/client-go 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to tikv/client-go. 😃

[zyguan]

@cfzjywxk PTAL

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, zyguan

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [cfzjywxk,zyguan]

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

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2025-02-14 01:27:25.878162368 +0000 UTC m=+579088.274384427: ☑️ agreed by zyguan.
• 2025-02-17 03:08:27.519627564 +0000 UTC m=+844349.915849626: ☑️ agreed by cfzjywxk.

[cfzjywxk]

/retest

[cfzjywxk]

@vldmit Thanks for the contribution, the #1574 could be closed.
The client-go dependency for tidb would be updated later to make it take effect.

[rleungx]

I'm trying to upgrade the client-go to the latest version in TiDB pingcap/tidb#59757. But some CI are failed. The log keeps printing missing address, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7104/pipeline
After I revert this commit in #1605, it succeeds, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7120/pipeline

But I haven't investigated why it failed. /cc @zyguan

[zyguan]

I'm trying to upgrade the client-go to the latest version in TiDB pingcap/tidb#59757. But some CI are failed. The log keeps printing missing address, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7104/pipeline After I revert this commit in #1605, it succeeds, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7120/pipeline

But I haven't investigated why it failed. /cc @zyguan

@rleungx It seems local.NewBackend uses config.PDAddr (instead of pdAddrs) here, which is empty due to it's generated by genConfig. This PR just exposes the issue.

[rleungx]

I'm trying to upgrade the client-go to the latest version in TiDB pingcap/tidb#59757. But some CI are failed. The log keeps printing missing address, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7104/pipeline After I revert this commit in #1605, it succeeds, see https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/7120/pipeline
But I haven't investigated why it failed. /cc @zyguan

@rleungx It seems local.NewBackend uses config.PDAddr (instead of pdAddrs) here, which is empty due to it's generated by genConfig. This PR just exposes the issue.

For some mock client, the addr is empty and sync will report an error, which fails the test.

[you06]

@rleungx It seems local.NewBackend uses config.PDAddr (instead of pdAddrs) here, which is empty due to it's generated by genConfig. This PR just exposes the issue.

What about wrapping Client.Sync and only run it when there are non-empty addresses?

[zyguan]

I've confirmed the issue with @D3Hunter, it should use pdAddrs here. I also pushed some commits to address the issue yesterday, however they've been reverted by recent force-push.

[rleungx]

I've confirmed the issue with @D3Hunter, it should use pdAddrs here. I also pushed some commits to address the issue yesterday, however they've been reverted by recent force-push.

Oh, my bad, I wrongly force pushed a commit and overwrote yours. I have changed it back.

[ti-chi-bot]

In response to a cherrypick label: cannot checkout release-8.5: error checking out release-8.5: exit status 1. output: error: pathspec 'release-8.5' did not match any file(s) known to git