OCPBUGS-99759: fix(nodepool): add CAPI autoscaler capacity annotations for scale-from-zero - #9110
OCPBUGS-99759: fix(nodepool): add CAPI autoscaler capacity annotations for scale-from-zero#9110judexzhu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@judexzhu: This pull request references Jira Issue OCPBUGS-99759, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe scale-from-zero workaround now manages Cluster Autoscaler CPU, memory, GPU-count, and GPU-type annotations alongside legacy machine annotations. It removes both formats when native capacity exists and populates or clears both when capacity is inferred. Unit and end-to-end tests cover AWS and Azure CPU-only and GPU scenarios. Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: judexzhu 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 |
openshift-ci
Bot
added
area/hypershift-operator
1950f62 to
17c88e9
Compare
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@judexzhu: This pull request references Jira Issue OCPBUGS-99759, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
![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
🧹 Nitpick comments (1)
test/e2e/autoscaling_test.go (1)
804-818: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate annotation values, not only key presence.
An empty or malformed CPU/memory annotation currently satisfies this predicate, even though the CAPI provider later parses these values as resource quantities and cannot build usable capacity. Parse the CAPI values, require non-zero quantities, and verify the legacy values remain consistent. (raw.githubusercontent.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/autoscaling_test.go` around lines 804 - 818, Update the annotation fallback predicate around the CAPI and legacy capacity keys to validate values rather than only checking key presence. Parse the CAPI CPU and memory annotations as resource quantities, reject missing, malformed, or zero values, and verify the legacy vCPU and memoryMb annotations represent consistent capacity before returning success; preserve the existing failure messages or equivalent context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/scale_from_zero.go`:
- Around line 134-140: Add the CAPI GPU type annotation alongside the count
annotation in the GPU handling within the scale-from-zero logic, using the
instance GPU resource name such as nvidia.com/gpu. Remove this annotation in
both cleanup paths, and extend the related tests to verify it is present for GPU
instances and absent for non-GPU instances.
---
Nitpick comments:
In `@test/e2e/autoscaling_test.go`:
- Around line 804-818: Update the annotation fallback predicate around the CAPI
and legacy capacity keys to validate values rather than only checking key
presence. Parse the CAPI CPU and memory annotations as resource quantities,
reject missing, malformed, or zero values, and verify the legacy vCPU and
memoryMb annotations represent consistent capacity before returning success;
preserve the existing failure messages or equivalent context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7434075a-e927-4bcb-b795-57c680689d29
📒 Files selected for processing (3)
hypershift-operator/controllers/nodepool/scale_from_zero.gohypershift-operator/controllers/nodepool/scale_from_zero_test.gotest/e2e/autoscaling_test.go
|
@judexzhu: This pull request references Jira Issue OCPBUGS-99759, which is valid. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
judexzhu
changed the title
openshift-ci-robot
removed
jira/severity-important
|
@judexzhu: No Jira issue is referenced in the title of this pull request. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/valid-bug
judexzhu
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9110 +/- ##
==========================================
+ Coverage 44.51% 44.56% +0.05%
==========================================
Files 774 774
Lines 96997 97009 +12
==========================================
+ Hits 43179 43234 +55
+ Misses 50830 50783 -47
- Partials 2988 2992 +4
... and 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira refresh |
|
@judexzhu: No Jira issue is referenced in the title of this pull request. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
judexzhu
changed the title
openshift-ci-robot
added
jira/severity-important
|
@judexzhu: This pull request references Jira Issue OCPBUGS-99759, which is valid. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@judexzhu: This pull request references Jira Issue OCPBUGS-99759, which is valid. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
…m-zero HyperShift deploys cluster-autoscaler with --cloud-provider=clusterapi, which reads capacity.cluster-autoscaler.kubernetes.io/* annotation keys. The scale-from-zero workaround only wrote machine.openshift.io/* keys (Machine API provider namespace), which the CAPI provider ignores. Add correct CAPI annotation keys alongside existing legacy keys: - capacity.cluster-autoscaler.kubernetes.io/cpu - capacity.cluster-autoscaler.kubernetes.io/memory (with MiB suffix) - capacity.cluster-autoscaler.kubernetes.io/gpu-count Legacy machine.openshift.io/* keys are preserved for backward compatibility and will be deprecated in a follow-up. Bug: https://issues.redhat.com/browse/OCPBUGS-99759
17c88e9 to
5a6f6f0
Compare
elmiko
left a comment
elmiko
left a comment
There was a problem hiding this comment.
i'm not overly familiar with this code, but using both annotations seems like a good update to me. the cluster-autoscaler at version <=4.22 should recognize both annotations, but after 4.23/5.0 it will not.
/lgtm
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
@judexzhu: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
Summary
--cloud-provider=clusterapi, which readscapacity.cluster-autoscaler.kubernetes.io/*annotation keysmachine.openshift.io/*keys (Machine API provider namespace), which the CAPI provider silently ignoresDesign Decision: Co-existing Keys
Legacy
machine.openshift.io/*keys are preserved alongside newcapacity.cluster-autoscaler.kubernetes.io/*keys rather than replaced. Rationale:Future deprecation: Legacy
machine.openshift.io/*keys should be removed in a follow-up once we confirm no consumers depend on them. The constants are already grouped and commented as "Legacy" to make this straightforward.Related
AWSMachineTemplate.Status.Capacitynever populated (separate root cause — missing ownerReferences). Together with this bug, scale-from-zero had no working capacity signal.Changes
scale_from_zero.gocapiCPUKey,capiMemoryKey,capiGPUCountKeyconstants; set both key sets; memory usesMiBsuffix for CAPI; cleanup covers bothscale_from_zero_test.goautoscaling_test.goTest plan
go test ./hypershift-operator/controllers/nodepool/...)golangci-lint run)Bug: https://issues.redhat.com/browse/OCPBUGS-99759
🤖 Generated with Claude Code
Summary by CodeRabbit