General: Keep a trigger that fails metric spec discovery in the HPA - #8198
Open
rickbrouwer wants to merge 5 commits into
Open
rickbrouwer wants to merge 5 commits into
rickbrouwer wants to merge 5 commits into
Conversation
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: kedacore/keda/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds generation-scoped last-known metric-specification fallback and explicit incomplete-discovery errors. HPA and push-scaler reconciliation preserve usable metadata during transient failures. Tests cover discovery failure, recovery, persistence, activation, and fallback scaling. ChangesPartial metric discovery recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ExternalScaler
participant ScalersCache
participant HPAController
participant ScaleHandler
participant ScaledObject
participant TargetDeployment
ExternalScaler-->>ScalersCache: return metric specifications
ScalersCache->>HPAController: provide live or last-known specifications
HPAController->>ScaledObject: retain metric names during incomplete discovery
ScaleHandler->>ScaledObject: request reconciliation for unresolved push metrics
ExternalScaler-->>ScalersCache: become unreachable
ScalersCache-->>HPAController: return partial specifications and an error
ExternalScaler->>TargetDeployment: recover and provide activation
TargetDeployment-->>HPAController: scale after recovery
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
✅ 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. |
Member
Author
|
/run-e2e partial_metric_discovery_test passed tests: 1failed tests: 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 00780298-4496-41fb-b412-e6b0d097d364
📒 Files selected for processing (8)
controllers/keda/hpa.gocontrollers/keda/hpa_test.gocontrollers/keda/scaledobject_controller_test.gopkg/scaling/cache/scalers_cache.gopkg/scaling/cache/scalers_cache_test.gopkg/scaling/scale_handler.gopkg/scaling/scale_handler_test.gotests/internals/partial_metric_discovery/partial_metric_discovery_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kedacore/keda-docs(manual)kedacore/charts(manual)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Member
Author
|
/run-e2e internals|external* passed tests: 43failed tests: 0 |
rickbrouwer
marked this pull request as ready for review
September 18, 2026 13:41
rickbrouwer
marked this pull request as draft
September 18, 2026 13:55
Member
Author
|
/run-e2e partial_metric_discovery_test passed tests: 1failed tests: 0 |
Member
Author
|
/run-e2e internals|external* passed tests: 43failed tests: 0 |
Member
Author
|
/run-e2e internals|external* passed tests: 43failed tests: 0 |
rickbrouwer
marked this pull request as ready for review
September 20, 2026 15:35
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is mainly meant as a proposal, to review the solution direction before investing in polish.
When an external scaler is unreachable, discovery returns nothing for that trigger and the partial result is persisted. The trigger is dropped from the HPA and from status.externalMetricNames, which erases its health status and breaks the metric name lookup the push handler uses, so activations keep being dropped after the scaler recovers.
This keeps the trigger in place instead:
Once the trigger keeps its metric spec, the failing value query reaches the existing fallback path, so failureThreshold and behavior now apply to an unreachable external scaler like they do to any other trigger. Before this we never got that far, because without a spec the trigger was gone before its value was ever queried.
Configuring fallback stays optional: without it the trigger reports an error and the HPA holds its current scale.
Checklist
Fixes #8168