*: fix GracefulStop issue when using cmux for TLS

[fuweid]

The gRPC server supports to use GracefulStop to drain all the inflight RPCs, including streaming RPCs.

When we use non-cmux mode to start gRPC server (non-TLS or TLS+gRPC-only), we always invoke GracefulStop to drain requests.

For cmux mode (gRPC.ServeHTTP), since the connection is maintained by http server, gRPC server is unable to send GOAWAY control frame to client. So, it's always force close all the connections and doesn't drain requests by default.

In gRPC v1.61.0 version, it introduces new experimental feature WaitForHandlers to block gRPC.Stop() until all the RPCs finish. This patch is to use WaitForHandlers for cmux mode's graceful shutdown.

This patch also introduces v3rpcBeforeSnapshot failpoint. That's used to verify cmux mode's graceful shutdown behaviour.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[fuweid]

ping @ahrtr @serathius @jmhbnz @siyuanfoundation

[siyuanfoundation]

What happens if the GracefulStop takes too long?

[fuweid]

GracefulStop might be hang when there is deadlock during applying changes. I run into issue that the Snapshot streaming RPCs sends data in very slow rate. The GracefulStop will be blocked until Snapshot finished. However, the connection is closed. Readiness probe failed and then kubelet sends SIGKILL. If there is no probe and force to kill it, the server will be blocked until all the RPCs finished.

[siyuanfoundation]

why is this test affected?

[fuweid]

Because I change gs.Stop to gs.GracefulStop in serving code's cleanup.

etcd/server/embed/serve.go

Lines 159 to 163 in 0cd5999

	defer func(gs *grpc.Server) {
	if err != nil {
	sctx.lg.Warn("stopping insecure grpc server due to error", zap.Error(err))
	gs.Stop()
	sctx.lg.Warn("stopped insecure grpc server due to error", zap.Error(err))

ETCD calls http.Shutdown during stopServers.

etcd/server/embed/etcd.go

Lines 458 to 465 in 0cd5999

	func stopServers(ctx context.Context, ss *servers) {
	// first, close the http.Server
	if ss.http != nil {
	ss.http.Shutdown(ctx)
	}
	if ss.grpc == nil {
	return
	}

That call will close the net.Listener. So both http.Serve and grpc.Serve will exit because of using closed connection. Before this patch, we always call gs.Stop in the following code. It's kind of conflict with stopServers logic which wants graceful shutdown. So, I change it and it takes a little longger than before. Hope it can help

etcd/server/embed/serve.go

Lines 159 to 163 in 0cd5999

	defer func(gs *grpc.Server) {
	if err != nil {
	sctx.lg.Warn("stopping insecure grpc server due to error", zap.Error(err))
	gs.Stop()
	sctx.lg.Warn("stopped insecure grpc server due to error", zap.Error(err))

[ahrtr]

Based on my discussion with @fuweid last week, we might not need this PR?

• Only certain gRPC version (1.60?) has the problem. Current 3.5 and 3.4 are not depending on gRPC 1.60
• also GracefulStop has no timeout, in theory it may be blocked forever.

[fuweid]

Hi @ahrtr

Only certain gRPC version (1.60?) has the problem. Current 3.5 and 3.4 are not depending on gRPC 1.60

grpc/grpc-go#6922 fixed gracefulstop issue introduced by v1.60 version.

For the cmux mode, both http handlers and gRPC handlers share one port, we don't have GracefulStop.
IMO, we should support cmux mode with gracefulstop, because we already support gracefulstop for other modes, like separate ports and http://.

also GracefulStop has no timeout, in theory it may be blocked forever.

I think we can create new issue to track this. For example, add a new flag for timeout of gracefulstop.

[ahrtr]

we should support cmux mode with gracefulstop

We are going to get rid of cmux. Let's revisit this after we finish that. Thanks.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[k8s-ci-robot]

@fuweid: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci-etcd-robustness-release34-amd64	ac95dd7	link	true	/test ci-etcd-robustness-release34-amd64
ci-etcd-robustness-release36-amd64	ac95dd7	link	true	/test ci-etcd-robustness-release36-amd64
ci-etcd-robustness-release35-amd64	ac95dd7	link	true	/test ci-etcd-robustness-release35-amd64
pull-etcd-govulncheck-main	ac95dd7	link	true	/test pull-etcd-govulncheck-main
pull-etcd-govulncheck	ac95dd7	link	true	/test pull-etcd-govulncheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.