fix(metricsservice): improve gRPC connection resilience with keepalive and proper state waiting#7463
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
rohansood10
force-pushed
the
fix/grpc-reconnect-7251
branch
from
5d775fd to
3041c23
Compare
|
Good catch @rickbrouwer! Removed the temporal changes from this PR — they belong in #7462. Force-pushed to clean up the history. |
|
Friendly ping — any chance to review this when you get a moment? Happy to make changes if needed. |
|
/run-e2e internal |
|
Thanks @JorTurFer for the approval! All CI checks have passed. This PR improves gRPC connection resilience in the metrics service with keepalive settings and proper state waiting. |
|
Thanks for the improvements @rohansood10 , Question, in |
|
Hi @rickbrouwer, thanks for the review! Great questions:
|
|
Hi @rickbrouwer, Great observation! You're right that checking the connection state before waiting could improve the user experience. Here's my analysis: Current approach pros:
Your suggestion pros:
Proposed solution: func (c *GrpcClient) GetMetrics(ctx context.Context, scaledObjectRef *v1alpha1.ScaledObject) (*external_metrics.ExternalMetricValueList, error) {
// Quick check for known-bad connection state
if c.connection.GetState() == connectivity.Shutdown {
return nil, fmt.Errorf("gRPC connection is shutdown")
}
// Existing WaitForReady logic for transient failures
err := c.waitForConnectionReady(ctx, scaledObjectRef.Name, scaledObjectRef.Namespace)
if err != nil {
return nil, err
}
// ... rest of the method
}This way we fail fast for permanently failed connections while still handling transient network issues gracefully. What do you think? |
1 similar comment
|
Hi @rickbrouwer, Great observation! You're right that checking the connection state before waiting could improve the user experience. Here's my analysis: Current approach pros:
Your suggestion pros:
Proposed solution: func (c *GrpcClient) GetMetrics(ctx context.Context, scaledObjectRef *v1alpha1.ScaledObject) (*external_metrics.ExternalMetricValueList, error) {
// Quick check for known-bad connection state
if c.connection.GetState() == connectivity.Shutdown {
return nil, fmt.Errorf("gRPC connection is shutdown")
}
// Existing WaitForReady logic for transient failures
err := c.waitForConnectionReady(ctx, scaledObjectRef.Name, scaledObjectRef.Namespace)
if err != nil {
return nil, err
}
// ... rest of the method
}This way we fail fast for permanently failed connections while still handling transient network issues gracefully. What do you think? |
|
@rickbrouwer I've added a fail-fast check for the Shutdown state in GetMetrics - if the connection is shut down, it returns an error immediately instead of waiting for the context timeout. Also added a CHANGELOG entry. Let me know what you think! |
…e and proper state waiting - Add gRPC keepalive parameters (ping every 30s, 10s timeout, permit without stream) - Add ConnectParams with 5s minimum connect timeout - Replace busy-polling WaitForConnectionReady with WaitForStateChange-based approach - Add fail-fast check for Shutdown state in GetMetrics Fixes kedacore#7251 Signed-off-by: Rohan Sood <[email protected]>
rohansood10
force-pushed
the
fix/grpc-reconnect-7251
branch
from
210e5b1 to
1965949
Compare
|
@rickbrouwer just checking in - any further thoughts on this? happy to make changes if needed |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
github-actions
Bot
added
the
stale
rickbrouwer
removed
the
stale
Signed-off-by: Rick Brouwer <[email protected]>
|
/run-e2e internal passed tests: 34failed tests: 0 |
…e and proper state waiting (kedacore#7463) Signed-off-by: Rohan Sood <[email protected]> Signed-off-by: Rick Brouwer <[email protected]> Co-authored-by: Rick Brouwer <[email protected]> Signed-off-by: Yurii Shcherbak <[email protected]>
Description
Fixes the issue where the gRPC connection between the metrics API server and the KEDA operator gets permanently stuck, requiring a pod restart to recover.
Root Cause
Two issues contributed to the stuck connection:
Busy-polling in WaitForConnectionReady: The function used a tight loop calling
Connect()every 500ms. This interfered with gRPC's internal reconnection backoff mechanism — eachConnect()call could reset the backoff, preventing the connection from stabilizing. The gRPC documentation recommends usingWaitForStateChangeinstead.No keepalive configuration: Dead TCP connections (e.g., after network partitions, pod rescheduling, or DNS changes) were not detected until the next RPC attempt timed out. Without keepalive pings, a connection could appear healthy but be completely unresponsive.
Fix
WaitForStateChange: This blocks efficiently until the connection state transitions, allowing gRPC's built-in exponential backoff to work correctlyPermitWithoutStream: true) to detect dead connections earlyConnectParams: Sets a 5-second minimum connect timeout for faster initial establishmentGetMetricsnow checks the connection state upfront and returns an error immediately if the connection is shut down, instead of waiting for the context timeoutTesting
The package has no existing test files. Changes were verified via compilation. Integration testing in a cluster environment is recommended.
Checklist
Fixes #7251