-
Notifications
You must be signed in to change notification settings - Fork 167
fix: update Hyperdisk attach limits to match GCP documentation for Ge… #2127
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
fix: update Hyperdisk attach limits to match GCP documentation for Ge… #2127
Conversation
Welcome @arsiesys! |
Hi @arsiesys. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
/ok-to-test |
pkg/common/utils.go
Outdated
if len(limitMap) > 0 { | ||
return limitMap[len(limitMap)-1].value | ||
} | ||
return 0 |
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.
Do not think we should return 0 here, maybe use the default volumeLimitSmall.
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.
I can return a static 15 as the constant is declared in node.go.
Or, I could move the constant from node.go to the common package, let me know which path we prefer.
7b49535
to
6df040f
Compare
/test pull-gcp-compute-persistent-disk-csi-driver-e2e-windows-2022 |
/assgn @mattcary |
return a4HyperdiskLimit, nil | ||
} | ||
} | ||
if strings.HasPrefix(machineType, "x4-") { |
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.
Why moved this outside? Because if it is inside the machineType is x4 or a4 it does not need to go through the loop. But on the other hand it does not matter so much in the real world
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.
You're right! Moving these checks outside was what I though could be a performance optimisation.
Since x4/a4 machine types don't match any of the gen4 prefixes (c4a-, c4-, n4-, c4d-), keeping them inside the loop means we'd unnecessarily check them 4 times (once per iteration) before ultimately returning the correct limit.
By moving them outside:
- gen4 machines: Exit early when matched, never reaching these checks
- Other machines: Only check x4/a4 once at the end
This reduces string comparisons from up to 12 (4 iterations × 3 checks) to at most 6 (4 gen4 + x4 + a4).
I also considered that x4 and a4 may be less frequently used than the ones in the loop. I will put it back if you think it would be better or not the subject of the current PR (would be understandable).
…n4 machines Signed-off-by: Loïc Yavercovski <[email protected]>
6df040f
to
d1ae089
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arsiesys, sunnylovestiramisu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-1.20 |
@sunnylovestiramisu: #2127 failed to apply on top of branch "release-1.20":
In response to this:
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. |
…imits fix: update Hyperdisk attach limits to match GCP documentation for Gen4 machines
…imits fix: update Hyperdisk attach limits to match GCP documentation for Gen4 machines
What type of PR is this?
What this PR does / why we need it:
This PR updates the Hyperdisk balanced attach limits to align with the latest GCP documentation for Gen4 machine families (
C4
,C4D
,N4
, andC4A
).Previously, a single generic map was used, which did not reflect the correct per-family attach limits for newer machine types.
Key changes:
C4
,C4D
,N4
, andC4A
machines.This fixes inaccurate attach limits for Gen4 machines, ensuring compliance with the [official GCP guidance](https://cloud.google.com/compute/docs/general-purpose-machines).
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: