feat(scheduler): support template node simulation filtering - #2046
Conversation
📝 WalkthroughWalkthroughAdds deep-copy support for device and node usage data, constructs transient node usage from Kubernetes nodes, and introduces simulation-based extender filtering with configurable scoring diagnostics. Tests cover template-node selection, failure reporting, unregistered nodes, cache isolation, and nested copy independence. ChangesScheduler node simulation filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SchedulerFilter
participant filterSimulation
participant getSimulationNodesUsage
participant buildTransientNodeInfo
participant calcScoreWithOptions
SchedulerFilter->>filterSimulation: pass template NodeList
filterSimulation->>getSimulationNodesUsage: build simulated candidates
getSimulationNodesUsage->>buildTransientNodeInfo: discover devices
buildTransientNodeInfo-->>getSimulationNodesUsage: return NodeInfo or failure
getSimulationNodesUsage-->>filterSimulation: return usage and FailedNodes
filterSimulation->>calcScoreWithOptions: score candidates
calcScoreWithOptions-->>filterSimulation: return scores
filterSimulation-->>SchedulerFilter: return selected node and failures
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 simulation-based node filtering in the scheduler by adding NodeInfo to NodeUsage, refactoring node usage building, and implementing filterSimulation along with corresponding unit tests. The review feedback highlights two important issues: first, the DeepCopy method for NodeUsage performs a shallow copy of device.DeviceInfo elements containing reference types, which can lead to concurrent mutation issues; second, buildTransientNodeInfo lacks defensive nil checks for node and deviceInfo, risking nil pointer dereferences.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/scheduler.go (1)
798-817: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSimulation requests still record a pod Event on the zero-resource path.
recordScheduleFilterResultEvent(...)at Line 801 runs before theargs.Nodes != nilcheck, so a simulation (template-node) request that happens to request no resources will still emit an Event on the pod. This contradicts the PR goal of keeping event recording and other mutations disabled for the simulation path. Gate the event on the non-simulation branch.🛠️ Proposed reorder
if resourceReqTotal == 0 { klog.V(1).InfoS("Pod does not request any resources", "pod", args.Pod.Name) - s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", fmt.Errorf("does not request any resource")) if args.Nodes != nil { return &extenderv1.ExtenderFilterResult{ Nodes: args.Nodes, FailedNodes: nil, Error: "", }, nil } + s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", fmt.Errorf("does not request any resource")) return &extenderv1.ExtenderFilterResult{ NodeNames: args.NodeNames, FailedNodes: nil, Error: "", }, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/scheduler.go` around lines 798 - 817, Simulation requests on the zero-resource path are still emitting pod events because recordScheduleFilterResultEvent in scheduler.go runs before the args.Nodes nil check. Move the event recording into the non-simulation branch of the zero-resource handling in the scheduler filter logic, so only the args.NodeNames path records the event and the args.Nodes template-node path remains side-effect free. Use the existing resourceReqTotal zero check and the args.Nodes / args.NodeNames split in the filter path to keep the fix localized.
🧹 Nitpick comments (2)
pkg/scheduler/score.go (1)
105-109: 📐 Maintainability & Code Quality | 🔵 TrivialConsider an options struct over positional booleans.
calcScoreWithOptions(..., recordEvents, detailedFailureReason)with call sites liketrue, falseis easy to misread/transpose as more callers are added (e.g., the simulation path). An options struct would self-document at call sites.♻️ Optional refactor
-func (s *Scheduler) calcScore(nodes *map[string]*NodeUsage, resourceReqs device.PodDeviceRequests, task *corev1.Pod, failedNodes map[string]string) (*policy.NodeScoreList, error) { - return s.calcScoreWithOptions(nodes, resourceReqs, task, failedNodes, true, false) +type calcScoreOptions struct { + recordEvents bool + detailedFailureReason bool +} + +func (s *Scheduler) calcScore(nodes *map[string]*NodeUsage, resourceReqs device.PodDeviceRequests, task *corev1.Pod, failedNodes map[string]string) (*policy.NodeScoreList, error) { + return s.calcScoreWithOptions(nodes, resourceReqs, task, failedNodes, calcScoreOptions{recordEvents: true, detailedFailureReason: false}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/score.go` around lines 105 - 109, Refactor the boolean flags on calcScoreWithOptions into a small options struct so call sites no longer pass ambiguous positional values like true, false. Update calcScore to construct and pass the options, and adjust all call sites (including any simulation path) to use the new self-documenting struct fields instead of raw booleans; keep the behavior of recordEvents and detailedFailureReason unchanged.pkg/scheduler/scheduler_test.go (1)
1430-1682: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test setup into a helper to reduce duplication.
All four new tests repeat identical boilerplate:
NewScheduler(), fake clientset wiring,podListerconstruction, and thesConfig/InitDevicesWithConfigblock. Consider a small helper (e.g.newTestSchedulerWithNvidiaConfig(t)) returning a ready*Schedulerto cut repetition and keep future config changes in one place.♻️ Example helper
func newTestSchedulerForTemplateFilterTests(t *testing.T) *Scheduler { s := NewScheduler() client.KubeClient = fake.NewSimpleClientset() s.kubeClient = client.KubeClient s.podLister = informers.NewSharedInformerFactoryWithOptions(client.KubeClient, time.Hour).Core().V1().Pods().Lister() sConfig := &config.Config{ NvidiaConfig: nvidia.NvidiaConfig{ ResourceCountName: "hami.io/gpu", ResourceMemoryName: "hami.io/gpumem", ResourceMemoryPercentageName: "hami.io/gpumem-percentage", ResourceCoreName: "hami.io/gpucores", DefaultGPUNum: 1, }, } require.NoError(t, config.InitDevicesWithConfig(sConfig)) return s }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/scheduler_test.go` around lines 1430 - 1682, All four template-filter tests duplicate the same scheduler initialization boilerplate, so extract that setup into a shared helper to keep the tests smaller and centralize config changes. Create a helper such as newTestSchedulerForTemplateFilterTests(t) that performs the NewScheduler, fake clientset wiring, podLister creation, and config.InitDevicesWithConfig setup, then update TestFilterUsesTemplateNodesWithoutSideEffects, TestFilterTemplateNodesDoesNotTouchSchedulingCaches, TestFilterTemplateNodesReturnsDetailedFailureReason, and TestFilterTemplateNodesMissingRegisterAnnotation to use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 798-817: Simulation requests on the zero-resource path are still
emitting pod events because recordScheduleFilterResultEvent in scheduler.go runs
before the args.Nodes nil check. Move the event recording into the
non-simulation branch of the zero-resource handling in the scheduler filter
logic, so only the args.NodeNames path records the event and the args.Nodes
template-node path remains side-effect free. Use the existing resourceReqTotal
zero check and the args.Nodes / args.NodeNames split in the filter path to keep
the fix localized.
---
Nitpick comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 1430-1682: All four template-filter tests duplicate the same
scheduler initialization boilerplate, so extract that setup into a shared helper
to keep the tests smaller and centralize config changes. Create a helper such as
newTestSchedulerForTemplateFilterTests(t) that performs the NewScheduler, fake
clientset wiring, podLister creation, and config.InitDevicesWithConfig setup,
then update TestFilterUsesTemplateNodesWithoutSideEffects,
TestFilterTemplateNodesDoesNotTouchSchedulingCaches,
TestFilterTemplateNodesReturnsDetailedFailureReason, and
TestFilterTemplateNodesMissingRegisterAnnotation to use it.
In `@pkg/scheduler/score.go`:
- Around line 105-109: Refactor the boolean flags on calcScoreWithOptions into a
small options struct so call sites no longer pass ambiguous positional values
like true, false. Update calcScore to construct and pass the options, and adjust
all call sites (including any simulation path) to use the new self-documenting
struct fields instead of raw booleans; keep the behavior of recordEvents and
detailedFailureReason unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ac88118-c7cb-4612-822d-5f9ebd17d834
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
pkg/scheduler/nodes.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/scheduler/score.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Manual integration evidence for the template-node Setup:
Template node excerpt used in the warm-path cases: {
"metadata": {
"name": "aks-userpool-85868546-vmss000000",
"annotations": {
"hami.io/node-nvidia-register": "[{\"id\":\"GPU-MOCK-0\",\"count\":10,\"devmem\":11441,\"devcore\":100,\"type\":\"NVIDIA-Tesla-K80\",\"mode\":\"hami-core\"},{\"id\":\"GPU-MOCK-1\",\"count\":10,\"devmem\":11441,\"devcore\":100,\"type\":\"NVIDIA-Tesla-K80\",\"mode\":\"hami-core\"}]"
}
}
}
Request excerpt: {
"pod": {
"metadata": {"name": "template-fit"},
"spec": {
"schedulerName": "hami-scheduler",
"containers": [{
"resources": {
"limits": {
"nvidia.com/gpu": "1",
"nvidia.com/gpumem": "300",
"nvidia.com/gpucores": "40"
}
}
}]
}
},
"nodes": {
"items": [{"metadata": {"name": "aks-userpool-85868546-vmss000000"}}]
}
}Response excerpt observed from {
"Nodes": {
"items": [
{"metadata": {"name": "aks-userpool-85868546-vmss000000"}}
]
},
"FailedNodes": null,
"Error": ""
}
Request excerpt: {
"pod": {
"metadata": {"name": "template-unfit"},
"spec": {
"containers": [{
"resources": {
"limits": {
"nvidia.com/gpu": "1",
"nvidia.com/gpumem": "12000",
"nvidia.com/gpucores": "40"
}
}
}]
}
},
"nodes": {
"items": [{"metadata": {"name": "aks-userpool-85868546-vmss000000"}}]
}
}Response excerpt observed from {
"Nodes": null,
"FailedNodes": {
"aks-userpool-85868546-vmss000000": "2/2 CardInsufficientMemory"
},
"Error": ""
}
Request excerpt: {
"pod": {
"metadata": {"name": "template-unregistered"},
"spec": {
"containers": [{
"resources": {
"limits": {
"nvidia.com/gpu": "1",
"nvidia.com/gpumem": "300",
"nvidia.com/gpucores": "40"
}
}
}]
}
},
"nodes": {
"items": [{
"metadata": {
"name": "aks-userpool-85868546-vmss000000",
"annotations": {}
}
}]
}
}Response excerpt observed from {
"Nodes": null,
"FailedNodes": {
"aks-userpool-85868546-vmss000000": "node unregistered"
},
"Error": ""
}These three cases were the manual pre-e2e validation for this PR:
|
|
/assign fishman |
|
/assign wawa0210 |
759e515 to
94c78d4
Compare
94c78d4 to
15955c7
Compare
Signed-off-by: spencercjh <jiahao.cai@dynamia.ai>
Signed-off-by: spencercjh <jiahao.cai@dynamia.ai>
|
@mesutoezdil |
thx, can you open an pr for that? we have done few things last week (meant new rules for contributions), sometimes some things are being overlooked cc @archlitchi |
|
/lgtm |
|
We ran a dedicated validation for cold-zero node group scale-up and the current result is: Without additional changes, HAMi does not yet support cold-zero node group scale-up. What is already validated is the warm-path case: when the node group already has an existing node template that can be evaluated with the expected GPU resource surface, Cluster Autoscaler can make the scale-up decision and the workload can eventually land after the new node finishes HAMi/device-plugin registration. What is not working yet is the cold-zero path, where the target node group starts from Our experiments narrowed the problem down as follows:
So the current boundary is:
A simplified sequence is shown below: sequenceDiagram
participant Pod as GPU Pod
participant Webhook as HAMi Admission Webhook
participant CA as Cluster Autoscaler
participant Template as Zero-size Template Node
participant HAMi as HAMi Scheduler Logic
Pod->>Webhook: Create pod (author may request only nvidia.com/gpu)
Webhook->>Pod: Mutate request (inject nvidia.com/gpucores=100)
Pod->>CA: Unschedulable pod reaches CA evaluation
CA->>Template: Build/evaluate zero-size nodegroup template
Template-->>CA: GPU SKU may expose nvidia.com/gpu
Template-->>CA: Missing nvidia.com/gpucores (and sometimes gpumem)
CA-->>Pod: NotTriggerScaleUp
Note over CA,HAMi: Cold-zero fails before a real node is created
In short, this work shows that supporting cold-zero scale-up requires additional changes so that the zero-size template path exposes the HAMi GPU resource contract expected after admission, rather than only the base GPU presence. |
can you describe the spec of your user pool and node (aks-userpool-85868546-vmss000000) here? it seems it has two GPUs, but i'm not quite certain |
|
I think we can accept this design, but we need to note some risks here:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, spencercjh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
Most of this Pull Request was generated or revised with the assistance of AI tools, including Codex and Raft(Slock). I have reviewed the resulting content and take full responsibility for its accuracy, security, licensing compliance, and inclusion in this project.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a simulation-only template-node filter path to the HAMi scheduler extender.
When the extender receives
ExtenderArgs.Nodes, it now:NodeInfoandDeviceUsagefrom the provided template/synthetic nodesThe normal scheduling path that consumes
ExtenderArgs.NodeNamesis unchanged.Which issue(s) this PR fixes:
Related to #1099 and kubernetes/autoscaler#9786
Special notes for your reviewer:
Implementation notes:
Filter()now branches onargs.Nodes != niland uses a simulation-only path for template-node evaluation.GetNodeDevices().Integration validation:
hami-schedulerdeploymentmock-device-pluginin the cluster as the fixture sourcesvc/hami-schedulerand sent real HTTPSPOST /filterrequests against the live extenderhami.io/node-nvidia-registerreturns fitCardInsufficientMemorynode unregisteredLocal validation already run before opening this PR:
make lintmake tidymake verifyDoes this PR introduce a user-facing change?:
HAMi scheduler extender can now evaluate template nodes passed through
ExtenderArgs.Nodesfor simulation use cases, reusing existing device-fit logic without mutating live scheduling state.Summary by CodeRabbit
New Features
Bug Fixes