Skip to content

[3.4] Backport #12671 clientv3: Replace balancer with upstream grpc solution#16844

Merged
ahrtr merged 2 commits intoetcd-io:release-3.4from
chaochn47:release-3.4-replace-balancer
Oct 31, 2023
Merged

[3.4] Backport #12671 clientv3: Replace balancer with upstream grpc solution#16844
ahrtr merged 2 commits intoetcd-io:release-3.4from
chaochn47:release-3.4-replace-balancer

Conversation

@chaochn47
Copy link
Copy Markdown
Member

@chaochn47 chaochn47 commented Oct 27, 2023 •
edited
Loading

Breakdown of #16827

Backports

Changes made on top of the original PR:

  • Unrelated go.mod and go.sum files are removed
  • Internal resolver and endpoints packages are moved into clientv3 path
  • Stripped grpc proxy changes

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@chaochn47
Copy link
Copy Markdown
Member Author

chaochn47 commented Oct 27, 2023 •
edited
Loading

Proxy test failed. https://github.com/etcd-io/etcd/actions/runs/6672706002/job/18137135993?pr=16844

I have to backport its follow up PR to make CI green.

@chaochn47 chaochn47 force-pushed the release-3.4-replace-balancer branch from eb92e28 to cbf686a Compare October 27, 2023 22:53
@chaochn47 chaochn47 marked this pull request as ready for review October 27, 2023 23:17
@chaochn47 chaochn47 requested review from ahrtr and serathius October 28, 2023 04:14
@ahrtr ahrtr mentioned this pull request Oct 28, 2023
Closed
24 tasks
ahrtr
ahrtr reviewed Oct 28, 2023
View reviewed changes
Comment thread clientv3/client.go Outdated
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Oct 28, 2023

Stripped grpc proxy changes

There are two changes with grpc proxy in #12706,

  • proxy/grpcproxy/health.go
    It should be fine to ignore this change, because the c will never be nil.
  • etcdmain/grpc_proxy.go
    Why stripped this change?

@chaochn47
Copy link
Copy Markdown
Member Author

chaochn47 commented Oct 29, 2023 •
edited
Loading

Stripped grpc proxy changes

There are two changes with grpc proxy in #12706,

* proxy/grpcproxy/health.go
  It should be fine to ignore this change, because the `c` will never be nil.

* etcdmain/grpc_proxy.go
  Why stripped this change?

etcdmain/grpc_proxy.go change is unrelated to unblock the CVE change. The client was created for proxy health check in #12114.

I assume we want to reduce the moving parts for the sake of bumping up gRPC versions, so I stripped this change.

Does that make sense to you? @ahrtr

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Oct 29, 2023

The client was created for proxy health check in #12114.

OK, This PR (a feature) hasn't been backported to 3.4 at all, so no need to backport the grpc-proxy related change in #12706 to 3.4 either.

@chaochn47
Copy link
Copy Markdown
Member Author

chaochn47 commented Oct 29, 2023 •
edited
Loading

The client was created for proxy health check in #12114.

OK, This PR (a feature) hasn't been backported to 3.4 at all, so no need to backport the grpc-proxy related change in #12706 to 3.4 either.

Yeah, I could have documented it clearer.

@ahrtr Other than this contentious change, is there anything else I should follow up for this PR?

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Oct 29, 2023

@ahrtr Other than this contentious change, is there anything else I should follow up for this PR?

Overall looks good to me. The only minor comment is #16844 (comment)

Thanks.

@chaochn47 chaochn47 force-pushed the release-3.4-replace-balancer branch from cbf686a to 69a2c6f Compare October 30, 2023 14:58
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Oct 30, 2023

Just merged #16849, please rebase this PR.

@chaochn47
[3.4] Backport etcd-io#12671 clientv3: Replace balancer with upstream…
83da5ff 
… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>
@chaochn47
[3.4] backport etcd-io#12706 clientv3: PS: Replace balancer with upst…
3a0dd2d 
…ream grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>
@chaochn47 chaochn47 force-pushed the release-3.4-replace-balancer branch from 69a2c6f to 3a0dd2d Compare October 30, 2023 16:27
ahrtr
ahrtr approved these changes Oct 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @chaochn47

cc @jmhbnz @serathius @wenjiaswe @tjungblu

jmhbnz
jmhbnz approved these changes Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Backports LGTM - Thanks @chaochn47

serathius
serathius reviewed Oct 31, 2023
View reviewed changes
Comment thread clientv3/client.go
@ahrtr ahrtr merged commit 0fb0045 into etcd-io:release-3.4 Oct 31, 2023
@ahrtr ahrtr mentioned this pull request Oct 31, 2023
Closed
@chaochn47 chaochn47 deleted the release-3.4-replace-balancer branch October 31, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahrtr ahrtr ahrtr approved these changes

@serathius serathius serathius approved these changes

@jmhbnz jmhbnz jmhbnz approved these changes

Assignees

No one assigned

Labels

area/clientv3

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chaochn47 @ahrtr @serathius @jmhbnz @wenjiaswe