feat: support for ultrassd#1704
Conversation
rakechill
left a comment
rakechill
left a comment
There was a problem hiding this comment.
I think my main concern (reflected in a few different comments) is if we should treat the source of truth for a NAP cluster as the MachineAPI/ManagedCluster state OR the aksnodeclass fields?
Need a few things confirmed:
- Is this only set during cluster creation? or can it be enabled/disabled through the cluster lifecycle?
- Is it set at the cluster level? agentpool level? or both?
- Will/can/should Machine API report this as a DriftAction when the MC field for this changes?
If this is only enabled at cluster creation for AKS, I think this should instead be set via helm values -> env vars/flags -> opts.
If this can be disabled/enabled after cluster creation, I think this nodeclass field should be limited to self-hosted and machineapi should handle setting this for machines based on MC object.
This is set during cluster creation and nodepool creation. It can't be changed once a pool has been created. Enabling at the cluster means the system pool gets the UltraSSD capabilities, not that all nodepools/nodes in the cluster will have the functionality by default.
It's agentpool level. When you pass it during cluster create you're just enabling it on the initial create request. And other nodepools you add need to opt in. It's also immutable.
The field can't change once the cluster/pool is created, so I don't think we'd have to worry about this.
I think this would make sense if it was a global field that controlled all nodepools. But today each pool needs to opt into it, which is why I think it would make sense to stick it into either the NodePool or AKSNodeClass CRDs. Clusters can have a mix of ultrassd enabled/disabled pools on the same cluster. |
I see, didn't realize that enabling for the cluster == enabling for the system nodepool. In that case, it makes sense to me for this to be something that gets enabled from the Karpenter side. Will customers be able to change this after the fact then? That would be different behavior than what AKS currently provides. I'd expect there to be some validation if a customer tries to switch from Enabled to Disabled. Or, if we don't plan to mimic RP behavior, can we get clarity on why RP doesn't allow disablement after enablement? |
There was a problem hiding this comment.
Your comments from the previous review make sense + I better understand the agentpool<>cluster relationship wrt this feature.
A few remaining thoughts:
- Is there any way to make this per-nodepool? I expect many clusters will have different Nodepools for different use cases + may prefer that level of granularity. Maybe checking with @wdarko1 based on current customer requirements could be useful.
- Are the other features we have on AKSNodeClass also configurable per-agentpool in AKS? Or are they cluster-wide?
- Should we block disablement after enablement to align with current AKS behavior?
- Should we only allow ultra SSD to be enabled in Karpenter if it's enabled on the cluster? Basically introducing a validation here based on cluster state.
I feel like using the AKSNodeClass is the pattern that's already established. NodePools seem to be more about generic Karpenter scheduling specs and NodeClass about provider specific stuff. The NodePool also references the NodeClass so cx still has some level of granularity by swapping around the NodeClass that their NodePool points to. Will ask @wdarko1 for his thoughts.
FIPS, LocalDNS, and Artifact Streaming all operate at nodepool level in AKS. In particular, FIPS seems the most similar, since you can enable it at cluster creation time for the system pool but requires opt-in for subsequent nodepools.
AKS RP does reject attempts to change the UltraSSD settings on an existing pool today. In theory, if someone tries to edit the field on the NodeClass CR, we should consider the node as drifted and replace it.
I think No. Today in AKS you can make a cluster without enabling ultra ssd at creation and add an ultra-ssd enabled nodepool to it. |
|
I think that the proposed implementation would be pretty consistent with AKS. If you disable the field on the NodeClass, all the NodeClaims belonging to that NodePool/Class would be considered drifted and replaced, the same way you'd have to replace a nodepool in AKS. Interestingly, while AKS nodepools do not let you update the ultra ssd field, standalone VMs can actually enable/disable it, but require being stopped and deallocated first. |
rakechill
left a comment
rakechill
left a comment
There was a problem hiding this comment.
design lgtm, let's just add a customer experience section to the design doc that makes the enablement/disablement conditions clear + adds support/reasoning from the docs + current AKS behavior.
|
|
||
| Reasons: | ||
|
|
||
| - it is a provisioning feature, not just a schedulable label, |
There was a problem hiding this comment.
| - it is a provisioning feature, not just a schedulable label, | |
| - it is a provisioning feature, not a schedulable label, |
|
|
||
| #### Offerings Filtering | ||
|
|
||
| Ultra SSD is only available in regions and zones that support it, and only by specific SKUs. Therefore, we need to check availability for each zone when creating Offerings for InstanceTypes. |
There was a problem hiding this comment.
How does AKS API validates UltraSSD enablement?
Worth referencing that here and "prove" that Karpenter obliges it, especially if that is determined by how Karpenter select SKUs.
This is so that we can avoid provisioning time failures or accidentally having to support what AKS doesn't.
There was a problem hiding this comment.
I added a reference to this doc https://learn.microsoft.com/en-us/azure/aks/use-ultra-disks. AKS will reject requests for cluster or nodepool creations that have --enable-ultra-ssd but whose SKUs don't have UltraSSD available in the specified regions
| func configureUltraSSDEnabled(nodeClass *v1beta1.AKSNodeClass) *bool { | ||
| if nodeClass == nil || nodeClass.IsUltraSSDEnabled() == false { | ||
| return nil | ||
| } | ||
| return lo.ToPtr(nodeClass.IsUltraSSDEnabled()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Given this field affects instance filtering owned by Karpenter, Karpenter wants to own it and give Machine API a clear true or false.
Karpenter wouldn't want to give Machine API an unclear nil if Karpenter already assumes that it is false through instance filtering.
There was a problem hiding this comment.
Let's also make this point of defaulting in each layer clear in the design doc.
There was a problem hiding this comment.
Updated it for machine, which mirrors AKS behavior. Today in AKS though, vmss where UltraSSD == false leave the property as nil, so I'm thinking we do the same for the bootstrappingclient path.
|
|
||
| ## Overview | ||
|
|
||
| AKS supports Azure Ultra Disks by enabling Ultra SSD on the cluster or on a node pool at creation time with `--enable-ultra-ssd`. Nodes created from that cluster or node pool can then attach Persistent Volumes backed by the `UltraSSD_LRS` storage class. |
There was a problem hiding this comment.
Worth clarifying in the design doc the differences between cluster-level and pool-level API in AKS, and how Karpenter support here build on top of that.
E.g., if it is enabled at cluster-level, what will happen to the pools that don't enable it, vice-versa? If it is really just systempool and likely doesn't affect anything in Karpenter layer, then still worth a note.
There was a problem hiding this comment.
Added a note to the design doc in the cx experience section
Agree that customers should be able to select UltraSSD at the nodepool level. Customers can use multiple |
…e/karpenter-provider-azure into pablotrivino/enalbeUltraSSD
| LinuxOSConfig *LinuxOSConfiguration `json:"linuxOSConfig,omitempty"` | ||
| // ultraSSD enables Ultra SSD for the provisioned nodes. | ||
| // +optional | ||
| UltraSSD *UltraSSD `json:"ultraSSD,omitempty"` |
There was a problem hiding this comment.
We didn't put OSDiskSizeGB there but I wonder if we want a Disk or Storage section here, so we can group storage-related capabilities in the same way we do for GPU/Linux/etc, rather than UltraSSD which is too narrowly scoped (probably) to ever get new fields:
storage:
enableUltraSSD:
or somthing?
| offerings := []*cloudprovider.Offering{} | ||
| for zone := range offeringZones { | ||
| if params.UltraSSDEnabled { | ||
| if zone == "0" && !sku.IsUltraSSDAvailableWithoutAvailabilityZone() { |
There was a problem hiding this comment.
Do we have examples where ultrassd is enabled in some zones but not others? I assume this must happen given skewer supports it but might be good to understand what exactly that looks like.
|
|
||
| ### Decision 1: Where should Ultra SSD be configured? | ||
|
|
||
| #### Add a strongly typed field to `AKSNodeClass` |
There was a problem hiding this comment.
Did you consider a label?
scheduling.NewRequirement(v1beta1.LabelSKUStoragePremiumCapable, corev1.NodeSelectorOpIn, fmt.Sprint(sku.IsPremiumIO())),
scheduling.NewRequirement(v1beta1.LabelSKUAcceleratedNetworking, corev1.NodeSelectorOpIn, fmt.Sprint(sku.IsAcceleratedNetworkingSupported())),
Are labels, and feel very similar to me to ultraSSD. I also think doing a label would make the implementation a bit cleaner (because computeRequirements already takes params *instanceTypeParameters and the SKU, so we could do all of the filtering for ultraSSD+ Zones there).
It also means that a workload could ask for ultraSSD on the workload, rather than having to have a totally separate NodePool.
So I am wondering why not a well known label rather than a field on AKSNodeClass.
5 participants
Fixes #
Description
This change introduces support for ultra ssd. Included is a design doc that talks more about the implementation. UltraSSD will be enabled/disabled via a setting on the AKSNodeClass. The implementation here should be as close to AKS as possible.
How was this change tested?
New E2E: https://github.com/Azure/karpenter-provider-azure/actions/runs/27987096466/job/82831071878
*
Does this change impact docs?
Release Note