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

vendor: bump grpc to v1.27.1 #49114

Merged
merged 1 commit into from
May 21, 2020
Merged

vendor: bump grpc to v1.27.1 #49114

merged 1 commit into from
May 21, 2020

Conversation

tbg
Copy link
Member

@tbg tbg commented May 15, 2020

Upgrade gRPC to 1.27.1 (latest is 1.29). This doesn't pick up anything that we need, but it claims to contain some improvements, and now is the right time in the cycle to bump if we're going to.

TODO: does this want a release note?

Notable PRs (from gRPC-go's release notes):

Release note (general change): gRPC was updated from 1.21 to 1.27.1.

@tbg tbg requested a review from petermattis May 15, 2020 12:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm:

I'm not sure about the release note. This isn't a user visible change, nor does it fix any known bugs. @knz might have a stronger opinion.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @petermattis)

@knz
Copy link
Contributor

knz commented May 15, 2020

The first change gets rid of Decompressor is not installed for grpc-encoding "gzip" which we know was happening at least in the cli quit so that's code we can simplify.

The last one was cause of log spam when nodes disconnect in the cluster.

For the first one no need for a release note because of the code I added to hide the error, but maybe remove that code or file the cleanup issue and assign it to me.

The last one may or may not need a release note, but that's something that support may want to be pinged about.

@tbg
Copy link
Member Author

tbg commented May 19, 2020

Of course there's a yak shave. Bumping grpc updates this file

https://github.com/grpc/grpc-go/blob/master/health/grpc_health_v1/health.pb.go#L25

Note the IsV3 - used to be IsV2. (the file is included as a dependency via etcd)

This in turn requires us to update github.com/golang/protobuf to v1.4+

At that version, github.com/golang/protobuf/protoc-gen-go/generator/generator.go

emits a warning at init() time:

WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
A future release of golang/protobuf will delete this package,
which has long been excluded from the compatibility promise.

We treat warnings as errors in protoc, so now make protobuf fails. I want to continue treating warnings as errors.

Still pulling on this and trying to understand who actually pulls that package in and whether we can convince them to use the suggested replacement package google.golang.org/protobuf/compiler/protogen. I hope it's not gogo/protobuf but I'll probably find that it is (gogoproto is not well maintained any more).

@tbg
Copy link
Member Author

tbg commented May 19, 2020

Here are the importers:

go list -f '{{.ImportPath}} {{join .Imports " "}}' ./pkg/... ./vendor/... | digraph reverse github.com/cockroachdb/cockroach/vendor/github.com/golang/protobuf/protoc-gen-go/generator
github.com/cockroachdb/cockroach/vendor/github.com/golang/protobuf/protoc-gen-go/generator
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/generator
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/gengateway
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger

Well that sucks.

@tbg
Copy link
Member Author

tbg commented May 19, 2020

Ok I might have it. grpc-gateway bump did the trick. Verifying now.

@tbg
Copy link
Member Author

tbg commented May 19, 2020

Ugh. So after grpc-gateway bump its protoc plugin started emitting warnings for RPCs that didn't have a HttpRule. I tried using

    option (google.api.http) = {    };

which almost works except pbjs throws up:

/home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/protobufjs/cli/pbjs.js:235
            throw err;
            ^

Error: illegal name '}' (pkg/server/serverpb/admin.proto, line 868)
    at illegal (/home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/protobufjs/src/parse.js:94:16)

Others have complained about this warning as well but the outcome is unfortunately that the warning stayed and Uber (the "others") wrapped their prototool to remove the warning from stderr. What a waste of everyone's time.

grpc-ecosystem/grpc-gateway@ee51345

As it stands, it seems that we're going to have to wrap the grpc-gateway gen tool, I think?

@tbg
Copy link
Member Author

tbg commented May 19, 2020

I'm going to fork grpc-gateway and nuke the warning message instead.

@tbg
Copy link
Member Author

tbg commented May 20, 2020

Ok this should now build. @petermattis mind giving this another look?

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: though I hope the Gopkg.toml changes haven't made a switch to go mod more difficult.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@tbg
Copy link
Member Author

