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

health, grpc: Deliver health service updates through the health listener #7900

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Dec 5, 2024

As part of the dualstack changes described in A61, client side health check updates need to be delivered through the health listener instead of the raw connectivity listener. This PR creates a health producer that manages the health service client and allows registration of a listener for receiving updates. The RegisterHealthListener method in acBalancerWrapper decides whether client side health checking is enabled, if it is, it registers the health listener with the health producer. acBalancerWrapper wraps the LB policy's listener to synchronize updates from the health producer.

Existing health check tests are run against the health producer when the GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST is set to true.

RELEASE NOTES: N/A

@arjan-bal arjan-bal added Type: Feature New features or improvements in behavior Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Dec 5, 2024
@arjan-bal arjan-bal added this to the 1.70 Release milestone Dec 5, 2024
@arjan-bal arjan-bal requested review from easwars and dfawley December 5, 2024 08:10
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 89.02439% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.04%. Comparing base (317271b) to head (5cf90ee).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
health/producer.go 86.66% 4 Missing and 2 partials ⚠️
balancer_wrapper.go 91.89% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7900      +/-   ##
==========================================
- Coverage   82.13%   82.04%   -0.10%     
==========================================
  Files         377      378       +1     
  Lines       38179    38258      +79     
==========================================
+ Hits        31360    31389      +29     
- Misses       5525     5561      +36     
- Partials     1294     1308      +14     
Files with missing lines Coverage Δ
balancer_wrapper.go 85.27% <91.89%> (-0.83%) ⬇️
health/producer.go 86.66% <86.66%> (ø)

... and 19 files with indirect coverage changes

@arjan-bal arjan-bal changed the title health, grpc: Deliver health service updates via the health listener health, grpc: Deliver health service updates through the health listener Dec 5, 2024
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
health/producer.go Outdated Show resolved Hide resolved
test/healthcheck_test.go Outdated Show resolved Hide resolved
test/healthcheck_test.go Show resolved Hide resolved
test/healthcheck_test.go Outdated Show resolved Hide resolved
test/healthcheck_test.go Outdated Show resolved Hide resolved
test/healthcheck_test.go Outdated Show resolved Hide resolved
@arjan-bal arjan-bal force-pushed the health-service-producer branch from ab9cd38 to 956bd64 Compare December 6, 2024 07:50
@arjan-bal arjan-bal assigned easwars and unassigned arjan-bal Dec 6, 2024
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Show resolved Hide resolved
test/healthcheck_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned arjan-bal and unassigned easwars Dec 9, 2024
@arjan-bal arjan-bal assigned easwars and unassigned arjan-bal Dec 10, 2024
balancer_wrapper.go Outdated Show resolved Hide resolved
balancer_wrapper.go Outdated Show resolved Hide resolved
@easwars easwars assigned arjan-bal and unassigned easwars Dec 10, 2024
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Dec 11, 2024
Comment on lines 48 to 51
cancelDone: doneCh,
cancel: grpcsync.OnceFunc(func() {
close(doneCh)
}),
Copy link
Member

Choose a reason for hiding this comment

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

Consider a grpcsync.Event instead of having two fields here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the two fields, one to trigger the cancellation of the context that is passed to the health client and the second to wait till the health check client to return. Waiting for the client close isn't required. The health checking in addrConn also cancels the context and lets the client shut down asynchronously. I got rid of the channel to wait for the client to return here.

grpc-go/clientconn.go

Lines 1342 to 1354 in c1b6b37

onClose := func(r transport.GoAwayReason) {
ac.mu.Lock()
defer ac.mu.Unlock()
// adjust params based on GoAwayReason
ac.adjustParams(r)
if ctx.Err() != nil {
// Already shut down or connection attempt canceled. tearDown() or
// updateAddrs() already cleared the transport and canceled hctx
// via ac.ctx, and we expected this connection to be closed, so do
// nothing here.
return
}
hcancel()

if acbw.healthData != hd {
return
}
if registerFn == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to implement a nopHealthListener that is actually a real implementation that only reports Ready, once, to avoid special-case handling here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to make this method return either the heallh producer's register listener function or a no-op register listener function.

@arjan-bal arjan-bal force-pushed the health-service-producer branch from a45a236 to 5cf90ee Compare December 12, 2024 06:12
@arjan-bal arjan-bal assigned arjan-bal and unassigned dfawley Dec 12, 2024
@arjan-bal arjan-bal merged commit 38a8b9a into grpc:master Dec 12, 2024
15 checks passed
@arjan-bal arjan-bal deleted the health-service-producer branch December 12, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants