SPLAT-2167: Added dedicated host support for AWS #374
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. |
📝 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 and 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
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.5.0)/bin/bash: line 1: golangci-lint: command not found 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. |
| - ec2:DescribeDhcpOptions | ||
| - ec2:DescribeImages | ||
| - ec2:DescribeInstances | ||
| - ec2:DescribeInstanceTypes |
There was a problem hiding this comment.
I believe it would be nice to add this change to a new commit referencing the reason of adding it (upstream PR, jira feature, etc).
this would be used to document in the future and/or in managed services to eventually update the managed IAM.
There was a problem hiding this comment.
Sure! There is a warning message that is caused by this missing. I am not 100% sure this feature introduced it, but I am happy to put into a separate commit.
pkg/conversion/capi2mapi/aws.go
Outdated
| mapaProviderConfig.HostPlacement = &mapiv1beta1.HostPlacement{ | ||
| Affinity: ptr.To(mapiv1beta1.HostAffinityDedicatedHost), | ||
| DedicatedHost: &mapiv1beta1.DedicatedHost{ | ||
| ID: *m.awsMachine.Spec.HostID, |
There was a problem hiding this comment.
do we need to check HostID isnt nil to be safe or is there already a previous validation of this scenario?
There was a problem hiding this comment.
good question. lots of these CRs have checks in their respective webhooks / CRDs. I can add a check here as well. I was on the fence about it.
|
@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
|
Thank you @vr4manta , I checked @damdo 's pr kubernetes-sigs/cluster-api-provider-aws#5801 seems only set |
do we log the resulting values? I think its creating "" versions of the field which makes it not != null. I can add a check to ignore "" as well in that case. I'll also take a deeper look to see what else we can do to fix the conversions. |
|
Thank you @vr4manta I created a bug https://issues.redhat.com/browse/OCPBUGS-73821 to better trace the issue and attached the details on that. |
|
@vr4manta I noticed you submitted a PR to modify MAPI. However, I'm wondering if we should modify CAPI instead? Because configuring I also tested this directly with AWS, and setting only Affinity=default is not allowed: This confirms that Affinity requires Tenancy=host to be set as well. We should align with AWS's behavior, shouldn't we? |
I agree this should be fixed upstream. The other PR is to make the two match since its broke now. It will take sprints / months to get this backported into the openshift/cluster-api-provider-aws. Once fixed, we can make match so there isn't breakage this this lasting during development. But I'll discuss with @damdo what options we have and i'll update this PR with the results. |
|
@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.22.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. |
|
Created upstream PR to fix CAPA to match AWS cli. |
|
since this PR is on hold, I am updating it to support dynamic DH support since we are waiting on upstream CAPA fixes landing into OCP fork. |
|
New changes are detected. LGTM label has been removed. |
|
@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.22.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. |
![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
🤖 Fix all issues with AI agents
In `@pkg/conversion/mapi2capi/aws_fuzz_test.go`:
- Around line 187-252: The provider-config filler uses c.FillNoCustom(ps) in
fuzzProviderConfig which bypasses custom funcs so fuzzPlacement never runs for
AWSMachineProviderConfig; call f.fuzzPlacement(&ps.Placement, c) from
fuzzProviderConfig (before you reset ps.Region) to explicitly apply placement
fuzzing for the Placement field (ensure you reference fuzzPlacement,
fuzzProviderConfig, FillNoCustom, and ps.Placement when making the change).
|
@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.22.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.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/conversion/capi2mapi/aws.go`:
- Around line 283-326: In convertHostAffinityHost, when validating a provided
HostID (hasHostID branch) replace the incorrect error symbol errHostIDRequired
with errHostIDInvalidFormat so the field.Invalid call on
fldPath.Child("dedicatedHost").Child("id") returns the accurate "invalid format"
error (this mirrors the usage in convertHostAffinityDefault); update the
field.Invalid invocation to reference errHostIDInvalidFormat for the HostID
format check.
🧹 Nitpick comments (1)
pkg/conversion/mapi2capi/aws.go (1)
463-469: Potential side effect: sorting modifies the original slice in the unmarshaled spec.The
tagsvariable is assigned directly from the spec's slice, sosort.Slice(tags, ...)mutates the originalspec.Placement.Host.DedicatedHost.DynamicHostAllocation.Tagsslice. While this achieves deterministic ordering before returning, it modifies the input data as a side effect. If callers expect the returned spec to be unchanged elsewhere, this could cause subtle issues.Consider whether this is intentional. If so, a brief comment documenting this behavior would help future readers.
|
@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.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/conversion/capi2mapi/aws.go`:
- Around line 328-357: convertHostAffinityDefault currently returns nil on an
invalid HostID but returns a partial host when DynamicHostAllocation is invalid;
make the behavior consistent by returning nil whenever there are any validation
errors. In practice, keep the field.Invalid calls for HostID and
DynamicHostAllocation as-is (references: spec.HostID,
awsDedicatedHostNamePattern, spec.DynamicHostAllocation,
fldPath.Child("dedicatedHost").Child("id"),
fldPath.Child("dynamicHostAllocation")), then before the final return check if
errorList is non-empty and if so return nil, errorList; otherwise return host,
errorList.
🧹 Nitpick comments (1)
pkg/conversion/capi2mapi/aws.go (1)
59-65: Consider using the new tenancy constants inconvertAWSTenancyToMAPI.The new
TenancyDefault,TenancyDedicated, andTenancyHostconstants (lines 60-64) duplicate the string literals used inconvertAWSTenancyToMAPI(lines 621-625). Using the constants would improve consistency and reduce the risk of typos.Suggested refactor
func convertAWSTenancyToMAPI(fldPath *field.Path, capiTenancy string) (mapiv1beta1.InstanceTenancy, *field.Error) { switch capiTenancy { - case "default": + case TenancyDefault: return mapiv1beta1.DefaultTenancy, nil - case "dedicated": + case TenancyDedicated: return mapiv1beta1.DedicatedTenancy, nil - case "host": + case TenancyHost: return mapiv1beta1.HostTenancy, nil case "": return "", nil default: return "", field.Invalid(fldPath, capiTenancy, errUnsupportedCAPATenancy) } }Also applies to: 619-632
|
@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.22.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: 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. |
|
@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.22.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. |
9 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
✏️ Tip: You can customize this high-level summary in your review settings.