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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
63da061
Add priority as annotation to use spot containers for virtual kubelet
telotlik Apr 5, 2022
151530d
Add new test to validate null priority
telotlik Apr 6, 2022
d8fda88
Updating priority check in pod annotation
telotlik Apr 6, 2022
74fd22c
Update tests and add priority annotation const
telotlik Apr 7, 2022
3f23906
Update priority string check
telotlik Apr 7, 2022
d803498
Iterate on ACI Spot CG on Virtual Kubelets - new function for propert…
suselva Apr 13, 2022
b3bc432
Merge pull request #1 from telotlik/users/suselva/aci-spot
suselva Apr 21, 2022
cc15b33
api version when using priority
fnuarnav Apr 21, 2022
c0e15d8
update: set apiVersion based on extensible list of Tags
fnuarnav May 2, 2022
fb1a943
use VersioProvider to check ContainerGroupProperties
fnuarnav May 3, 2022
bbc2355
fix indentation; validate version format
fnuarnav May 4, 2022
c4c3b0a
Revert "fix indentation; validate version format"
fnuarnav May 4, 2022
bc57388
version set in code follows correct format
fnuarnav May 4, 2022
49c3e26
added unit tests for versioning
fnuarnav May 4, 2022
2843053
update logging; use more c# style documentation comments
fnuarnav May 5, 2022
f2a006a
remove print stmt; log uri in create
fnuarnav May 6, 2022
7f42078
fix indents
fnuarnav May 6, 2022
2b1c322
use virtual-kubelet.io/priority; update comment
fnuarnav May 6, 2022
a35c90a
use tag name virtual-kubelet.io-proprity
fnuarnav May 6, 2022
b75b772
fix typo
fnuarnav May 6, 2022
bd6bc81
set priority Tag with correct values only when priority annotaiton is…
fnuarnav May 6, 2022
e222422
clean up code
fnuarnav May 6, 2022
d780b79
define priorityTagNae as a constant
fnuarnav May 9, 2022
35334f6
Merge pull request #3 from telotlik/users/arnav/review-comments
fnuarnav May 9, 2022
7c147e3
Merge remote-tracking branch 'upstream/master' into telotlik/VKACISpo…
fnuarnav May 9, 2022
79ce469
Merge branch 'telotlik/VKACISpotContainers' into users/arnav/api-version
fnuarnav May 9, 2022
9613c46
fixed typo in comment
fnuarnav May 9, 2022
122d8bc
try adding parallelism to avoid timeout
fnuarnav May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions client/aci/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ const (
OnFailure ContainerGroupRestartPolicy = "OnFailure"
)

// ContainerGroupPriority enumerates the values for container group priority.
type ContainerGroupPriority string

const (
// Regular specifies the regular priority for container group restart priority.
Regular ContainerGroupPriority = "Regular"
// Spot specifies the spot priority for container group restart priority.
Spot ContainerGroupPriority = "Spot"
)

// ContainerNetworkProtocol enumerates the values for container network protocol.
type ContainerNetworkProtocol string

Expand Down Expand Up @@ -94,9 +104,10 @@ type ContainerGroupProperties struct {
Volumes []Volume `json:"volumes,omitempty"`
InstanceView ContainerGroupPropertiesInstanceView `json:"instanceView,omitempty"`
Diagnostics *ContainerGroupDiagnostics `json:"diagnostics,omitempty"`
SubnetIds []*SubnetIdDefinition `json:"subnetIds,omitempty"`
SubnetIds []*SubnetIdDefinition `json:"subnetIds,omitempty"`
Extensions []*Extension `json:"extensions,omitempty"`
DNSConfig *DNSConfig `json:"dnsConfig,omitempty"`
Priority ContainerGroupPriority `json:"priority,omitempty"`
}

// ContainerGroupPropertiesInstanceView is the instance view of the container group. Only valid in response.
Expand All @@ -105,7 +116,7 @@ type ContainerGroupPropertiesInstanceView struct {
State string `json:"state,omitempty"`
}

// SubnetIdDefinition is the subnet ID, the format should be
// SubnetIdDefinition is the subnet ID, the format should be
// /subscriptions/{subscriptionID}/resourceGroups/{ResourceGroup}/providers/Microsoft.Network/virtualNetworks/{VNET}/subnets/{Subnet}
type SubnetIdDefinition struct {
ID string `json:"id,omitempty"`
Expand Down Expand Up @@ -242,7 +253,7 @@ type GPUSKU string

const (
// K80 specifies the K80 GPU SKU
K80 GPUSKU = "K80"
K80 GPUSKU = "K80"
// P100 specifies the P100 GPU SKU
P100 GPUSKU = "P100"
// V100 specifies the V100 GPU SKU
Expand Down Expand Up @@ -460,7 +471,7 @@ type ExtensionType string

// Supported extension types
const (
ExtensionTypeKubeProxy ExtensionType = "kube-proxy"
ExtensionTypeKubeProxy ExtensionType = "kube-proxy"
ExtensionTypeRealtimeMetrics ExtensionType = "realtime-metrics"
)

Expand Down
18 changes: 18 additions & 0 deletions provider/aci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

)

const (
statusReasonPodDeleted = "NotFound"
statusMessagePodDeleted = "The pod may have been deleted from the provider"
Expand Down Expand Up @@ -657,6 +661,19 @@ func (p *ACIProvider) CreatePod(ctx context.Context, pod *v1.Pod) error {
containerGroup.Location = p.region
containerGroup.RestartPolicy = aci.ContainerGroupRestartPolicy(pod.Spec.RestartPolicy)
containerGroup.ContainerGroupProperties.OsType = aci.OperatingSystemTypes(p.operatingSystem)
if pod.Annotations != nil {
priority, priorityExists := pod.Annotations[priorityTypeAnnotation]
if priorityExists {

if strings.EqualFold(priority, string(aci.Spot)) {
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

return fmt.Errorf("The pod requires either Regular or Spot priority. Invalid value %s", priority)
}
}
}

// get containers
containers, err := p.getContainers(pod)
Expand Down Expand Up @@ -716,6 +733,7 @@ func (p *ACIProvider) CreatePod(ctx context.Context, pod *v1.Pod) error {
"Namespace": pod.Namespace,
"UID": podUID,
"CreationTimestamp": podCreationTimestamp,
"Priority": pod.Annotations["priority"],
}

p.amendVnetResources(&containerGroup, pod)
Expand Down
Loading