-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add grpc client-side reconnection #4598
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4598 +/- ##
===========================================
- Coverage 36.15% 15.68% -20.48%
===========================================
Files 14 9 -5
Lines 3864 1760 -2104
===========================================
- Hits 1397 276 -1121
+ Misses 2349 1459 -890
+ Partials 118 25 -93
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
633b4ab
to
457b01b
Compare
// | ||
// Note: the retry policy only has an effect if ws-manager is run with the GRPC_GO_RETRY=on env var set. | ||
// see https://github.com/grpc/grpc-go/blob/506b7730668b5a13465224b0d8133f974a3f843d/dialoptions.go#L522-L524 | ||
var retryPolicy = `{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this is fine because
- we never had
GRPC_GO_RETRY
set, or because - some other option is taking over the functionality this config provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some other option is taking over the functionality this config provided?
The changes in the PR introduce three changes:
- grpc.KeepaliveEnforcementPolicy
Allow more frequent pings. Default is 5 minutes https://github.com/grpc/grpc-go/blob/87eb5b7502493f758e76c4d09430c0049a81a557/internal/transport/defaults.go#L40
This implies that more frequent pings result in an error HTTP/2 error code: ENHANCE_YOUR_CALM Received Goaway too_many_pings
- grpc.WithKeepaliveParams
Adjust keepalive ping interval to avoid the default https://github.com/grpc/grpc-go/blob/87eb5b7502493f758e76c4d09430c0049a81a557/internal/transport/defaults.go#L35
and quickly to restart of pods. Also, enablesPermitWithoutStream
keepalive pings even with no active RPCs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we never had GRPC_GO_RETRY set, or because
Good point. This env variable was configured only in ws-manager
. Now is present in all components with grpc
457b01b
to
0b6923c
Compare
In case of connection errors (pod restart), reconnect.
Example error:
{"@type":"type.googleapis.com/google.devtools.clouderrorreporting.v1beta1.ReportedErrorEvent","error":"invalid ref: rpc error: code = Unavailable desc = error reading from server: read tcp 10.0.128.131:55298-\u003e172.20.175.142:8080: read: no route to host:\n github.com/gitpod-io/gitpod/registry-facade/pkg/registry.(*RemoteSpecProvider).GetSpec\n github.com/gitpod-io/gitpod/registry-facade/pkg/registry/imagecfg.go:55\n - invalid
How to test:
ws-manager
podRight now we only see errors in the log (and "Pulling container image …")