feat: Add ACI Spot virtual nodes support to virtual kubelet #192
Conversation
…y set and update local test - use constant variable for priority annotation keyname - move the CG priority property set to a function - Update Local Test
helayoty
changed the title
provider/aci.go
Outdated
| containerGroup.ContainerGroupProperties.Priority = aci.Spot | ||
| } else if strings.EqualFold(priority, string(aci.Regular)) { | ||
| containerGroup.ContainerGroupProperties.Priority = aci.Regular | ||
| } else { |
There was a problem hiding this comment.
won't this fail all the existing ones?
There was a problem hiding this comment.
Validated that this change does not affect existing behavior
Iterate on ACI Spot Changes
provider/aci.go
Outdated
| @@ -61,6 +61,10 @@ const ( | |||
| gpuTypeAnnotation = "virtual-kubelet.io/gpu-type" | |||
| ) | |||
|
|
|||
| const ( | |||
| priorityTypeAnnotation = "priority" | |||
There was a problem hiding this comment.
This string should be prefixed in some way in order to not conflict with potential annotations already in use by a customer pod for maximum compatibility with customers' existing pods. I suspect priority is a reasonably frequent "private" annotation.
See for example virtualKubeletDNSNameLabel or gpuTypeAnnotation.
I'd say that an argument can be made that given how ACI-specific this is, we could "namespace" it even more specifically than just virtual kubelet, but at the very least, IMO, it should match the other annotation-based extensions here.
| @@ -728,6 +738,26 @@ func (p *ACIProvider) CreatePod(ctx context.Context, pod *v1.Pod) error { | |||
| return p.createContainerGroup(ctx, pod.Namespace, pod.Name, &containerGroup) | |||
| } | |||
|
|
|||
| // Set the Container Group Priority Property | |||
| // Accepted Values : Regular, Spot | |||
There was a problem hiding this comment.
Since the priority is not an explicit parameter to this function, this comment should explain how priority is passed in, if we already mention what the accepted values are :)
Additionally, as far as I can tell from the code, it is also allowed not to specify it at all, so I'd recommend mentioning that too.
This reverts commit bbc2355.
addressing review comments
|
@telotlik please rebase and make sure all pipelines pass. |
This change adds ACI spot virtual node support to virtual kubelet.
The user can add ACI priority ('Spot' or 'Regular') to the pod definition as a part of Annotations, which will be used to direct the pod to run either on ACI Spot pool virtual nodes (for Spot priority) or ACI pool virtual nodes (for Regular priority).
This change enables users to add Spot Virtual Node to existing AKS cluster or create a new cluster which has Spot Virtual Nodes. All pods deployed on Spot Virtual Nodes will be run on ACI Spot Containers.
Validated the change by testing locally on minikube as well as by deploying on AKS cluster. Also added tests to verify that the pod is created only when valid values are passed for priority property in annotations.