tbg commented May 20, 2020

@otan / @jordanlewis (I think you're still team go mod, right?) want me to hold off here? I'm happy figuring all of the above out in a go mod world one more time provided it looks like go mod is still en route to success soonish.

@jordanlewis
Copy link
Member

We already have a bunch of Gopkg.toml changes since our first cut. We're going to have to completely redo it anyway. So I don't think this is going to make thigns much worse, right @otan?

@otan
Copy link
Contributor

otan commented May 20, 2020

yep, going to have to re-do it. feel free to further ahead / get this in (before Friday would be great)

@tbg
Copy link
Member Author

tbg commented May 20, 2020

bors r=petermattis

@craig
Copy link
Contributor

craig bot commented May 20, 2020

Build failed

@tbg tbg requested a review from a team as a code owner May 21, 2020 10:18
@tbg
Copy link
Member Author

tbg commented May 21, 2020

bors r=petermattis

Filed #49359 for the issue @knz mentioned.

cc @roncrdb about #49114 (comment) (sorry, not sure how to ping "support" from an issue).

@craig
Copy link
Contributor

craig bot commented May 21, 2020

Merge conflict

@tbg tbg force-pushed the grpc-127 branch 2 times, most recently from 1ab21ca to 542884c Compare May 21, 2020 10:28
Upgrade gRPC to 1.27.1 (latest is 1.29). This doesn't pick up anything that we need, but it claims to contain some improvements, and now is the right time in the cycle to bump if we're going to.

Notable PRs (from gRPC-go's release notes):

- client: fix race between client-side stream cancellation and compressed server data arriving (grpc/grpc-go#3054)
- clientconn: fix potential deadlock caused by ResetConnectBackoff (grpc/grpc-go#3051)
- internal: fix context leak when stream is not created successfully (grpc/grpc-go#2985)
- server: set and advertise max frame size of 16KB (grpc/grpc-go#3018)
- transport: block reading frames when too many transport control frames are queued (grpc/grpc-go#2970)
    Addresses CVE-2019-9512 (Ping Flood), CVE-2019-9514 (Reset Flood), and CVE-2019-9515 (Settings Flood).
- server: avoid an unnecessary allocation per-RPC for OK status (grpc/grpc-go#2920)
        Special Thanks: @dzbarsky
- server: avoid call to trace.FromContext and resulting allocations when tracing is disabled (grpc/grpc-go#2926)
        Special Thanks: @dzbarsky
- http2client: remove unnecessary allocations for header fields (grpc/grpc-go#2925)
        Special Thanks: @dzbarsky
- status: avoid allocations when returning an OK status (grpc/grpc-go#2929)
        Special Thanks: @dzbarsky
- server: avoid allocations related to tracking excessive pings (grpc/grpc-go#2923)
        Special Thanks: @dzbarsky
- transport: call Unlock in defer to avoid data race (grpc/grpc-go#2953)
- client: fix canceled vs deadline exceeded double-check logic (grpc/grpc-go#2906)
- client: fix race between transport draining and new RPCs (grpc/grpc-go#2919)

Of course there was also a yak shave. Bumping grpc updates this file

https://github.com/grpc/grpc-go/blob/master/health/grpc_health_v1/health.pb.go#L25

Note the IsV3 - used to be IsV2. (the file is included as a dependency via etcd)

This in turn requires us to update github.com/golang/protobuf to v1.4+

At that version, `github.com/golang/protobuf/protoc-gen-go/generator/generator.go`

emits a warning at `init()` time:

> WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
        A future release of golang/protobuf will delete this package,
        which has long been excluded from the compatibility promise.

We treat warnings as errors in protoc, so now `make protobuf` fails. I
want to continue treating warnings as errors. So we also bump the
importer, which is grpc-gateway:

```
go list -f '{{.ImportPath}} {{join .Imports " "}}' ./pkg/... ./vendor/... | digraph reverse github.com/cockroachdb/cockroach/vendor/github.com/golang/protobuf/protoc-gen-go/generator
github.com/cockroachdb/cockroach/vendor/github.com/golang/protobuf/protoc-gen-go/generator
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/generator
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/gengateway
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
github.com/cockroachdb/cockroach/vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger
```

That new version of grpc-gateway added a silly warning of its own, so we
now use our own fork that has that warning stripped out.

Release note: None
@tbg
Copy link
Member Author

tbg commented May 21, 2020

Had to re-do the vendor dir due to merge skew

bors r=petermattis

@tbg
Copy link
Member Author

tbg commented May 21, 2020

Build is not running, dear bors

bors r-

@craig
Copy link
Contributor

craig bot commented May 21, 2020

Canceled

@tbg
Copy link
Member Author

tbg commented May 21, 2020

bors r=petermattis

@craig
Copy link
Contributor

craig bot commented May 21, 2020

Build succeeded

@craig craig bot merged commit cdaacc5 into cockroachdb:master May 21, 2020
@tbg tbg deleted the grpc-127 branch May 21, 2020 12:11
aliher1911 added a commit to aliher1911/cockroach that referenced this pull request Oct 11, 2021
Changes in cockroachdb#49114 lead to default connection timeout value
dropping to 0 instead of default 20s.
This is not always enough for multiregion clients with hub spoke
network topologies as connections doesn't have enought time to go
through initially.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Default connection timeout for grpc
connections is set to 20s to match default value.
aliher1911 added a commit that referenced this pull request Oct 11, 2021
Changes in #49114 lead to default connection timeout value
dropping to 0 instead of default 20s.
This is not always enough for multiregion clients with hub spoke
network topologies as connections doesn't have enought time to go
through initially.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Default connection timeout for grpc
connections is set to 20s to match default value.
aliher1911 added a commit to aliher1911/cockroach that referenced this pull request Oct 12, 2021
Changes in cockroachdb#49114 lead to default connect timeout value dropping
to 0 instead of default 20s.

This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.
craig bot pushed a commit that referenced this pull request Oct 12, 2021
71417: rpc: restore 20s connect timeout for grpc dialer r=erikgrinaker a=aliher1911

Changes in #49114 lead to default connect timeout value
dropping to 0 instead of default 20s.
This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.

Fixes #71411

71438: flowinfra: unify CleanupBeforeRun and Cleanup r=yuzefovich a=yuzefovich

We recently introduced `CleanupBeforeRun` to the `Flow` interface which
should be called in case the flow is never `Run()` or `Start()`ed. Its
implementation is the same as `Cleanup` for the row-based flows but is
slightly different from `vectorizedFlow.Cleanup` for the vectorized
flow. This commit unifies the two methods into one: namely, we only need
to be careful to not assert that all closers have been closed if the
flow is not started. I think it is acceptable given that the idea with
closers is that they need to release the resources which are supposed to
be allocated only when the closers become used (for example, we always
instantiate disk queues lazily, behind the spilling queue implementation
which starts out with no disk resources used in the constructor).

Release note: None

Co-authored-by: Oleg Afanasyev <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
aliher1911 added a commit to aliher1911/cockroach that referenced this pull request Oct 13, 2021
Changes in cockroachdb#49114 lead to default connect timeout value dropping
to 0 instead of default 20s.

This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.
aliher1911 added a commit to aliher1911/cockroach that referenced this pull request Oct 13, 2021
Changes in cockroachdb#49114 lead to default connect timeout value dropping
to 0 instead of default 20s.

This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.
blathers-crl bot pushed a commit that referenced this pull request Oct 13, 2021
Changes in #49114 lead to default connect timeout value dropping
to 0 instead of default 20s.

This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.
ericharmeling pushed a commit to ericharmeling/cockroach that referenced this pull request Oct 20, 2021
Changes in cockroachdb#49114 lead to default connect timeout value dropping
to 0 instead of default 20s.

This is not always enough for multiregion clients with hub spoke
network topologies as connections don't have enough time to initially
go through.
This change restores initial value that matches grpc defaults.

Release note (bug fix): Connect timeout for grpc connections is
set to 20s to match pre 20.2 default value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants