Enable client side gRPC health check by default

[ahrtr]

Followup to #16278

Previously the default gRPC service config for the resolver is {"loadBalancingPolicy": "round_robin"}.

etcd/client/v3/internal/resolver/resolver.go

Line 44 in 7ab7612

r.serviceConfig = cc.ParseServiceConfig(`{"loadBalancingPolicy": "round_robin"}`)

Now I propose to change the default service config to {"loadBalancingPolicy": "round_robin"}, "healthCheckConfig": {"serviceName": ""}. The benefit is that

• any applications which depend on etcd client sdk don't need to care about the low level grpc service config.
• Is it also a best practice to always enable client side health check if the loadBalancingPolicy is round_robin? @dfawley But I am not whether is there any performance penalty? Probably we need to run rw-heatmaps to double confirm.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ahrtr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ahrtr]

Please anyone feel free to work on this on top of this PR.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.73%. Comparing base (7ab7612) to head (fa2078f).
Report is 835 commits behind head on main.

❗ Current head fa2078f differs from pull request most recent head a9f846b

Please upload reports for the commit a9f846b to get more accurate results.

Files with missing lines	Patch %	Lines
server/etcdmain/grpc_proxy.go	0.00%	3 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
client/v3/internal/resolver/resolver.go	84.00% <100.00%> (ø)
server/etcdserver/api/v3rpc/interceptor.go	73.71% <100.00%> (+1.31%)	⬆️
server/etcdmain/grpc_proxy.go	14.91% <0.00%> (-0.14%)	⬇️

... and 20 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18882      +/-   ##
==========================================
+ Coverage   68.72%   68.73%   +0.01%     
==========================================
  Files         420      420              
  Lines       35532    35537       +5     
==========================================
+ Hits        24418    24428      +10     
- Misses       9681     9687       +6     
+ Partials     1433     1422      -11     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab7612...a9f846b. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[serathius]

How this change is related to health checks?

[ahrtr]

gRPC client will call the Watch RPC on the health check service automatically for the stream RPC. Refer to https://grpc.io/docs/guides/health-checking/#enabling-client-health-checking

Most likely we will do similar change for isRPCSupportedForLearner as well,

etcd/server/etcdserver/api/v3rpc/util.go

Lines 142 to 151 in 7ab7612

	func isRPCSupportedForLearner(req any) bool {
	switch r := req.(type) {
	case *pb.StatusRequest:
	return true
	case *pb.RangeRequest:
	return r.Serializable
	default:
	return false
	}
	}

[ahrtr]

The client side sees a grpc error, not sure why.

$ ./bin/etcdctl get k1
2024/11/12 13:49:23 ERROR: [core] [Channel #1 SubChannel #2]Health check is requested but health check function is not set.

I am pretty sure that we have registered the health service and set the healthpb.HealthCheckResponse_SERVING status. @dfawley @easwars @aranjans can you please share some thought on this?

etcd/server/etcdserver/api/v3rpc/grpc.go

Lines 77 to 79 in 7ab7612

	hsrv := health.NewServer()
	healthNotifier := newHealthNotifier(hsrv, s)
	healthpb.RegisterHealthServer(grpcServer, hsrv)

etcd/server/etcdserver/api/v3rpc/health.go

Line 68 in 7ab7612

hc.hs.SetServingStatus(allGRPCServices, healthpb.HealthCheckResponse_SERVING)

[chaochn47]

$ ./bin/etcdctl get k1
2024/11/12 13:49:23 ERROR: [core] [Channel #1 SubChannel #2]Health check is requested but health check function is not set.

@ahrtr Side effect of importing import _ "google.golang.org/grpc/health" would register the health check function here and mentioned in the feature example

[lavacat]

@ahrtr

The client side sees a grpc error, not sure why.

Maybe you need an import that sets internal.HealthCheckFunc

Any danger to roll this out enabled by default? Do we need a config option? I think, yes. The change is minimal but if there are any bugs in grpc health impl, we might need a way to disable.

[ahrtr]

Thanks both. It's a bad pattern to have a blank-import in a non-main package,

internal/resolver/resolver.go:18:2: blank-imports: a blank import should be only in a main or test package, or have a comment justifying it (revive)
	_ "google.golang.org/grpc/health"

[ahrtr]

/retest

[k8s-ci-robot]

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

Test name	Commit	Details	Required	Rerun command
pull-etcd-integration-1-cpu-amd64	a9f846b	link	true	/test pull-etcd-integration-1-cpu-amd64

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.

[easwars]

@arjan-bal

[ahrtr]

Any danger to roll this out enabled by default? Do we need a config option?

YES, we definitely need a config option for this; otherwise it will be a breaking change. If the server side health check isn't enabled, the client will will get an error something like below,

2024/11/13 07:39:59 ERROR: [core] [Channel #1 SubChannel #2]Subchannel health check is unimplemented at server side, thus health check is disabled

Please anyone feel free to continue to work on this task on top of this PR,

• add a config flag and a function something like WithGRPCHealthCheckEnabled for the client side;
• run benchmark test to evaluate the performance impact.

[lavacat]

Please anyone feel free to continue to work on this task on top of this PR

assigned to myself

[ahrtr]

assigned to myself

thx

[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.

[dfawley]

Is it also a best practice to always enable client side health check if the loadBalancingPolicy is round_robin? @dfawley But I am not whether is there any performance penalty? Probably we need to run rw-heatmaps to double confirm.

Enabling heatlh checking by default is probably fine. The performance cost is slightly longer startup times (an extra round-trip or two for the health check RPC, basically). And if the server supports it, then the health checks also occupy one stream per connection. But any server that supports health checking should be expecting that and be OK with it. Providing an easy flag to turn it on/off might be better than turning it on by default, though.

YES, we definitely need a config option for this; otherwise it will be a breaking change. If the server side health check isn't enabled, the client will will get an error something like below,

The client will log this error, but we probably shouldn't have considered it an error. Maybe a warning, or even just "info" would have been more appropriate. The client will behave exactly as it would if health checking had not been enabled.