HPA: Fix int overflow in GetExternalPerPodMetricReplicas #127050
Conversation
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @omerap12. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
sig/apps
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -369,9 +369,21 @@ func (c *ReplicaCalculator) GetExternalPerPodMetricReplicas(statusReplicas int32 | |||
| usageRatio := float64(usage) / (float64(targetUsagePerPod) * float64(replicaCount)) | |||
| if math.Abs(1.0-usageRatio) > c.tolerance { | |||
| // update number of replicas if the change is large enough | |||
| replicaCount = int32(math.Ceil(float64(usage) / float64(targetUsagePerPod))) | |||
| replicaCountResult := math.Ceil(float64(usage) / float64(targetUsagePerPod)) | |||
There was a problem hiding this comment.
Should we calculate here first and then use the math.Ceil method?
like this:
replicaCountResult := float64(usage) / float64(targetUsagePerPod)
if replicaCountResult > float64(math.MaxInt32) {
replicaCount = math.MaxInt32
} else {
replicaCount = math.Ceil(int32(replicaCountResult))
}There was a problem hiding this comment.
Yes. it makes sense. Ill adjust thanks :)
| expectedReplicas: math.MaxInt32, | ||
| metric: &metricInfo{ | ||
| name: "qps", | ||
| levels: []int64{math.MaxInt64}, |
There was a problem hiding this comment.
I don't quite understand the purpose of this test case. Does our test case test the effect of int overflow?
|
I'm not quite following the backstory behind this fix. Any chance you could give me a scenario to reproduce it? Also, one thing that's got me curious – when it comes to handling integer overflows, why are we only addressing the maximum overflow and not the minimum? I'm wondering what kind of situations could lead to this issue. |
Hey @googs1025, thanks for your review! We might also want to consider addressing the minimum. Perhaps @sheepster1 could provide us with some additional insights. |
|
@googs1025 Wrote about how I encountered this issue over at #126892 The exact setup I am using is keda with the Prometheus scaler querying a value larger than maxInt/0.1. I think this can be mocked with a query like this: |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
What type of PR is this?
/kind bug
What this PR does / why we need it:
Addresses potential integer overflows in
GetExternalPerPodMetricReplicasfunction to prevent unexpected behavior with extreme metric values.Which issue(s) this PR fixes:
As part of #127022, we've identified a need to address potential integer overflows in our scaling calculations. This PR implements the fix for the
GetExternalPerPodMetricReplicasfunction.Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: