-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Bugfix - Metrics API / aggregate from kubernetes service endpoint slices : add missing final semaphore acquisition #7370
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
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. |
bbcc2ea to
b868224
Compare
the final semaphore acquisition was still missing, which resulted in loop not waiting until all metrics were gathered from each metrics API server Signed-off-by: julian GUINARD <[email protected]>
b868224 to
791b8f5
Compare
|
/run-e2e metrics_api |
|
thanks @rickbrouwer , e2e passed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! lgtm!
wozniakjan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a synchronization bug in the metrics API scaler where the function would not wait for all goroutines to complete before aggregating results from multiple Kubernetes service endpoints. The fix adds a final semaphore acquisition to ensure all metrics have been gathered before proceeding.
Key Changes:
- Added final semaphore acquisition to wait for all goroutines to complete their work
- Added debug logging to track the start and end of endpoint iteration
Comments suppressed due to low confidence (1)
pkg/scalers/metrics_api_scaler.go:453
- The nbErrors and expectedNbMetrics variables are accessed without mutex protection after the semaphore acquisition. While the semaphore should ensure goroutines have completed, if the semaphore acquisition fails (line 442-444), these variables may still be modified by running goroutines, creating a race condition. Consider protecting these reads with the mutex or returning early if semaphore acquisition fails.
if nbErrors > 0 && nbErrors == len(endpointsUrls) {
err = fmt.Errorf("could not get any metric successfully from the %d provided endpoints", len(endpointsUrls))
}
if s.metadata.AggregationType == AverageAggregationType {
aggregation /= float64(expectedNbMetrics)
}
s.logger.V(1).Info(fmt.Sprintf("fetched %d metrics out of %d endpoints from kubernetes service : %s is %v\n", expectedNbMetrics, len(endpointsUrls), s.metadata.AggregationType, aggregation))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
the final semaphore acquisition was still missing, which resulted in loop not waiting until all metrics were gathered from each metrics API server Signed-off-by: julian GUINARD <[email protected]> Signed-off-by: Dima Altukhov <[email protected]>
this relates to discussion #6565 (comment) of PR 6565
the final semaphore acquisition was still missing, which resulted in loop not waiting until all metrics were gathered from each metrics API server
Checklist