update grpc-go version to v1.32.0 which has some breaking api changes #12398

skyao · 2020-10-15T07:44:59Z

update grpc-go version to v1.32.0 which has some breaking api changes in naming and loadbalancer package.

The discussion is in this issue #12124

In this PR, there are two changes according to the breaking api changes in grpc-go (from v1.29.1 to v1.32.0):

grpc naming package removed

For naming resolver, it is hard to change all the code in etcd packages (naming and grpcproxy) from grpc naming to grpc resolver. A simple way is to copy the grpc naming package (in fact just one file naming.go containing 200 lines code) into etcd naming package.

I put this naming.go file in clientv3/naming/grpcnaming/naming.go.
grpc loadbalancer api changed
```
For loadbalancer, there are some small breaking api changes but one BIG change in Balancer interface...
```
I changed clientv3/balancer/balancer.go by following the code of grpc balancer/base/balancer.go.

Please help to review this file carefully, thanks.

… in naming and loadbalancer package

ptabor

Overall seems go to me. I don't have experience with the grpc balancer code though.
The biggest worry is change of protobuf to 1.4.2

FYI: The balancer tests were flaky before this PR #12372 (comment)

ptabor · 2020-10-15T08:17:17Z

go.mod

-	github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
-	github.com/golang/protobuf v1.3.5
+	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e
+	github.com/golang/protobuf v1.4.2


Updating above protobuf 1.3.x is creating runtime errors, please check:

I've seen:

panic: field raftpb.Message.type has invalid type: got raftpb.MessageType, want pointer goroutine 57 [running]: google.golang.org/protobuf/internal/impl.fieldInfoForScalar(0xd38e40, 0xc00034ca80, 0xbaca27, 0x4, 0x0, 0x0, 0xd38f60, 0xbcd160, 0xbaca2d, 0x45, ...) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message_reflect_field.go:228 +0x7d7 google.golang.org/protobuf/internal/impl.(*MessageInfo).makeKnownFieldsFunc(0xc000166780, 0x180, 0xffffffffffffffff, 0x168, 0xffffffffffffffff, 0xc00035c240, 0xc00035c270, 0xc00035c2a0, 0xc00035c2d0) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message_reflect.go:67 +0x97d google.golang.org/protobuf/internal/impl.(*MessageInfo).makeReflectFuncs(0xc000166780, 0xd38f60, 0xc4aee0, 0x180, 0xffffffffffffffff, 0x168, 0xffffffffffffffff, 0xc00035c240, 0xc00035c270, 0xc00035c2a0, ...) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message_reflect.go:36 +0x65 google.golang.org/protobuf/internal/impl.(*MessageInfo).initOnce(0xc000166780) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message.go:90 +0x192 google.golang.org/protobuf/internal/impl.(*MessageInfo).init(...) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message.go:72 google.golang.org/protobuf/internal/impl.(*messageReflectWrapper).Has(0xc000323300, 0xd38e40, 0xc00034ca80, 0xc00034ca80) /home/ptab/private/golang/pkg/mod/google.golang.org/protobuf@v1.25.0/internal/impl/message_reflect_gen.go:185 +0xf9 github.com/golang/protobuf/proto.(*textWriter).writeMessage(0xc000339f80, 0xd37080, 0xc000323300, 0x0, 0x0) /home/ptab/private/golang/pkg/mod/github.com/golang/protobuf@v1.4.2/proto/text_encode.go:278 +0x935 github.com/golang/protobuf/proto.(*TextMarshaler).marshal(0x10cb480, 0xd2cb40, 0xc000353ba0, 0xc000100210, 0x112a6c0, 0xc0001001e0, 0xc8deb0, 0xc0001dc118) /home/ptab/private/golang/pkg/mod/github.com/golang/protobuf@v1.4.2/proto/text_encode.go:86 +0x190 github.com/golang/protobuf/proto.(*TextMarshaler).Text(...) /home/ptab/private/golang/pkg/mod/github.com/golang/protobuf@v1.4.2/proto/text_encode.go:44 github.com/golang/protobuf/proto.CompactTextString(...) /home/ptab/private/golang/pkg/mod/github.com/golang/protobuf@v1.4.2/proto/text_encode.go:106 go.etcd.io/etcd/v3/raft/raftpb.(*Message).String(...) /home/ptab/corp/etcd/raft/raftpb/raft.pb.go:409 go.etcd.io/etcd/v3/raft.TestNodeProposeWaitDropped.func1(0xc0001f9a40, 0x2, 0x0, 0x1, 0x0, 0x0, 0x0, 0xc000358410, 0x1, 0x1, ...) /home/ptab/corp/etcd/raft/node_test.go:462 +0x31d go.etcd.io/etcd/v3/raft.(*raft).Step(0xc0001f9a40, 0x2, 0x0, 0x1, 0x0, 0x0, 0x0, 0xc000358410, 0x1, 0x1, ...) /home/ptab/corp/etcd/raft/raft.go:990 +0xa58 go.etcd.io/etcd/v3/raft.(*node).run(0xc0003485a0) /home/ptab/corp/etcd/raft/node.go:346 +0xc1c created by go.etcd.io/etcd/v3/raft.TestNodeProposeWaitDropped /home/ptab/corp/etcd/raft/node_test.go:474 +0x545 FAIL go.etcd.io/etcd/v3/raft 0.124s

