Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: upgrade etcd to v3.5.15 #8441

Merged
merged 8 commits into from
Aug 15, 2024
Merged

*: upgrade etcd to v3.5.15 #8441

merged 8 commits into from
Aug 15, 2024

Conversation

lhy1024
Copy link
Contributor

@lhy1024 lhy1024 commented Jul 25, 2024

What problem does this PR solve?

Issue Number: Close #3877 Ref #5134

What is changed and how does it work?

Check List

Tests

  • Unit test

Release note

*: upgrade etcd to v3.5.15 

Copy link
Contributor

ti-chi-bot bot commented Jul 25, 2024

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

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2024
@lhy1024 lhy1024 changed the title update etcd to 3.5.15 *: upgrade etcd to v3.5.15 Jul 30, 2024
@ti-chi-bot 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 Jul 30, 2024
@lhy1024 lhy1024 marked this pull request as ready for review August 7, 2024 10:12
@ti-chi-bot 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 7, 2024
// If Linearizable is set to false, the member list will be returned with server's local data.
// If Linearizable is set to true, it is served with linearizable guarantee. If the server is disconnected from quorum, `MemberList` call will fail.
c := clientv3.RetryClusterClient(client)
resp, err := c.MemberList(newCtx, &etcdserverpb.MemberListRequest{Linearizable: false})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set it to false to keep it consistent with previous behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is RetryClusterClient needed here and not elsewhere?

Copy link
Contributor Author

@lhy1024 lhy1024 Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to pass &etcdserverpb.MemberListRequest{Linearizable: false}

If we use resp, err :=client.MemberList(ctx), it will always be true

https://github.com/etcd-io/etcd/blame/v3.5.15/client/v3/cluster.go#L125-L132

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.59%. Comparing base (1c1cd99) to head (35ae79d).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8441      +/-   ##
==========================================
+ Coverage   77.40%   77.59%   +0.19%     
==========================================
  Files         473      474       +1     
  Lines       61934    61866      -68     
==========================================
+ Hits        47941    48006      +65     
+ Misses      10409    10326      -83     
+ Partials     3584     3534      -50     
Flag Coverage Δ
unittests 77.59% <100.00%> (+0.19%) ⬆️

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

Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
tests/integrations/go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 8, 2024
@@ -88,7 +89,7 @@ func CheckClusterID(localClusterID types.ID, um types.URLsMap, tlsConfig *tls.Co
trp := &http.Transport{
TLSClientConfig: tlsConfig,
}
remoteCluster, gerr := etcdserver.GetClusterFromRemotePeers(nil, []string{u}, trp, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the default value of the new function GetClusterFromRemotePeers true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is introduced by #8017 and etcd-io/etcd#17330, it means Enable 3.4 to be compatible with next version 3.5, to allow 3.4 server to join 3.5 cluster and start on 3.5 schema.

@lhy1024
Copy link
Contributor Author

lhy1024 commented Aug 8, 2024

/hold
After discussion with @wuhuizuo, upgrade of etcd will be released in v8.4 rather than v8.3

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2024
go.mod Outdated Show resolved Hide resolved
@@ -739,6 +739,7 @@ func (c *Config) GenEmbedEtcdConfig() (*embed.Config, error) {
cfg.PeerTLSInfo.TrustedCAFile = c.Security.CAPath
cfg.PeerTLSInfo.CertFile = c.Security.CertPath
cfg.PeerTLSInfo.KeyFile = c.Security.KeyPath
//nolint:staticcheck
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after etcd-io/etcd#18015, AllowedCN is Deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be replaced by AllowedCNs in a later pr to support multi cn

tests/integrations/go.mod Outdated Show resolved Hide resolved
pkg/utils/etcdutil/etcdutil.go Outdated Show resolved Hide resolved
cancel()
if err != nil {
return listResp, errs.ErrEtcdMemberList.Wrap(err).GenWithStackByCause()
return (*clientv3.MemberListResponse)(resp), errs.ErrEtcdMemberList.Wrap(err).GenWithStackByCause()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need (*clientv3.MemberListResponse)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the return value of ListEtcdMembers is expected to be (*clientv3.MemberListResponse)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the current type without (*clientv3.MemberListResponse)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I notice you are using retryClusterClient. Is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current type is etcdserverpb.MemberListResponse

Copy link
Contributor Author

@lhy1024 lhy1024 Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to pass &etcdserverpb.MemberListRequest{Linearizable: false}

If we use resp, err :=client.MemberList(ctx), it will always be true

https://github.com/etcd-io/etcd/blame/v3.5.15/client/v3/cluster.go#L125-L132

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.MemberList call c.remote.MemberList. c.remote is built by RetryClusterClient
https://github.com/etcd-io/etcd/blob/v3.5.15/client/v3/cluster.go#L60-L67

Signed-off-by: lhy1024 <admin@liudos.us>
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 9, 2024
Copy link
Contributor

ti-chi-bot bot commented Aug 9, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-08-08 03:07:47.977539285 +0000 UTC m=+494197.844638374: ☑️ agreed by nolouch.
  • 2024-08-09 06:11:15.794041927 +0000 UTC m=+591605.661141015: ☑️ agreed by rleungx.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024
@lhy1024
Copy link
Contributor Author

lhy1024 commented Aug 14, 2024

/unhold
https://github.com/tikv/pd/tree/release-8.3 has been created cc @wuhuizuo

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2024
Signed-off-by: lhy1024 <admin@liudos.us>
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2024
Signed-off-by: lhy1024 <admin@liudos.us>
@lhy1024
Copy link
Contributor Author

lhy1024 commented Aug 15, 2024

/test pull-integration-realcluster-test

Copy link
Contributor

ti-chi-bot bot commented Aug 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: niubell, nolouch, rleungx

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:

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 ti-chi-bot bot added the approved label Aug 15, 2024
@ti-chi-bot ti-chi-bot bot merged commit cc2900c into tikv:master Aug 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update etcd to v3.5.x
5 participants