Upgrade to `google.golang.org/grpc@1.35.0` #12636

dhermes · 2021-01-20T20:57:11Z

This is VERY similar to #12609, but I tried to minimize the diffs by using import aliases and also resolved some breakages in interfaces from google.golang.org/grpc/balancer.

This does not try to fully migrate to the new resolver pattern as in #12614, but just provides a bridge to the latest version of google.golang.org.

Getting past 1.29.1 (there were several breaking changes in 1.30.0, including the removal of google.golang.org/grpc/naming) is critical to be able to use etcd in codebases that also use k8s>=1.17 with go mod.

This is intended so that `go mod tidy` can be used to re-generate imports (to prepare for an update of `google.golang.org/grpc`.

The official recommendation is to move to `google.golang.org/grpc/resolver` but this is not exactly a drop-in replacement.

This is because the `grpc/naming` breaking change happened in `v1.30.0` (i.e. we need to get past `v1.29.1`). See: https://github.com/grpc/grpc-go/releases/tag/v1.30.0

The two implementations updated satisfy `(google.golang.org/grpc/balancer).Picker`, see the history of this interface: - https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Picker - https://pkg.go.dev/google.golang.org/grpc@v1.30.0/balancer#Picker - https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Picker

The `baseBalancer` struct satisfies `(google.golang.org/grpc/balancer).Balancer`, see the history of this interface: - https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Balancer - https://pkg.go.dev/google.golang.org/grpc@v1.30.0/balancer#Balancer - https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Balancer

See etcd-io#12564 (comment) for how I determined which order to visit packages in.

dhermes · 2021-01-20T21:10:55Z

client/v3/balancer/balancer.go

+// UpdateClientConnState implements "grpc/balancer.Balancer" interface.
+func (bb *baseBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error {
+ bb.HandleResolvedAddrs(ccs.ResolverState.Addresses, nil)
+ // TODO: This **should** incorporate the lines that warn in `HandleResolvedAddrs`.


E.g. here:

etcd/client/v3/balancer/balancer.go

Line 145 in 5dcd459

bb.lg.Warn("HandleResolvedAddrs called with error", zap.String("balancer-id", bb.id), zap.Error(err))

My goal here was to avoid a big diff in .go source files to make it easier for maintainers to grok what has changed. Notice that HandleResolvedAddrs() is no longer part of the Balancer interface:

https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Balancer

https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Balancer

So we could just rename HandleResolvedAddrs() to UpdateClientConnState() and just return err after some / all of the bb.lg.Warn() statements.

dhermes · 2021-01-20T21:16:45Z

client/v3/balancer/balancer.go

@@ -193,6 +205,11 @@ func (bb *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error)
 }
 }

+// UpdateSubConnState implements "grpc/balancer.Balancer" interface.
+func (bb *baseBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) {
+ bb.HandleSubConnStateChange(sc, s.ConnectivityState)


Ditto here on rename / repurpose of old code. HandleSubConnStateChange() is gone from the Balancer interface so we could just implement that here.

https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Balancer

https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Balancer

dhermes · 2021-01-20T21:17:54Z

client/v3/balancer/balancer.go

+ ConnectivityState: bb.connectivityRecorder.GetCurrentState(),
+ Picker: bb.picker,
+ }
+ bb.currentConn.UpdateState(state)


The ClientConn interface is what has changed here:

https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#ClientConn

https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#ClientConn

dhermes · 2021-01-20T21:18:56Z

client/v3/balancer/picker/err.go

-func (ep *errPicker) Pick(context.Context, balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) {
- return nil, nil, ep.err
+func (ep *errPicker) Pick(balancer.PickInfo) (balancer.PickResult, error) {
+ return balancer.PickResult{}, ep.err


Changed interface

https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Picker

https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Picker

dhermes · 2021-01-20T21:19:08Z

client/v3/balancer/picker/roundrobin_balanced.go

@@ -52,12 +51,12 @@ type rrBalanced struct {
 func (rb *rrBalanced) String() string { return rb.p.String() }

 // Pick is called for every client request.
-func (rb *rrBalanced) Pick(ctx context.Context, opts balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) {
+func (rb *rrBalanced) Pick(opts balancer.PickInfo) (balancer.PickResult, error) {


Changed interface

https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Picker

https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Picker

dhermes · 2021-01-20T21:19:52Z

client/v3/grpcnaming/naming.go

@@ -0,0 +1,33 @@
+package grpcnaming


Source reference: https://github.com/grpc/grpc-go/blob/v1.29.1/naming/naming.go

This is to address CI failure ``` FAIL: inconsistent versions for depencency: google.golang.org/genproto - google.golang.org/genproto@v0.0.0-20200526211855-cb27e3aa2013 from: go.etcd.io/etcd/api/v3 - google.golang.org/genproto@v0.0.0-20200806141610-86f49bd18e98 from: go.etcd.io/etcd/server/v3 FAIL: inconsistent dependencies FAIL: 'dep' failed at Wed Jan 20 21:00:57 UTC 2021 ```

ptabor · 2021-01-20T22:30:47Z

I fully support desire to go to grpc-1.35, but we cannot make it without solving the protobuf-1.4.2. dependency problem first.
We can go to grpc-1.33 (so last prior to grpc/grpc-go@06c094c),
assuming we solved the cmux dependency (soheilhy/cmux#81).

Happy to be proven I have missed something by presenting green tests on consistent protobuf version across modules
(in particular server & raft are sharing the same grpc runtime).
The 'bridging' approach breakes the use-cases that customers do depend on. Unfortunetelly the etcd as name-resolver use-case does not have good code coverage. We cannot assume just 'compiling' code is good.

dhermes · 2021-01-20T22:54:51Z

I fully support desire to go to grpc-1.35, but we cannot make it without solving the protobuf-1.4.2. dependency problem first

Gotcha, seeing some of these failing tests I was starting to patch together that there was a breaking change in core protobuf dependencies. I'll take a stab at rolling them back.

The core issue is the 1.29.1 to 1.30.0 jump so I am happy to try a version earlier than 1.35.0.

The 'bridging' approach breakes the use-cases that customers do depend on

Mind shedding more light on this? The changes here preserve the existing behavior, I'm curious what the concern might be.

…g/protobuf@v1.3.3`. Done via: ``` git grep -l 'google.golang.org/grpc v1.35.0' -- '*go.mod' | xargs sed -i '' s/'google.golang.org\/grpc v1.35.0'/'google.golang.org\/grpc v1.30.0'/g git grep -l 'github.com/golang/protobuf v1.4.2' -- '*go.mod' | xargs sed -i '' s/'github.com\/golang\/protobuf v1.4.2'/'github.com\/golang\/protobuf v1.3.3'/g ```

This included a `replace` directive for the `protobuf` version.

This included a `replace` directive for the `grpc` version.

This included a `replace` directive for the `grpc` and `protobuf` versions.

This included a `replace` directive for the `grpc` version.

dhermes · 2021-01-21T04:42:54Z

@ptabor I didn't realize 1.4.x is the reason the gogoprotobuf maintainers have decided to move on (gogo/protobuf#691)

It seems the only way to cross the 1.3->1.4 chasm for github.com/golang/protobuf is to move away from gogoprotobuf . I know previously gogoprotobuf was appealing over the Google provided implementation because it eschewed runtime reflection and has more idiomatic generated Go types, but I seem to recall that the 1.4 series (with the move to pushing parts of the implementation to google.golang.org/protobuf) tried to solve many of these problems.

UPDATE: I now see #12124 (comment), good to know you are on top of things! Sorry for the noise.

UPDATE: Sadly https://blog.golang.org/protobuf-apiv2 makes no mention of faster marshalling and unmarshalling without reflection.

ptabor · 2021-01-21T05:13:57Z

Mind shedding more light on this? The changes here preserve the existing behavior, I'm curious what the concern might be.

https://github.com/etcd-io/etcd/blob/master/Documentation/dev-guide/grpc_naming.md

Describes usage of etcd as grpc name resolver in customer's application. This requires support from grpc layer that for these interfaces for 1.30+ is gone.

UPDATE: Sadly https://blog.golang.org/protobuf-apiv2 makes no mention of faster marshalling and unmarshalling without reflection.

Elaborated on this in: #12197 (comment)

Proposal to drive this effort to completion:

I will change Replacement API for client/v3/naming package to be compatible with new GRPC1.30+ resolver API. #12614 (API) such it can be submitted side-be-side with existing code.
I think the API is close (if not already) to get @gyuho approval,
I'm looking for contributions that will fill the API with actual code (and I'm promising quick review turn-over).

ptabor · 2021-01-31T10:15:28Z

Could you, please, extract the client/v3/balancer/... change alone.

I think it should be compatible with:
#12652

dhermes · 2021-02-01T19:32:57Z

@ptabor will do. I'll send it out as a new PR. Thanks.

dhermes · 2021-02-01T20:51:35Z

Closing in favor of #12658. May revisit these changes at a later date (likely to just cherry-pick from this PR into a new PR).

Danny Hermes added 16 commits January 20, 2021 14:16

(Temporarily) removing indirect dependencies in go mod.

f790eee

This is intended so that `go mod tidy` can be used to re-generate imports (to prepare for an update of `google.golang.org/grpc`.

Directly vendoring in grpc/naming types to drop the dependency.

5cfaeaf

The official recommendation is to move to `google.golang.org/grpc/resolver` but this is not exactly a drop-in replacement.

Using vendored client/v3/grpcnaming.

061f660

Updating to google.golang.org/grpc@1.35.0 in all go.mod files.

eb1cc38

This is because the `grpc/naming` breaking change happened in `v1.30.0` (i.e. we need to get past `v1.29.1`). See: https://github.com/grpc/grpc-go/releases/tag/v1.30.0

Re-ran go mod tidy in api/.

c1b82bb

Re-ran go mod tidy in pkg/.

9f63964

See etcd-io#12564 (comment) for how I determined which order to visit packages in.

Re-ran go mod tidy in tools/mod/.

1141d4c

Re-ran go mod tidy in client/v2/.

79b2c3a

Re-ran go mod tidy in client/v3/.

04eb8b6

Re-ran go mod tidy in raft/.

e51f86c

Re-ran go mod tidy in server/.

91b12f9

Re-ran go mod tidy in etcdctl/.

41d0e29

Re-ran go mod tidy in tests/.

d4c917e

Re-ran go mod tidy in root package.

4a5ad64

dhermes commented Jan 20, 2021

View reviewed changes

Danny Hermes added 5 commits January 20, 2021 17:34

Re-ran go mod tidy in api/.

f8e52c7

This included a `replace` directive for the `protobuf` version.

Re-ran go mod tidy in pkg/.

5e45a43

Re-ran go mod tidy in client/v2/.

9dbf34c

Re-ran go mod tidy in client/v3/.

6b44f15

Danny Hermes added 5 commits January 20, 2021 17:39

Re-ran go mod tidy in raft/.

8220f16

Re-ran go mod tidy in server/.

60174e7

This included a `replace` directive for the `protobuf` version.

Re-ran go mod tidy in etcdctl/.

7d63cb6

This included a `replace` directive for the `grpc` version.

Re-ran go mod tidy in tests/.

7607937

This included a `replace` directive for the `grpc` and `protobuf` versions.

Re-ran go mod tidy in root directory.

b7f8d58

This included a `replace` directive for the `grpc` version.

kzys mentioned this pull request Jan 21, 2021

client: surface a connection error more clearly containerd/containerd#4447

Merged

ptabor marked this pull request as draft January 22, 2021 21:02

ptabor mentioned this pull request Jan 30, 2021

grpc bump: Implement master/client/v3/naming new endpointManager & resolver #12652

Closed

ptabor closed this Feb 2, 2021

dhermes deleted the remove-grpc-naming-2021-01-20 branch February 2, 2021 15:48

dhermes restored the remove-grpc-naming-2021-01-20 branch February 3, 2021 14:50

dhermes deleted the remove-grpc-naming-2021-01-20 branch February 3, 2021 14:52

dhermes pushed a commit to dhermes/etcd that referenced this pull request Feb 3, 2021

Pulling in all go.mod/go.sum changes from PR etcd-io#12636.

d527983

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to `google.golang.org/grpc@1.35.0` #12636

Upgrade to `google.golang.org/grpc@1.35.0` #12636

dhermes commented Jan 20, 2021

dhermes Jan 20, 2021 •

edited

Loading

dhermes Jan 20, 2021

dhermes Jan 20, 2021

dhermes Jan 20, 2021

dhermes Jan 20, 2021

dhermes Jan 20, 2021

ptabor commented Jan 20, 2021

dhermes commented Jan 20, 2021 •

edited

Loading

dhermes commented Jan 21, 2021 •

edited

Loading

ptabor commented Jan 21, 2021 •

edited

Loading

ptabor commented Jan 31, 2021

dhermes commented Feb 1, 2021

dhermes commented Feb 1, 2021

Upgrade to google.golang.org/grpc@1.35.0 #12636

Upgrade to google.golang.org/grpc@1.35.0 #12636

Conversation

dhermes commented Jan 20, 2021

dhermes Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

dhermes Jan 20, 2021

Choose a reason for hiding this comment

dhermes Jan 20, 2021

Choose a reason for hiding this comment

dhermes Jan 20, 2021

Choose a reason for hiding this comment

dhermes Jan 20, 2021

Choose a reason for hiding this comment

dhermes Jan 20, 2021

Choose a reason for hiding this comment

ptabor commented Jan 20, 2021

dhermes commented Jan 20, 2021 • edited Loading

dhermes commented Jan 21, 2021 • edited Loading

ptabor commented Jan 21, 2021 • edited Loading

ptabor commented Jan 31, 2021

dhermes commented Feb 1, 2021

dhermes commented Feb 1, 2021

Upgrade to `google.golang.org/grpc@1.35.0` #12636

Upgrade to `google.golang.org/grpc@1.35.0` #12636

dhermes Jan 20, 2021 •

edited

Loading

dhermes commented Jan 20, 2021 •

edited

Loading

dhermes commented Jan 21, 2021 •

edited

Loading

ptabor commented Jan 21, 2021 •

edited

Loading