Fix watch stream starvation #15403
Fix watch stream starvation #15403
Conversation
serathius
force-pushed
the
fix-watch-starving
branch
5 times, most recently
from
f705b1b to
51bf64c
Compare
|
This change breaks curl test :( Marking as draft for debug |
| @@ -51,6 +51,7 @@ func (e e2eRunner) NewCluster(ctx context.Context, t testing.TB, opts ...config. | |||
| WithStrictReconfigCheck(cfg.StrictReconfigCheck), | |||
| WithAuthTokenOpts(cfg.AuthToken), | |||
| WithSnapshotCount(cfg.SnapshotCount), | |||
| WithWatchProcessNotifyInterval(cfg.WatchProgressNotifyInterval), | |||
There was a problem hiding this comment.
Why do we bundle this with this PR ?
I assume the change should be neutral for the core-logic change... and is here due to needs of the grpc_test ?
There was a problem hiding this comment.
Yes, this change comes with 3 dedicated tests for all types of watch response. I needed to change some code to expose periodic progress notification.
server/embed/serve.go
Outdated
| } | ||
| m := cmux.New(listener) | ||
| go func() { errHandler(srv.Serve(m.Match(cmux.HTTP1()))) }() | ||
| go func() { errHandler(gs.Serve(m.Match(cmux.HTTP2()))) }() |
There was a problem hiding this comment.
Maybe add a TODO to consider switching to:
cmux.HTTP2HeaderField("content-type", "application/grpc")
to avoid issues if any client/prometheous switches to http2.
There was a problem hiding this comment.
I think what you suggest will be better, however we need to validate if it breaks non-TLS in any way.
server/embed/serve.go
Outdated
| sctx.lg.Error("Configure https server failed", zap.Error(err)) | ||
| listener := sctx.l | ||
| if sctx.secure { | ||
| listener, err = transport.NewTLSListener(listener, tlsinfo) |
There was a problem hiding this comment.
Are we covering here /healthz and /metrics endpoints with mTLS that used to be unprotected previously ?
… when sharing the same connection Signed-off-by: Marek Siarkowicz <[email protected]>
Signed-off-by: Marek Siarkowicz <[email protected]>
Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
fix-watch-starving
branch
from
51bf64c to
85b2fc9
Compare
Signed-off-by: Marek Siarkowicz <[email protected]>
Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
fix-watch-starving
branch
from
85b2fc9 to
17a73ea
Compare
|
Closing for #15431 |
Ref #15402
Cleaning up serving code and moving grpc server from under http server. Now multiplexing between grpc API and http API will be the same for both TLS and non-TLS config. It will be done by checking HTTP1 (http) vs HTTP2 (grpc).
cc @ahrtr @ptabor