feat: add CPU options with nested virtualization and instance-type filtering#9043
feat: add CPU options with nested virtualization and instance-type filtering#9043windsornguyen wants to merge 3 commits intoaws:mainfrom
Conversation
- Add CPUOptions struct to EC2NodeClassSpec with coreCount, threadsPerCore, and nestedVirtualization fields - Integrate CPU options into launch template creation pipeline - Add comprehensive validation tests for CPU options - Add integration test for end-to-end CPU options verification - Add documentation and usage examples - Addresses issue aws#8966: Support enabling Nested Virtualization in CPU options
- Uncomment NestedVirtualization field in cpuOptions function - Remove comment lines indicating AWS SDK doesn't support it
…ltering Add CPUOptions struct to EC2NodeClassSpec with coreCount, threadsPerCore, and nestedVirtualization fields. Integrate into the launch template pipeline and add instance-type filtering based on ProcessorInfo.SupportedFeatures from DescribeInstanceTypes. Key changes: - CPUOptions API with CEL validation (nestedVirtualization mutually exclusive with coreCount/threadsPerCore) - New karpenter.k8s.aws/instance-nested-virtualization label populated from EC2 ProcessorInfo.SupportedFeatures - NestedVirtualizationFilter rejects instance types lacking the feature - UnsupportedOperation added to unfulfillable capacity error codes - cpuOptions() converter returns nil for empty struct to avoid sending empty CpuOptions to EC2 - RFC in designs/ Fixes aws#8966 Co-authored-by: Satya Alla <satya.alla@walmart.com>
|
Preview deployment ready! Preview URL: https://pr-9043.d18coufmbnnaag.amplifyapp.com Built from commit |
jmdeal
left a comment
jmdeal
left a comment
There was a problem hiding this comment.
I didn't go deep into the implementation since I had some high-level comments on the design. Let me know if you have any questions.
| CoreCount *int32 `json:"coreCount,omitempty"` | ||
| ThreadsPerCore *int32 `json:"threadsPerCore,omitempty"` |
There was a problem hiding this comment.
Is support for coreCount and threadsPerCore a requirement for this integration? I think there's room for integration with these features in Karpenter, but it adds additional design ambiguity that nested virtualization doesn't necessarily have.
A straightforward implementation of these two features would filter out instances which don't support the configured values (we should do this if we go forward with it). The downside of this approach is that it severely restricts the diversity of NodePools. We may want to introduce a way to dynamically scale or select these values based on the selected instance type that way we can retain NodePool diversity. Nested virtualization doesn't have the same concern because it's on or off. Additionally, you likely don't want or need to mix instance types with nested virtualization enabled in the same NodePool.
| A new well-known label `karpenter.k8s.aws/instance-nested-virtualization` is populated | ||
| from `ProcessorInfo.SupportedFeatures` during instance type resolution. Instance types | ||
| that report `nested-virtualization` in their supported features receive the label value | ||
| `"true"`. |
There was a problem hiding this comment.
If we include a label, I don't think we want to set it to true just because an instance is compatible with nested virtualization. It should only be true if the feature is actually enabled. You can imagine that application owners may configure their pods with a label selector like karpenter.k8s.aws/instance-nested-virtualization IN 'true' and expect it to land on a node with nested virtualization enabled, not just a node that's capable of having it enabled.
| CEL validation enforces that `nestedVirtualization: enabled` cannot be combined with | ||
| `coreCount` or `threadsPerCore` (EC2 rejects the combination). | ||
|
|
||
| ### Instance Type Label |
There was a problem hiding this comment.
I don't know if we want to include a label unless we use that label to drive the configuration of the instance. This would be consistent with other features, such as instance-tenancy. For that feature, the label can take on two values (default or dedicated) and each instance type is compatible with both. If a NodeClaim is compatible with both we'll use default, but if it's only compatible with dedicated we'll use dedicated. This behavior gives customers increased flexibility - they can have a lightly-constrained NodePool and use pods requirements to drive configuration, or they can enforce a setting via a NodePool requirement. At that point adding the configuration surface on the NodeClass is technically optional, but there is value in including it since the NodeClass is often considered a more privileged resource owned by cluster administrators.
These are the options in my order of preference. I heavily lean towards 1 or 2, though all three options have precedent in the project.
- No label, only NodeClass configuration (applications can select via NodePool / NodeClass labels). Label based selection can be added in the future based on presented real-world use-cases.
- Both label and NodeClass configuration
- No NodeClass configuration, only label-based configuration
| empty `CpuOptions` block in the API call). The `NestedVirtualization` string is cast to | ||
| the SDK enum type `ec2types.NestedVirtualizationSpecification`. | ||
|
|
||
| ### Error Handling |
There was a problem hiding this comment.
I agree with this approach since it ensures we'll always be able to make provisioning progress, but is this just defensive or are there existing gaps in the filter that you're aware of?
There was a problem hiding this comment.
Let's continue to use ginkgo for all tests.
2 participants
Fixes #8966
Description
Adds
cpuOptionstoEC2NodeClassSpecwithcoreCount,threadsPerCore, andnestedVirtualizationfields. Builds on the plumbing from #8971 (co-authored with @salla2) and adds three production-hardened fixes discovered while running this in a vendored snapshot:Instance-type filtering: New
NestedVirtualizationFilterthat rejects instance types lackingnested-virtualizationinProcessorInfo.SupportedFeaturesfromDescribeInstanceTypes. A new well-known labelkarpenter.k8s.aws/instance-nested-virtualizationis populated during instance type resolution. Today only*8i*families (c8i, m8i, r8i and flex variants) report this feature.CEL mutual exclusion:
nestedVirtualization: enabledcannot be combined withcoreCountorthreadsPerCore(EC2 rejects this combination at the API level).Error caching:
UnsupportedOperationadded tounfulfillableCapacityErrorCodesso launches against incompatible types are cached instead of retried.RFC included in
designs/cpu-options-nested-virtualization.mdper @AndrewMitchell25's request.Verified across 5 AWS regions (us-east-1, us-west-2, eu-west-1, ap-southeast-1, ap-northeast-1) that
ProcessorInfo.SupportedFeaturescorrectly identifies supported types.How was this change tested?
cpuOptions()converter (5 tests)NestedVirtualizationFilter(4 tests)go build ./pkg/...andgo vet ./pkg/...passDoes this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.