CORS-4507: aws: support edge machine pool management with ClusterAPI#10625
CORS-4507: aws: support edge machine pool management with ClusterAPI#10625tthvo wants to merge 5 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references CORS-4507 which is a valid jira issue. 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 (8)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughEnables ClusterAPI management for edge compute machine pools. ChangesEdge Compute ClusterAPI Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/label platform/aws |
openshift-ci
Bot
added
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pkg/asset/machines/aws/clusterapi_machinesets.go`:
- Around line 85-90: The code accesses the map in.Zones using the key az without
checking if the key exists. In Go, accessing a non-existent map key silently
returns a zero-value, causing the code to proceed with an empty zone object and
skipping the preferred instance type selection. Fix this by using the two-value
form of map access (zone, ok := in.Zones[az]) in the edge pool handling section
where in.Pool.Name equals types.MachinePoolEdgeRoleName, and add a check to fail
fast when the zone key is missing (when ok is false) rather than continuing with
zero-value defaults.
- Around line 85-105: Add unit tests for the edge pool logic in
ClusterAPIMachineSets. Create a new test file clusterapi_machinesets_test.go
with test cases that exercise the code path when in.Pool.Name equals
types.MachinePoolEdgeRoleName. The tests must verify that when
zone.PreferredInstanceType is set, it is correctly assigned to the instanceType
variable, that all four edge node labels (node-role.kubernetes.io/edge,
machine.openshift.io/zone-type, machine.openshift.io/zone-group,
machine.openshift.io/parent-zone-name) are properly set on nodeLabels, and that
a MachineTaint with Key node-role.kubernetes.io/edge and Effect NoSchedule is
appended to nodeTaints with Propagation set to
capi.MachineTaintPropagationAlways. Use fixture data that includes zone objects
with PreferredInstanceType populated to ensure the complete edge pool branch is
exercised.
In `@pkg/types/defaults/machinepools_test.go`:
- Around line 209-215: The test case at line 211 is named "edge compute with
management already set" but uses types.MachinePoolComputeRoleName when creating
the MachinePool, which duplicates the previous compute case and fails to verify
that explicit edge Management is preserved. Change the Name field in the pool
creation from types.MachinePoolComputeRoleName to types.MachinePoolEdgeRoleName
so the test properly validates the edge role case with management preservation.
🪄 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: 935d0e1b-5fcd-4768-b659-0d13b36cb9ff
📒 Files selected for processing (5)
pkg/asset/machines/aws/clusterapi_machinesets.gopkg/types/defaults/machinepools.gopkg/types/defaults/machinepools_test.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
💤 Files with no reviewable changes (2)
- pkg/types/validation/installconfig.go
- pkg/types/validation/installconfig_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/asset/machines/aws/clusterapi_machinesets_test.go`:
- Around line 262-275: The test case for edge pool instance type override sets
the zone's PreferredInstanceType to "m5.xlarge", which is the same as the base
AWS instance type, so the test cannot actually validate that the override logic
is working. Change the PreferredInstanceType value in the zone to a different
instance type (e.g., "m6i.xlarge") to ensure the assertion proves the override
behavior is correct. Apply this same fix wherever else this pattern appears in
the test file (as noted in the "Also applies to" comment).
🪄 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: 0b7e1456-8173-4123-b1f7-bdd3a29c77da
📒 Files selected for processing (6)
pkg/asset/machines/aws/clusterapi_machinesets.gopkg/asset/machines/aws/clusterapi_machinesets_test.gopkg/types/defaults/machinepools.gopkg/types/defaults/machinepools_test.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
💤 Files with no reviewable changes (2)
- pkg/types/validation/installconfig.go
- pkg/types/validation/installconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/types/defaults/machinepools.go
- pkg/types/defaults/machinepools_test.go
- pkg/asset/machines/aws/clusterapi_machinesets.go
|
/test e2e-aws-ovn-devpreview |
Note: we use the same feature gate ClusterAPIComputeInstall as worker compute pool.
Defaults the edge machine pool management to CAPI when the appropriate feature gate is enabled.
Edge compute pools require MachineTaintPropagation, which is now available after we bump CAPI to v1.12.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/test e2e-aws-ovn-devpreview |
|
@tthvo: 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. |
|
/hold cancel CAPI 1.12 is already running in-cluster. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aws-ovn-shared-vpc-edge-zones |
2 participants
Descriptions
This PR adds support for generating CAPI machinesets for edge machine pool in AWS. This work depends on node taints support in CAPI v1.12+ (see OCPCLOUD-2899).
For example, use the below install-config snippet to install (
us-east-1):Note: The installer already vendors CAPI
v1.12.8so we can generate the manifests without waiting on CCAPIO.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests