CNTRLPLANE-3608: Add nested virtualization support for AWS EC2 NodePools - #8681
Open
jhjaggars wants to merge 11 commits into
Open
CNTRLPLANE-3608: Add nested virtualization support for AWS EC2 NodePools#8681jhjaggars wants to merge 11 commits into
jhjaggars wants to merge 11 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jhjaggars: This pull request explicitly references no 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds an optional CpuOptions field to AWSNodePoolPlatform with a NestedVirtualization setting (enabled/disabled). The nodepool controller now maps that value into AWSMachineTemplateSpec.Template.Spec.CPUOptions.NestedVirtualization via applyAWSMachineOptions. Tests were added to verify JSON compatibility and controller behavior. go.mod dependency versions were bumped and a small CLI placeholder hunk appears. Sequence Diagram(s)sequenceDiagram
participant NodePool
participant NodePoolController
participant AWSMachineTemplateSpec
NodePool->>NodePoolController: spec.platform.aws.cpuOptions.nestedVirtualization
NodePoolController->>AWSMachineTemplateSpec: set template.spec.CPUOptions.NestedVirtualization
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8681 +/- ##
==========================================
+ Coverage 45.97% 45.98% +0.01%
==========================================
Files 781 781
Lines 98056 98082 +26
==========================================
+ Hits 45079 45102 +23
- Misses 49905 49908 +3
Partials 3072 3072
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@cmd/cluster/aws/create.go`:
- Around line 410-414: Validate o.NestedVirtualization at the CLI boundary by
checking its value against the documented allow-list and return an error
immediately if it's not allowed; then compute the effective instance type (use
o.InstanceType if set else the code's default m5.large/m6g.large) and verify
that this instance type's family is one of the supported families for nested
virtualization, returning a clear validation error if incompatible; update the
block that assigns nodePool.Spec.Platform.AWS.CpuOptions (where
o.NestedVirtualization is used) to perform both checks and fail fast instead of
setting an invalid value.
In `@cmd/nodepool/aws/create.go`:
- Around line 108-109: Add early validation for the nested virtualization flag
before the NodePool spec is written. Implement allow-list based validation that
checks two things: first, verify that the nested virtualization value is only
"enabled" or "disabled" (reject any arbitrary values), and second, validate that
the selected instance family is compatible with nested virtualization (only C8i,
M8i, and R8i families are supported, so reject incompatible defaults like
m5.large and m6g.large). This validation should be added at the trust boundary
where the flag is processed and should reject invalid combinations before any
NodePool spec is created. Apply the same validation logic to the other affected
code sections around lines 176-180.
In `@support/controlplane-component/controlplane-component.go`:
- Around line 208-211: The deletion logic currently returns early based only on
the volatile field c.hasBeenApplied; update the delete path in the component's
delete handler (the method using c.hasBeenApplied) to avoid relying on in-memory
state by first checking persistent cluster state or resource markers
instead—e.g., query the API/server for the component resource or its
finalizer/annotation via the controller client (use the existing client/get
method used elsewhere) and only skip deletion when the resource truly does not
exist or when a persistent “never-delete” marker is present; remove the early
return that depends solely on c.hasBeenApplied and replace it with a
cluster-existence check and/or persistent state check before deciding not to
delete.
- Around line 186-190: The code marks c.hasBeenApplied = true regardless of
whether c.update(cpContext) returned an error; change the logic so
c.hasBeenApplied is set only when the update succeeded: call reconcilationError
= c.update(cpContext) and then set c.hasBeenApplied = true only if
reconcilationError == nil (i.e., after a successful return from c.update),
leaving it false when c.update fails so downstream delete/cleanup behavior
remains correct.
In `@support/controlplane-component/generic-adapter.go`:
- Around line 54-57: The short-circuit using the process-local flag
ga.hasBeenApplied causes deletion to be skipped after a controller restart;
remove that reliance and instead detect resource existence/ownership at runtime
before skipping deletion: in the delete path (where ga.hasBeenApplied is
checked) call the appropriate lookup/delete logic (e.g., use the adapter's
client or ga.get/ga.getResource method) to query the cluster for the resource or
its ownership/annotation, attempt the deletion if the resource is present, and
only ignore errors that indicate "not found"; keep the operation idempotent and
remove the ga.hasBeenApplied-only return so cleanup runs across restarts.
- Around line 21-22: The hasBeenApplied boolean on genericAdapter is mutated on
a value copy (so updates are lost) and is only in-memory (lost on restart);
change manifestsAdapters from map[string]genericAdapter to
map[string]*genericAdapter and update update() and any retrieval sites to use
pointer semantics so genericAdapter.reconcile() sets the real struct's
hasBeenApplied; also remove sole reliance on the in-memory flag by deriving
applied state from the cluster (e.g., check the target resource
existence/owner/annotation) or persist the flag into the control-plane
workload's Status (add a field in the CR status and read/write it in
update()/reconcile()) so the predicate-based cleanup survives restarts.
🪄 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: fea52243-7df4-4f72-a4be-ab67d3321aaa
⛔ Files ignored due to path filters (167)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/awsnodepoolplatform.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/cpuoptions.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsclusters.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsclustertemplates.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachinepools.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachines.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdgo.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go-v2/service/ec2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AttachVolume.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_CreateSecondaryNetwork.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_CreateSecondarySubnet.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DeleteSecondaryNetwork.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DeleteSecondarySubnet.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondaryInterfaces.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondaryNetworks.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondarySubnets.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DetachVolume.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceCpuOptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceMetadataDefaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceMetadataOptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_RunInstances.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_SearchTransitGatewayRoutes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/deserializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/generated.jsonis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/go_module_metadata.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/serializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/types/enums.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/validators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateAccessPolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateEncryptionConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateIdentityProviderConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreatePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeletePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeregisterCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddonConfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddonVersions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeClusterVersions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeIdentityProviderConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeInsight.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeInsightsRefresh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeUpdate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DisassociateAccessPolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAccessEntries.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAccessPolicies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAddons.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAssociatedAccessPolicies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListCapabilities.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListClusters.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListEksAnywhereSubscriptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListFargateProfiles.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListIdentityProviderConfigs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListInsights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListNodegroups.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListPodIdentityAssociations.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListTagsForResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListUpdates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_RegisterCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_StartInsightsRefresh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_TagResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UntagResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateClusterConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateClusterVersion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateNodegroupConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateNodegroupVersion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdatePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/auth.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/deserializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/endpoints.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/generated.jsonis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/go_module_metadata.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/internal/endpoints/endpoints.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/serializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/types/enums.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/types/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/validators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-online/ocm-common/pkg/resource/validations/kms_arn_regex_validation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/golang.org/x/crypto/blake2b/blake2b.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2bAVX2_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2bAVX2_amd64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2b_amd64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2b_generic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2b_ref.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/blake2x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blake2b/register.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1/awscluster_conversion.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1/awsmachine_conversion.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1/network_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1/types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1/zz_generated.conversion.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awscluster_defaults.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awscluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awscluster_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsclustercontrolleridentity_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsclusterroleidentity_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsclusterstaticidentity_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsclustertemplate_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachinetemplate_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/bastion.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/defaults.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/network_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/rosa/api/v1beta2/rosacontrolplane_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/rosa/api/v1beta2/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/awsfargateprofile_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/awsmachinepool_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/awsmanagedmachinepool_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/rosamachinepool_defaults.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/rosamachinepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/rosamachinepool_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/rosanetwork_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/rosaroleconfig_webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/validation.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/sigs.k8s.io/cluster-api-provider-aws/v2/pkg/eks/eks.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/pkg/hash/base36.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/aws.gocmd/cluster/aws/create.gocmd/nodepool/aws/create.gogo.modhypershift-operator/controllers/nodepool/aws.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/generic-adapter.go
jhjaggars
changed the title
|
@jhjaggars: This pull request references CNTRLPLANE-3608 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 epic to target the "5.0.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. |
jhjaggars
force-pushed
the
virt-on-new-aws-nodes
branch
from
June 10, 2026 14:57
3668271 to
3657529
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/aws.go (1)
76-76: ⚡ Quick winRemove redundant
omitemptyfrom JSON tag for consistency.The
omitemptytag doesn't omit empty structs (only pointers, maps, slices, strings, numbers). For value-type struct fields,omitzerois the correct tag. Line 126 demonstrates the correct pattern withSpotOptions:Spot SpotOptions `json:"spot,omitzero"`Proposed fix
- CpuOptions CpuOptions `json:"cpuOptions,omitzero,omitempty"` + CpuOptions CpuOptions `json:"cpuOptions,omitzero"`🤖 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 `@api/hypershift/v1beta1/aws.go` at line 76, The JSON tag for the CpuOptions field uses both "omitzero" and "omitempty" redundantly; update the CpuOptions declaration (CpuOptions CpuOptions `json:"cpuOptions,omitzero,omitempty"`) to remove "omitempty" so it matches the pattern used for Spot (e.g., `json:"spot,omitzero"`), leaving only "omitzero" to correctly omit zero-value structs.
🤖 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.
Nitpick comments:
In `@api/hypershift/v1beta1/aws.go`:
- Line 76: The JSON tag for the CpuOptions field uses both "omitzero" and
"omitempty" redundantly; update the CpuOptions declaration (CpuOptions
CpuOptions `json:"cpuOptions,omitzero,omitempty"`) to remove "omitempty" so it
matches the pattern used for Spot (e.g., `json:"spot,omitzero"`), leaving only
"omitzero" to correctly omit zero-value structs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e6025936-e97d-42d1-81ad-6d25745be666
⛔ Files ignored due to path filters (99)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/awsnodepoolplatform.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/cpuoptions.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsclusters.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsclustertemplates.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachinepools.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachines.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/cluster-api-provider-aws/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdgo.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go-v2/service/ec2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AttachVolume.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_CreateSecondaryNetwork.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_CreateSecondarySubnet.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DeleteSecondaryNetwork.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DeleteSecondarySubnet.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondaryInterfaces.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondaryNetworks.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeSecondarySubnets.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DetachVolume.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceCpuOptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceMetadataDefaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ModifyInstanceMetadataOptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_RunInstances.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_SearchTransitGatewayRoutes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/deserializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/generated.jsonis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/go_module_metadata.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/serializers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/types/enums.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/ec2/validators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateAccessPolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateEncryptionConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_AssociateIdentityProviderConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreateNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_CreatePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeleteNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeletePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DeregisterCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAccessEntry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddon.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddonConfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeAddonVersions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeCapability.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeClusterVersions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeEksAnywhereSubscription.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeFargateProfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeIdentityProviderConfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeInsight.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeInsightsRefresh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeNodegroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribePodIdentityAssociation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DescribeUpdate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_DisassociateAccessPolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAccessEntries.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAccessPolicies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAddons.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListAssociatedAccessPolicies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListCapabilities.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListClusters.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListEksAnywhereSubscriptions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListFargateProfiles.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListIdentityProviderConfigs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListInsights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListNodegroups.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListPodIdentityAssociations.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListTagsForResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_ListUpdates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_RegisterCluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_StartInsightsRefresh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_TagResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UntagResource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/eks/api_op_UpdateAccessEntry.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/aws.gocmd/cluster/aws/create.gocmd/nodepool/aws/create.gogo.modhypershift-operator/controllers/nodepool/aws.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/controllers/nodepool/aws.go
- cmd/nodepool/aws/create.go
- go.mod
- cmd/cluster/aws/create.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/nodepool_types_test.go (1)
19-24: ⚡ Quick winUse an isolated N-1 subnet fixture type in the compatibility struct.
awsNodePoolPlatformNMinus1currently embedsAWSResourceReferencefrom the current API. That can mask nested-field compatibility regressions because the N-1 fixture evolves with current code. Prefer a local N-1 subnet struct (e.g., justid) to pin the previous wire contract independently.Suggested change
+type awsResourceReferenceNMinus1 struct { + ID *string `json:"id,omitempty"` +} + type awsNodePoolPlatformNMinus1 struct { // instanceType is the EC2 instance type. InstanceType string `json:"instanceType"` //nolint:kubeapilinter // test-only N-1 compat struct // subnet is the subnet reference. - Subnet AWSResourceReference `json:"subnet"` //nolint:kubeapilinter // test-only N-1 compat struct + Subnet awsResourceReferenceNMinus1 `json:"subnet"` //nolint:kubeapilinter // test-only N-1 compat struct }As per coding guidelines, “When modifying API types, add serialization compatibility tests that define a struct matching the previous version of the type.”
🤖 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 `@api/hypershift/v1beta1/nodepool_types_test.go` around lines 19 - 24, The compatibility struct awsNodePoolPlatformNMinus1 currently reuses the live AWSResourceReference which can drift; replace the embedded AWSResourceReference in awsNodePoolPlatformNMinus1 with a local N-1 subnet fixture struct (e.g., type with just `ID string` tagged `json:"id"`) and change the Subnet field's type to that local struct so the test pins the previous wire contract independently (update any test serializers/deserializers referencing awsNodePoolPlatformNMinus1/Subnet accordingly).Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@api/hypershift/v1beta1/nodepool_types_test.go`:
- Around line 19-24: The compatibility struct awsNodePoolPlatformNMinus1
currently reuses the live AWSResourceReference which can drift; replace the
embedded AWSResourceReference in awsNodePoolPlatformNMinus1 with a local N-1
subnet fixture struct (e.g., type with just `ID string` tagged `json:"id"`) and
change the Subnet field's type to that local struct so the test pins the
previous wire contract independently (update any test serializers/deserializers
referencing awsNodePoolPlatformNMinus1/Subnet accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7c57b3b1-4e5a-4c38-9013-c81ecaf79699
⛔ Files ignored due to path filters (2)
cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/stable.nodepools.aws.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (3)
api/hypershift/v1beta1/nodepool_types_test.gocmd/cluster/aws/create.gohypershift-operator/controllers/nodepool/aws_test.go
💤 Files with no reviewable changes (1)
- cmd/cluster/aws/create.go
jhjaggars
force-pushed
the
virt-on-new-aws-nodes
branch
from
June 10, 2026 18:33
825b2ee to
c16401a
Compare
jhjaggars
force-pushed
the
virt-on-new-aws-nodes
branch
from
July 7, 2026 16:10
f22e45f to
f453ef0
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-ci-robot
removed
the
verified
jhjaggars
force-pushed
the
virt-on-new-aws-nodes
branch
from
August 13, 2026 13:29
6489b5f to
7f5671b
Compare
muraee
reviewed
Aug 14, 2026
Contributor
Author
|
Thanks all for the review — pushed 937c622 addressing @muraee's request for validation on nested virtualization instance-type compatibility (a new check in @clebs re: commit history cleanup — agreed it'd be cleaner, but since this will be squash-merged the intermediate history won't matter in the final commit. Leaving as-is unless there's a reason to tidy it before merge. All other review threads addressed and resolved. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jhjaggars 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 |
Contributor
Author
|
/retest ci/prow/verify |
Contributor
Author
|
/test verify |
yuqi-zhang
reviewed
Aug 19, 2026
| type CPUOptions struct { | ||
| // nestedVirtualizationPolicy indicates whether to enable nested virtualization on the instance. | ||
| // Supported on C8i, M8i, and R8i instance families. | ||
| // When omitted, nested virtualization is not enabled (AWS default behavior). |
Contributor
There was a problem hiding this comment.
One last godoc change from the API perspective: enums should document possibly values and their usage, for example, something like
// Valid values are "Enabled" and "Disabled".
// When set to "Enabled", nested virtualization is enabled on the instance.
// When set to "Disabled", nested virtualization is explicitly disabled on the instance.
…tform Add CpuOptions struct with NestedVirtualization field to AWSNodePoolPlatform for configuring nested virtualization on EC2 instances (C8i, M8i, R8i families). - Add CpuOptions type with omitzero value semantics - Add MinProperties=1 validation marker - Regenerate deepcopy and featuregated CRD manifests Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
…ool creation Add --nested-virtualization flag to hypershift create cluster aws and hypershift create nodepool aws commands. When set, configures CpuOptions.NestedVirtualization on the NodePool's AWS platform spec. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
…nested virtualization Propagate CpuOptions.NestedVirtualization from NodePool spec to CAPA AWSMachineTemplateSpec.CPUOptions during AWSMachineTemplate reconciliation. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Regenerate aggregated-docs.md and api.md to include the new CpuOptions type and NestedVirtualization field documentation. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Remove the AWS create-time nested virtualization flags so this option is only set through rendered manifests where the instance type can be reviewed. Add API compatibility, controller, and envtest coverage for CpuOptions so the new field is protected against regressions. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
…ller Replace raw string with typed NestedVirtualizationPolicy constants (enabled/disabled) for CpuOptions.NestedVirtualization. Add nil-guard for AWS platform in applyAWSMachineOptions. Expand test coverage with disabled and nil-platform cases in unit tests and CEL envtest suite. Regenerate client apply config and API docs.
- Remove redundant omitempty from CpuOptions JSON tag (omitzero suffices for zero-value struct omission, matching SpotOptions pattern) - Add descriptive failure messages to all Gomega assertions in TestApplyAWSMachineOptions for easier debugging
…onventions Rename CpuOptions to CPUOptions per Go acronym convention. Rename NestedVirtualization field to NestedVirtualizationPolicy to match the type name. Change enum values from lowercase enabled/disabled to PascalCase Enabled/Disabled per OpenShift API convention. Add explicit mapping in the controller between HyperShift and CAPA enum values. Add godoc comments documenting default behavior when cpuOptions is omitted and noting that reverting requires removing the entire cpuOptions field. Split applyAWSMachineOptions back into applyAWSPlacementOptions and a new applyAWSCPUOptions for better separation of concerns. Regenerate CRDs, deepcopy, client, and API docs. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
… instance families Add a check to validateAWSPlatformConfig (surfaced via the ValidPlatformConfig NodePool condition) that rejects cpuOptions.nestedVirtualizationPolicy=Enabled when instanceType isn't a C8i, M8i, or R8i family (including their -flex variants), matching AWS's documented CPU options support matrix. This fails fast with a clear condition message instead of only failing later at the AWS API when the instance is actually launched. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Fix minor gofmt column-alignment drift in the CPUOptions applyconfiguration struct and pick up unrelated API doc updates (Azure/monitoring/encryption types) that had fallen behind main since this branch was last rebased. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
jhjaggars
force-pushed
the
virt-on-new-aws-nodes
branch
from
August 21, 2026 16:30
937c622 to
9a1adc2
Compare
Member
|
/lgtm |
Contributor
|
Scheduling tests matching the |
Member
|
/test e2e-v2-azure-self-managed |
Contributor
|
@jhjaggars: The following test 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for enabling nested virtualization on AWS EC2 instances (C8i, M8i, R8i families) through the NodePool API. This allows running OpenShift Virtualization (KubeVirt) workloads on AWS-hosted clusters.
CpuOptionsstruct withNestedVirtualizationfield toAWSNodePoolPlatformNestedVirtualizationsupport inCPUOptions(PR kubernetes-sigs/cluster-api-provider-aws#5874)--nested-virtualizationflag tohypershift create cluster awsandhypershift create nodepool awsCpuOptions.NestedVirtualizationfrom NodePool spec to CAPAAWSMachineTemplateSpec.CPUOptions--nested-virtualization enabled --instance-type c8i.2xlargecpuOptions.nestedVirtualization: enabledpropagates through: NodePool → AWSMachineTemplate → AWSMachine → EC2 instanceCpuOptions.NestedVirtualization: enabledvia AWS CLIvmxCPU flag present on guest nodes via/proc/cpuinfoN/A — new feature
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Chores