SPLAT-2167: Added dedicated host support for AWS#374
SPLAT-2167: Added dedicated host support for AWS#374vr4manta wants to merge 6 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds AWS dedicated-host placement support across CAPA↔MAPI conversions, extends fuzz and unit tests for placement scenarios, re-enables host field diffing in infra comparisons, bumps several module deps, and adds Changes
Sequence Diagram(s)sequenceDiagram
participant CAPA as CAPA AWSMachine
participant C2M as capi2mapi.converter
participant MAPI as MAPI ProviderConfig/Placement
participant M2C as mapi2capi.converter
participant Controller as machineset/machinesync
CAPA->>C2M: provide AWSMachineSpec (Tenancy, HostAffinity, HostID, DynamicHostAllocation)
C2M->>MAPI: convert to ProviderConfig.Placement & ProviderStatus.DedicatedHost
MAPI->>M2C: expose ProviderConfig.Placement
M2C->>CAPA: convert Placement back to AWSMachineSpec/Status
Controller->>MAPI: compute diffs (includes hostID/hostAffinity)
Controller->>Controller: act on drift/updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
openshift-merge-robot
added
the
needs-rebase
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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-merge-robot
removed
the
needs-rebase
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
6355141 to
7ba97c3
Compare
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
dc53d82 to
e5cbce2
Compare
|
/test all |
|
/test all |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
ed41da7 to
320eac0
Compare
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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
bot
removed
the
do-not-merge/work-in-progress
|
@huali9 yes we need an upstream release for these: and then a upstream -> downstream sync |
|
/test e2e-aws-capi-techpreview |
|
I'm looking into the e2e failure to see what is up |
|
The error is caused by inconsistent between MAPI and CAPI. I already raised a bug https://issues.redhat.com/browse/OCPBUGS-73821 for this before. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
/test e2e-aws-capi-techpreview |
|
/test e2e-aws-capi-techpreview |
|
Fixed e2e. A few minor issues found in conversion logic (both ways -> capi2mapi and mapi2capi). Now manually running some of the tests from OCPBUGS-73821. Some tests may not be completely valid, but making sure behavior is same on both side. |
|
/test regression-clusterinfra-aws-ipi-techpreview-capi |
|
Thank you for addressing the e2e issues. I'm currently testing the feature. Given the scope of the changes, I'll need some time to perform a full retest of the functionality to ensure everything works as expected. However, if you prefer to merge the PR sooner, I can add the verified label now, as regression tests (e2e-aws-capi-techpreview and regression-clusterinfra-aws-ipi-techpreview-capi) have passed successfully. Please let me know which option works best for you. |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test e2e-aws-ovn e2e-aws-ovn-serial-1of2 e2e-aws-ovn-serial-2of2 e2e-aws-ovn-techpreview e2e-aws-ovn-techpreview-upgrade |
|
@vr4manta: 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. |
|
/retest |
@huali9 I am fine waiting on the finishing of the tests as long as it doesn't take too long. I may have to rebase again and rerun all tests if it takes too long and I'll be out tomorrow and Friday. Thanks for taking a look and for all the feedback! |
|
Thank you! @vr4manta I'll aim to complete the tests and updates by the end of this week. Thanks for your patience and feedback! |
|
I've completed testing/retesting this feature for both CAPI and MAPI. However, I found four issues related to dynamicHostAllocation (3 for CAPI and 1 for CPMS):
It appears that when CAPI tries to delete a dedicated host created by MAPI, it encounters a nil pointer error. I believe we should support cross-API operations (MAPI create → CAPI delete, and vice versa) since we already support MAPI2CAPI and CAPI2MAPI conversions. On the AWS console, the instance Terminated, but the dedicated host remains Available. Even after destroying the cluster, the dedicated host is still Available and must be manually released. Destroy job failed (I'm not sure the reason), some log shows:
I think the installer destroy process should support dedicated host cleanup as a safety net.
Syncing to MAPI fails with: However, the equivalent MAPI configuration works fine: I'm not sure why this sync error occurs. But this validation seems incorrect and should be removed, as the CAPI configuration is valid and also works in MAPI.
MAO logs show the host was allocated but not released: I've also updated the test cases with more detailed information: @vr4manta Could you please help investigate these issues? Thanks! |
10 participants
SPLAT-2167
Changes
ec2:DescribeInstanceTypesto cluster api credentials requestDependencies
Notes
There seems to be a required permission that was missing for dedicated host support. When running these changes, a warning event was observed in the cluster capi operator namespace.
Summary by CodeRabbit
New Features
Security / Permissions
Tests
Bug Fixes
Chores