fix(metricsservice): continuously monitor gRPC connection for reconnection - #7875
Conversation
✅ 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. |
|
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. |
![semgrep-code-kedacore[bot]](https://avatars.githubusercontent.com/u/49917779?s=60&v=4){kind=small}
ffbee6a to
43d37e3
Compare
|
Fixed — replaced the insecure test gRPC connection/server with a real self-signed TLS cert (matching the pattern used elsewhere in the repo, e.g. pkg/util/certificates_test.go), so this is no longer insecure even in test scope. Thanks for catching it! |
43d37e3 to
ee16046
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves resiliency of the metrics-apiserver’s gRPC connection to the KEDA operator by actively monitoring connection state in the background and driving reconnection immediately after a disconnect (instead of relying on request-scoped metric calls to trigger reconnect attempts).
Changes:
- Updated provider startup logic to continuously loop through “wait for ready → wait while ready → reconnect on disconnect”.
- Added
GrpcClient.WaitWhileConnectionReadyto block until the gRPC connection leaves theReadystate. - Added unit tests for
WaitWhileConnectionReadyand documented the user-visible behavior in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/provider/provider.go | Reworks the startup goroutine to continuously monitor connection readiness and trigger reconnection on disconnect. |
| pkg/metricsservice/client.go | Adds WaitWhileConnectionReady to detect when the connection leaves the Ready state. |
| pkg/metricsservice/client_test.go | Adds tests covering the new WaitWhileConnectionReady behavior. |
| CHANGELOG.md | Adds an Unreleased “Fixes” entry for continuous background reconnection monitoring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rickbrouwer
added
the
waiting-author-response
bb6f2ea to
b37ca5c
Compare
rickbrouwer
removed
the
waiting-author-response
|
Gentle ping — CI is passing on this one. Happy to address any feedback. |
| for { | ||
| if !grpcClient.WaitForConnectionReady(ctx, logger) { | ||
| grpcClientConnected = false | ||
| logger.Error(fmt.Errorf("timeout while waiting to establish gRPC connection to KEDA Metrics Service server"), "timeout", "server", grpcClient.GetServerURL()) | ||
| return | ||
| } |
| grpcClientConnected = true | ||
| logger.Info("Connection to KEDA Metrics Service gRPC server has been successfully established", "server", grpcClient.GetServerURL()) | ||
|
|
||
| if !grpcClient.WaitWhileConnectionReady(ctx, logger) { | ||
| return | ||
| } | ||
| grpcClientConnected = false | ||
| logger.Info("Connection to KEDA Metrics Service gRPC server was lost, attempting to reconnect", "server", grpcClient.GetServerURL()) |
| server, lis, conn := newTLSTestServerAndClientConn(t) | ||
| defer server.Stop() | ||
| defer conn.Close() | ||
|
|
||
| client := &GrpcClient{connection: conn} | ||
| logger := logr.Discard() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| require.True(t, client.WaitForConnectionReady(ctx, logger), "connection should become ready") | ||
|
|
||
| done := make(chan bool, 1) | ||
| go func() { | ||
| done <- client.WaitWhileConnectionReady(ctx, logger) | ||
| }() | ||
|
|
||
| // Stop the server to force the connection out of the Ready state. | ||
| server.Stop() | ||
| _ = lis.Close() |
|
Friendly bump — CI is passing and the branch is conflict-free. Let me know if there's anything to address. |
|
There are still open feedback comments here as well. Review these (check whether they are justified or not). Do not immediately assume that everything is justified; copilot does not always provide the correct feedback. You may resolve the ones that are justified. |
…ction The provider's startup goroutine called WaitForConnectionReady once and then exited, leaving reconnection to be driven entirely by whichever GetExternalMetric request happened to be in flight when the connection dropped. Those calls use a request-scoped context that may not leave enough time for the connection to recover (e.g. after the operator pod's IP changes and DNS re-resolution is rate limited), so the connection can sit unrecovered until another request happens to retry it. Add WaitWhileConnectionReady to block while the connection stays healthy, and loop the monitoring goroutine so it actively re-drives WaitForConnectionReady as soon as the connection leaves the Ready state, independent of incoming requests. Fixes kedacore#7874 Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
b37ca5c to
526a863
Compare
Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
Continuously monitor the metrics service gRPC connection in the background so reconnection is actively driven after a connection loss, instead of depending solely on whichever metric request happens to be in flight at the time.
Checklist
Problem
pkg/provider/provider.go'sNewProviderpreviously launched a one-shot goroutine that calledWaitForConnectionReadyexactly once at startup, then exited regardless of the outcome. After that, the only thing capable of re-driving reconnection was whicheverGetExternalMetricrequest happened to callWaitForConnectionReadynext — and that call uses the request's own (typically short) context deadline.When the keda-operator pod's IP changes (e.g. after a rescheduling event) while the metrics-apiserver's gRPC client still has the old address resolved, recovery depends on a new DNS resolution happening within the lifetime of some incoming request's context. If that doesn't line up favorably, the connection can sit unrecovered for longer than necessary, and the only signal is each individual request failing with a connection error.
Change
WaitWhileConnectionReadytoGrpcClient, which blocks while the connection stays in theReadystate and returns as soon as it transitions away (mirroring the existingWaitForConnectionReady's use ofWaitForStateChangeinstead of busy-polling).NewProviderto loop: wait for ready → wait while ready → on disconnect, immediately loop back intoWaitForConnectionReadywith a long-lived context (not tied to any single request), so reconnection is driven independently of incoming traffic.GetExternalMetric— it's unaffected.Testing
Added
pkg/metricsservice/client_test.gocoveringWaitWhileConnectionReady:trueonce a real local gRPC server is stopped and the connection leavesReady.falseimmediately when the context is already cancelled.Full package suite (
./pkg/metricsservice/...,./pkg/provider/...) passes.Note: I wasn't able to reproduce the exact pod-IP-rotation scenario from the issue in a live cluster, so I can't claim this fully resolves every angle of it — but the previous "watch once, then never again" behavior was clearly insufficient for a long-running server regardless of the precise DNS/grpc-internals timing, so this should be a meaningful improvement either way. Happy to iterate based on feedback.
Closes #7874