Stop GCPManagedNodePool scaling being blocked by multiple factors#1657
Stop GCPManagedNodePool scaling being blocked by multiple factors#1657jonathanrainer wants to merge 5 commits into
GCPManagedNodePool scaling being blocked by multiple factors#1657Conversation
k8s-ci-robot
added
release-note
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jonathanrainer The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @jonathanrainer. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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
the
size/XL
jonathanrainer
force-pushed
the
fix/gcpmanagedmachinepool-scaling-blocked
branch
from
28c76df to
4b12c02
Compare
jonathanrainer
force-pushed
the
fix/gcpmanagedmachinepool-scaling-blocked
branch
from
4b12c02 to
1b58050
Compare
k8s-ci-robot
added
ok-to-test
jonathanrainer
force-pushed
the
fix/gcpmanagedmachinepool-scaling-blocked
branch
from
1b58050 to
0800f21
Compare
ConvertToSdkAutoscaling returns {Enabled: true} when passed nil, causing
checkDiffAndPrepareUpdateSize to unconditionally suppress manual scaling
for any node pool that has no Spec.Scaling block. Guard the check on
Spec.Scaling being non-nil so only explicitly-configured autoscaling
suppresses size updates.
ConvertToSdkLinuxNodeConfig returns a non-nil pointer even when passed nil, so comparing it against the nil value GKE returns in GetNodePool for unconfigured pools always produces a diff and triggers a spurious update. Guard the comparison on Spec.LinuxNodeConfig being non-nil.
GKE does not return NodeConfig.ResourceLabels in GetNodePool responses, so comparing desiredNodePool.ResourceLabels against the existing value always produces a diff (existing is always nil) and triggers a perpetual UpdateNodePool loop that blocks all other reconcile operations. Fix the comparison by reading labels from the node pool's instance template (properties.labels), which is the authoritative source GKE uses to stamp labels onto every node in the pool. This requires one additional Get call per reconcile cycle: InstanceGroupManagersClient.Get() to resolve the template URL, then InstanceTemplatesClient.Get() to read the template properties. A new InstanceTemplatesClient is added to the scope following the same pattern as InstanceGroupManagersClient. The comparison is a subset check — all desired labels must be present with the correct value; GKE-internal labels on the template are ignored to avoid spurious updates.
…starvation The original loop ran checks sequentially with an early return on the first positive result (config → autoscaling → size). Any persistent config diff would trigger a config update and return, meaning size and autoscaling checks never ran in that cycle. After the config update GKE enters RECONCILING state, causing the next cycle to return early too, so scaling changes via MachinePool.spec.replicas had no effect. Run all three checks unconditionally before applying any update, then apply in priority order: size (user-initiated scaling, most time-sensitive) → autoscaling config → general config. GKE still only allows one concurrent operation per node pool, so we apply one per cycle and requeue for any remaining work.
Table-driven tests for checkDiffAndPrepareUpdateConfig and checkDiffAndPrepareUpdateSize covering the cases fixed in the preceding commits: resource label sync via instance template (present, missing, unavailable), Kubernetes label diff, image type case-insensitive match, network tags, node locations, autoscaling suppression, nil Scaling guard, and regional cluster replica division.
jonathanrainer
force-pushed
the
fix/gcpmanagedmachinepool-scaling-blocked
branch
from
0800f21 to
f3e7bac
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jonathanrainer The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR tackles the 4 bugs that are listed out in #1656 which prevent manual
MachinePoolscaling from working with aGCPManagedNodePool. Namely:nilguard to the Autoscaling check - This means that nodepools that don't have an Autoscaling config won't return early, so manual scaling can occurnilguard toLinuxNodeConfig- This means thatLinuxNodeConfigis only compared when it's set, rather than comparing a non-nil empty struct generated byConvertToSdkLinuxNodeConfigto thenilthat GKE returnsAdditionalLabelsvia the InstanceTemplate not the NodePool - The NodePool object doesn't have a labels attribute on it, so instead of that we create an InstanceTemplate client, look up the template that's relevant and check the labels we're setting are correctly set there, and rely on GKE to propagate those into the nodes themselves.checkDiffAndPrepareUpdateSize,checkDiffAndPrepareUpdateAutoscaling,checkDiffAndPrepareUpdateConfig) before any update is applied - This means the current behaviour where updates are run sequentially and there's an early exit (and in turn causes size updates to be starved because of (3)), doesn't happen anymore. Updates are all checked and then run in priority order (size -> autoscaling -> general).Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1656
Special notes for your reviewer:
I've verified as much of this as I can on my own cluster, particularly the
additionalLabelschecks, but I also appreciate there's an alternative approach where you could fetch the Instances themselves and check, and I'm happy to pivot to that approach if we think that's demonstrably better.Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: