Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add ACI Spot virtual nodes support to virtual kubelet #192

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

telotlik
Copy link

@telotlik telotlik commented Apr 5, 2022

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.

@helayoty helayoty changed the title Add ACI Spot virtual nodes support to virtual kubelet feat: Add ACI Spot virtual nodes support to virtual kubelet Apr 18, 2022
@helayoty helayoty linked an issue Apr 18, 2022 that may be closed by this pull request
@helayoty helayoty self-requested a review April 18, 2022 15:13
provider/aci.go Outdated
containerGroup.ContainerGroupProperties.Priority = aci.Spot
} else if strings.EqualFold(priority, string(aci.Regular)) {
containerGroup.ContainerGroupProperties.Priority = aci.Regular
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this fail all the existing ones?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated that this change does not affect existing behavior

provider/aci.go Outdated
@@ -61,6 +61,10 @@ const (
gpuTypeAnnotation = "virtual-kubelet.io/gpu-type"
)

const (
priorityTypeAnnotation = "priority"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@helayoty
Copy link
Member

@telotlik please rebase and make sure all pipelines pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ACI Spot virtual nodes support to virtual kubelet
6 participants