on test -short -timeout=3m -cpu=4 --race=false ./...

It would be good to stay on 1.3.5 till we will replace gogo with a generator that supports 1.4.x

I checked the grpc code (go.mod file ) that grpc v1.32.0 only requires protobuf 1.3.3:

github.com/golang/protobuf v1.3.3

Why it changed to v1.4.2?

I have added a commit to downgrade the github.com/golang/protobuf version back to 1.3.5, the lasted release before v1.4.

by command go get github.com/golang/protobuf@v1.3.5

I found that when I execute command "go mod tidy", the version of github.com/golang/protobuf will auto changed to version v1.4.2.

clientv3/balancer/balancer.go

ptabor · 2020-10-15T10:09:09Z

Please make sure: PASSES="fmt unit integration" ./test passes locally.
It seems there are actionable test/build failures.

xiang90 · 2020-10-15T22:35:56Z

clientv3/naming/grpc.go

@@ -18,11 +18,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-


@skyao can we mark this package as deprecated? Better, we should move it into the grpcproxy package as an internal implementation detail.

There is no point to expose this anymore since gRPC already stop supporting it. When our users bump gRPC the same version as the etcd client, they cannot use this package anyway. If they use an older version of gRPC, they should also be using an older version of etcd client with naming support.

So we will lose nothing anyway.

ptabor · 2020-10-16T07:31:30Z

The clientv3 API is documented as API for external users (not pure dependency of grpc proxy):

https://etcd.io/docs/v3.4.0/dev-guide/grpc_naming/

Etcd is even mentioned as an (only) example of external resolver:
https://github.com/grpc/grpc/blob/master/doc/naming.md

But indeed just preserving it as copy would be useless to external customers of grpc>=1.30 as grpc.WithBalancer(b).
So to me:

we need to rewrite it to the resolver API
at the same time we need to use the same JSON for update message (so to copy the struct alone).

How about spiting the PR separately for balancer part vs. naming part ?

xiang90 · 2020-10-19T01:01:32Z

@ptabor

The clientv3 API is documented as API for external users (not pure dependency of grpc proxy):

It does not really matter.

The naming package is only for the gRPC-go client to register and resolve endpoints. There are two cases:

Project depends on gRPC < 1.32.0. It might depend on the naming interface provided by gRPC. Then they should use etcd client that depends on gRPC < 1.32.0 which has naming support. So it is fine before or after this change.
Project depends on gRPC >= 1.32.0. Then they SHOULD NOT depend on naming interface provided by gRPC, since gRPC 1.32.0 already removed the support anyway. They should also use etcd client that depends on gRPC >= 1.32.0 which do not support naming package. So it is also just fine.

ResolveError()

skyao · 2020-10-22T03:23:08Z

The clientv3 API is documented as API for external users (not pure dependency of grpc proxy):

https://etcd.io/docs/v3.4.0/dev-guide/grpc_naming/

Etcd is even mentioned as an (only) example of external resolver:
https://github.com/grpc/grpc/blob/master/doc/naming.md

But indeed just preserving it as copy would be useless to external customers of grpc>=1.30 as grpc.WithBalancer(b).
So to me:

we need to rewrite it to the resolver API

at the same time we need to use the same JSON for update message (so to copy the struct alone).

How about spiting the PR separately for balancer part vs. naming part ?

I think we can't split them: if we upgrade the grpc version and only fix one of them, then etcd can't compile.

ptabor · 2020-10-22T07:18:04Z

go.mod

