feat(scheduler): add GPU-aware PreemptVerb victim refinement#1924
feat(scheduler): add GPU-aware PreemptVerb victim refinement#1924maishivamhoo123 wants to merge 9 commits into
Conversation
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maishivamhoo123 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a vGPU preemption mechanism for the HAMi scheduler, adding a new /preempt endpoint, implementing the VgpuPreempt extender plugin to refine victim selection, and integrating it into the scheduler. The review feedback provides valuable recommendations to optimize and clean up the implementation. Specifically, it suggests processing co-located pods in a deterministic order (Annotated, Native, then unannotated vGPU pods) to avoid false negatives, removing unused fields (devicesSnapshot and mutex) from the VgpuPreempt struct, utilizing sync/atomic instead of a mutex for metrics counters to reduce contention, and simplifying the resource allocation simulation by directly assigning the mutated device usage copy.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
accountVGPURequests returns nil even when no device can satisfy the request. The caller podFitsAfterPreemption then thinks the pod fits when it does not. That can cause wrong preemption decisions. The unsatisfied branch needs to return an error. |
|
pdbViolationsUpperBound takes addedLen but never uses it. Either the formula is wrong and addedLen should be part of it, or the parameter should be dropped. |
|
nativeWholeGPUResources, extractDeviceTypeFromResourceName, and isVGPUResourcePod all use a hardcoded vendor list that is missing several vendors HAMi already supports: metax-tech.com, mthreads.com, kunlun.com, aws.amazon.com (neuron), vastai.com, enflame.com. Pods from those vendors will not be seen as GPU pods and preemption will skip them. The device registry already has this info via device.GetDevices(), so it would be better to derive vendor detection from there. |
|
informerFactory is only set inside Start(), so GetInformerFactory() returns nil if called before that. preempt.New() passes it straight to factory.Core().V1() with no nil check. That will panic. A nil guard in preempt.New() would prevent the crash. |
|
isCriticalPod protects kube-public but that namespace is not for system pods. It is just publicly readable cluster info. No critical workloads run there so protecting it from preemption seems wrong. |
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
@mesutoezdil I’ve addressed all the suggested changes. Could you please take another look at the PR and let me know if there’s anything else I should update? Thank you! |
|
The vendor list fix is incomplete. getNativeWholeGPUResources() has the new vendors and accountNativeWholeGPUs uses it, but podFitsAfterPreemption still references the old nativeWholeGPUResources global in two places: line 469 inside the hasNativeGPU lambda Those two spots still miss metax-tech.com, mthreads.com, kunlun.com, aws.amazon.com, vastai.com and enflame.com. Either replace the global with the new function there too, or remove the global entirely and have everyone call getNativeWholeGPUResources(). |
|
extractDeviceTypeFromResourceName does not match the actual device registry keys and accountNativeWholeGPUs will silently fail for almost every vendor including NVIDIA. The state map is keyed by CommonWord() values from the device registry. Those values are: NVIDIA, AMDGPU, MLU, DCU, Metax-GPU, Mthreads, AWSNeuron, Vastai, Enflame. But extractDeviceTypeFromResourceName returns lowercase or abbreviated strings: "nvidia", "amd", "cambricon", "hygon", "metax-tech", "mthreads", "aws". None of those match except "kunlun". When accountNativeWholeGPUs does state[devType] with the wrong key it gets nil and returns "no device state for type". In production this means preemption will always fail for native GPU pods, including nvidia.com/gpu. The tests pass because the mockDevice uses return "nvidia" and is registered under that key. Production uses NvidiaGPUDevice = "NVIDIA". The fix is to derive the device type key from device.GetDevices() by matching resource names against GetResourceNames() on each registered device, instead of hardcoding string patterns. |
|
Please check the device registry key values before the next round. The CommonWord() return values in each device package are the actual map keys used at runtime, and they do not match what extractDeviceTypeFromResourceName returns. Worth reading through the device packages alongside this code. |
@mesutoezdil Thank you for pointing this out . Thank you! |
sure i will go through device package |
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
@mesutoezdil I added all the recommended changes . Can you please take a look |
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
| return fmt.Errorf("insufficient free devices for %d whole GPU(s) of type %q", count, devType) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This call double-counts the preemptor resources on the default HAMi config.
When ResourceCountName is "nvidia.com/gpu" (the default in values.yaml), accountNativeWholeGPUs marks one full GPU slot as consumed because nvidia.com/gpu is in getNativeWholeGPUResources(). Then the Fit() loop below runs on the same state and tries to allocate the same slot again. On a single-GPU node the slot is already fully consumed so Fit() returns false and podFitsAfterPreemption always returns false for normal HAMi vGPU pods.
The tests do not catch this because the mock device uses hami.io/vgpu as the resource name, which is not in getNativeWholeGPUResources(), so accountNativeWholeGPUs returns errNoNativeGPURequested and execution falls through to Fit() correctly. The real config uses nvidia.com/gpu which is in that map, so production hits the bug.
Fix: remove this accountNativeWholeGPUs call for the preemptor. The Fit() loop already handles the preemptor request. accountNativeWholeGPUs is for co-located pods that the scheduler cannot parse through device.Resourcereqs.
|
@maishivamhoo123 please go through the three review comments carefully before requesting another review. Read your code carefully and successfully complete the tests. Otherwise, we will constantly have to spend time on this. Check the code integrity again after the changes you made. It is currently problematic. |
| // getNativeWholeGPUResources returns resource names that represent entire physical GPUs. | ||
| // Dynamically built from common patterns HAMi supports. | ||
| func getNativeWholeGPUResources() map[string]bool { | ||
| return map[string]bool{ |
There was a problem hiding this comment.
It's better to get the resourceName from config or DevicesMap
HAMi/pkg/scheduler/config/config.go
Line 75 in 5dca58e
Line 171 in 5dca58e
| return true | ||
| } | ||
| if owner := metav1.GetControllerOf(pod); owner != nil && owner.Kind == "DaemonSet" { | ||
| return true |
There was a problem hiding this comment.
If a pod is not scheduled by HAMi, can it be evicted?
| for _, cReqs := range preemptorReqs { | ||
| for devType, req := range cReqs { | ||
| if req.Nums > 0 || req.Memreq > 0 || req.Coresreq > 0 { | ||
| requestedDevTypes[devType] = true |
| // Also add native whole GPU requests to requestedDevTypes | ||
| for _, c := range append(preemptor.Spec.Containers, preemptor.Spec.InitContainers...) { | ||
| for rName := range c.Resources.Requests { | ||
| if nativeGPUResources[string(rName)] { |
There was a problem hiding this comment.
Why check again with nativeGPUResources?
| } | ||
| // Manual deep copy of the struct | ||
| copy := *u | ||
| dst[i] = © |
There was a problem hiding this comment.
DeviceUsage has its own deep copy function DeepCopy
3 participants
Summary
This PR adds
PreemptVerbsupport to the HAMi scheduler extender, enabling GPU-aware refinement of Kubernetes preemption decisions.What this change does
Preemptinterface.PriorityClasssemantics.PodDisruptionBudgetconstraints during additional victim selection.This implementation focuses on GPU-aware victim refinement while preserving the existing Kubernetes preemption workflow.
Fixes #1878
AI Assistance Disclosure
AI-assisted tools were used to help review implementation logic, improve code comments, and assist with writing and validating unit tests. The overall design, implementation, and final verification of the changes were performed by the @maishivamhoo123 .