@@ -3,6 +3,7 @@ module go.etcd.io/etcd/v3
 go 1.15

 require (
+	cloud.google.com/go/bigquery v1.6.0 // indirect


Etcd cannot depend on bigquery.

ptabor · 2020-10-22T07:19:09Z

go.mod

+	golang.org/x/time v0.0.0-20191024005414-555d28b269f0
+	golang.org/x/tools v0.0.0-20200806022845-90696ccdc692 // indirect
+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+	google.golang.org/api v0.29.0 // indirect


google.golnag.org/api & appengine are not good dependencies for this module.

git mod graph might help with understanding what is pulling them in.
This would also help with:

#12398 (comment)

(I'm sorry - I overlooked that my comments in review were 'pending' and I assumed they got submitted).

lemonlinger · 2020-11-18T09:28:04Z

Kindly ping~ How's the pr going? @skyao

skyao · 2020-11-19T11:36:16Z

@ptabor shoud we continue for this issue of grpc version upgrade? I think we need to list what to change and also how.

ptabor · 2020-11-19T15:36:45Z

I'm not maintainer here, but my recommendation is following:

TLDR; Copy naming.Update class alone. Re-implement rest in terms of 'grpc.resolver' library.

The naming package: defines a format of key-value pairs that represent a (name -> addr) binding.
The format is that under the key: target+ "/"+addr we persist a json:
{ Op: 'Add', Addr: '192.168.0.15', Metadata: ''}
it uses marshaling of the naming.Update format.
The API is public: https://etcd.io/docs/v3.4.0/dev-guide/grpc_naming/
We need to assume some customers have etcd 3.4.0 instance and speak to it using client libraries coming from etcd 3.5.0 and 3.4.0. There is no atomic switch of 'client libraries'. It might happen that some customer wrote (resolver.Update) using 3.4.0:

r.Update(context.TODO(), "my-service", naming.Update{Op: naming.Delete, Addr: "1.2.3.4"})

While they want to use etcd 3.5.0 client and use https://godoc.org/google.golang.org/grpc#WithResolvers to let grpc use etcd for naming resolution.

Thus the format provided by: naming.Update{...} must not change, while the use-case of using of etcd as a naming resolution source should be preserved. Of course the code that users used to integrate the resolver from 3.5.0 will be different that the code that used to use with 3.4.0, but that's OK as @xiang90 pointed out.

I think we can't split them: if we upgrade the grpc version and only fix one of them, then etcd can't compile.
I believe grpc 1.29.1 supports both: old and new interfaces. So I would assume we can do partial changes without bumping the grpc version, and bump when we are ready with all the changes.

ptabor · 2020-12-17T16:33:32Z

@skyao Do you plan to continue working on this issue ?

skyao · 2020-12-30T07:06:06Z

@skyao Do you plan to continue working on this issue ?

I would like to continue this work, but now I'm comfused about how to continue...... Is there any conclusion?

ptabor · 2020-12-30T14:51:45Z

@skyao: I assumed that if we get the state from the recommendation (#12398 (comment)), nobody would complain that its bad outcome for the product.

The main risk factor is me overlooking something, thus making this proposal infeasible. If you think, you can implement it,
I would assume its the best verification and it will get committed (a lot of customers struggle with the issue).

@xiang90 @gyuho @jpbetz Could you, please, review the recommendation from #12398 (comment) to unblock work on this issue ?

skyao · 2021-01-10T10:31:54Z

I would like to try my best to continue this topic of grpc version upgrade, but it seems the topic is too big to discuss and update code in ONE PR (and also this original PR is now have too many conflicting to current master branch).

I will split this PR to some new and small PRs which will just focus in one specified issue in grpc version upgrade:

naming package is removed in grpc v1.30.0

New PR is here: Prepar to upgrade grpc version: grpc naming packge is moved in v1.30.0 #12609

In this PR we will still stay at grpc v1.29.1 but prepare to upgrade to new version which removed grpc naming package.
grpc loadbalancer api has some broken changes
we need to decide which versions should we upgrade to: including grpc / protobuf / gogo-protobuf ...

ptabor · 2021-01-31T22:30:57Z

Closing this one in favor of: #12652.

If balancer alone change passes all the tests - we will need it.

update grpc-go version to v1.32.0 which has some breaking api changes…

2859e9f

… in naming and loadbalancer package

skyao mentioned this pull request Oct 15, 2020

Update grpc-go to latest version dapr/dapr#2101

Closed

ptabor mentioned this pull request Oct 15, 2020

picker: replace picker with V2Picker ，picker is deprecated #12384

Closed

ptabor suggested changes Oct 15, 2020

View reviewed changes

xiang90 reviewed Oct 15, 2020

View reviewed changes

skyao mentioned this pull request Oct 16, 2020

REQUEST: New membership for @skyao dapr/community#25

Closed

4 tasks

skyao added 2 commits October 22, 2020 11:14

downgrade github.com/golang/protobuf version to 1.3.5

4d66d4b

add doc for method

b1d600e

ResolveError()

update to pass fmt checking

5ba58f2

ptabor reviewed Oct 27, 2020

View reviewed changes

ptabor mentioned this pull request Oct 27, 2020

Is there a plan to update google.golang.org/grpc? #11650

Closed

klizhentas mentioned this pull request Nov 6, 2020

Update to grpc 1.32 gravitational/teleport#4727

Closed

ptabor mentioned this pull request Nov 12, 2020

Please upgrade grpc version used in etcd go.mod #12470

Closed

joe-elliott mentioned this pull request Nov 25, 2020

build error grafana/tempo#371

Closed

lindluni mentioned this pull request Dec 3, 2020

[WIP][FAB-18363] Update Protobuf to 1.4.3 hyperledger/fabric#2185

Closed

dims mentioned this pull request Dec 3, 2020

Need a replacement for deprecated/unmaintained gogo-protobuf dependency kubernetes/kubernetes#96564

Open

ptabor mentioned this pull request Dec 21, 2020

Get clientv3 error with grpc v1.33.2 #12571

Closed

scDisorder mentioned this pull request Jan 4, 2021

Update grpc-go to v1.33.0 #12597

Closed

rmfitzpatrick mentioned this pull request Jan 5, 2021

Add MonitorCustomConfig support to Smart Agent Receiver signalfx/splunk-otel-collector#54

Merged

ptabor mentioned this pull request Jan 7, 2021

etcd not compatible with grpc v1.30.0 #12124

Closed

scDisorder mentioned this pull request Jan 9, 2021

do not use grpc/naming package #12608

Closed

skyao mentioned this pull request Jan 10, 2021

Prepar to upgrade grpc version: grpc naming packge is moved in v1.30.0 #12609

Closed

omaskery mentioned this pull request Jan 18, 2021

Upgrade etcd/grpc version tikv/client-go#63

Closed

ptabor closed this Jan 31, 2021

fuweid mentioned this pull request Oct 17, 2023

Resolve CVE-2023-44487 #16740

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update grpc-go version to v1.32.0 which has some breaking api changes #12398

update grpc-go version to v1.32.0 which has some breaking api changes #12398

skyao commented Oct 15, 2020

ptabor left a comment

ptabor Oct 15, 2020

skyao Oct 18, 2020

skyao Oct 22, 2020

skyao Oct 22, 2020

skyao Oct 22, 2020

ptabor commented Oct 15, 2020

xiang90 Oct 15, 2020

ptabor commented Oct 16, 2020

xiang90 commented Oct 19, 2020

skyao commented Oct 22, 2020

ptabor Oct 22, 2020

ptabor Oct 22, 2020

ptabor Oct 27, 2020

lemonlinger commented Nov 18, 2020

skyao commented Nov 19, 2020

ptabor commented Nov 19, 2020

ptabor commented Dec 17, 2020

skyao commented Dec 30, 2020

ptabor commented Dec 30, 2020 •

edited

Loading

skyao commented Jan 10, 2021 •

edited

Loading

ptabor commented Jan 31, 2021

update grpc-go version to v1.32.0 which has some breaking api changes #12398

update grpc-go version to v1.32.0 which has some breaking api changes #12398

Conversation

skyao commented Oct 15, 2020

ptabor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptabor commented Oct 15, 2020

Choose a reason for hiding this comment

ptabor commented Oct 16, 2020

xiang90 commented Oct 19, 2020

skyao commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonlinger commented Nov 18, 2020

skyao commented Nov 19, 2020

ptabor commented Nov 19, 2020

ptabor commented Dec 17, 2020

skyao commented Dec 30, 2020

ptabor commented Dec 30, 2020 • edited Loading

skyao commented Jan 10, 2021 • edited Loading

ptabor commented Jan 31, 2021

ptabor commented Dec 30, 2020 •

edited

Loading

skyao commented Jan 10, 2021 •

edited

Loading