feat(scheduler): correct resource accounting for init containers#1773
feat(scheduler): correct resource accounting for init containers#1773maishivamhoo123 wants to merge 16 commits into
Conversation
|
[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 support for Kubernetes InitContainers within the scheduler and admission webhook. It updates the resource calculation logic in the Filter and fitResourceQuota functions to use the maximum of the largest init container's request and the sum of regular containers' requests, aligning with standard Kubernetes behavior. Additionally, the scoring logic in calcScore now utilizes a node state cloning mechanism to ensure init containers are evaluated against the node's full capacity. Review feedback suggests handling potential errors during JSON unmarshaling and optimizing the cloning process to avoid unnecessary performance overhead for pods that do not contain init containers.
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:
|
|
@archlitchi , @DSFans2014 , @Shouren and @member can you please review this PR. |
@maishivamhoo123 I am busy testing the recent updates for k8s-dra-driver, i will be back when the tests are finished. |
|
@Shouren can you please check this google docs. |
|
@maishivamhoo123 Can you show me the Pod Annotations after a successful scheduling? |
|
@Shouren can you please review this PR? |
|
hi, since i just got this PR merged:#1818, does this one needs to adjust? |
Yes i need to change the JSON serialization logic to deep copy function i will change and let you know . Thank you! |
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>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
maishivamhoo123
force-pushed
the
fix/nodelock-time-parsec
branch
from
d443f9e to
f6cc3d6
Compare
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
CC @DSFans2014 |
CC @DSFans2014 |
DSFans2014
left a comment
DSFans2014
left a comment
There was a problem hiding this comment.
@maishivamhoo123 It looks like the PR title doesn't correspond to the changes being made.
There was a problem hiding this comment.
Should we need add special handling before AddPod? Otherwise, resources from both initContainers and containers will be counted.
HAMi/pkg/scheduler/scheduler.go
Lines 798 to 805 in 430b458
maishivamhoo123
changed the title
github-actions
Bot
added
kind/feature
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 161-176: The init-container alias stripping path in scheduler
logic is calling stripInitContainerAliasSlots with nil resource requirements,
which disables the slot-count guard and can misalign pod quota accounting.
Update the allInitDone branch in scheduler’s pod handling to pass the existing
resourceReqs into stripInitContainerAliasSlots instead of nil so the guard
remains active for both onAddPod and onUpdatePod paths.
In `@pkg/scheduler/score.go`:
- Around line 360-371: The peak/commit accounting in score.go is using slice
position from nodeCopy.Devices.DeviceLists, but fitInDevices can reorder that
slice in place and cause updates to land on the wrong device. Update the
init-container peak loop and the final commit loop to look up and write usage by
Device.ID instead of the pi index, using the existing fitInDevices/device
bookkeeping in score.go to keep peakUsage and committed usage aligned with the
correct device regardless of sort order or heterogeneous nodes.
- Around line 105-124: Restore the rollback logic in score.go so it does not
depend on the current DeviceLists order after sort.Sort(node.Devices) mutates
it. Update the devSnapshot captured in the score function to store the
*device.DeviceUsage for each entry, and change rollbackAll to write Used,
Usedcores, and Usedmem back through that saved pointer instead of
node.Devices.DeviceLists[i].Device.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b832f3-8107-4868-a0ae-19df1b048733
📒 Files selected for processing (5)
pkg/device/common/common.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/scheduler/score.gopkg/scheduler/score_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/scheduler_test.go
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 (2)
pkg/scheduler/webhook.go (2)
189-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid pre-scaling
memoryReqhere.FitQuotaalready multiplies the quota limit bymemoryFactor, so scaling the request again makes the admission check stricter than intended whenmemoryFactor > 1. Pass the raw request through and letFitQuotaapply the factor.🤖 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/webhook.go` around lines 189 - 195, The quota admission path in webhook.go is double-applying the memory factor by pre-scaling memoryReq before calling FitQuota. Update the logic around the memory adjustment and the FitQuota call so memoryReq remains the raw request value, and rely on FitQuota to apply memoryFactor itself; keep the existing logging in sync with the unscaled request for the relevant webhook flow.
160-171: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInit-container GPU requests above 1 are skipped in quota accounting
pkg/scheduler/webhook.go:160-171
Thereq == 1guard drops init containers requesting 2+ GPUs, so their memory/CPU never contributes toinitMemoryReq/initCoresReq. Match the regular-container branch here, or reject multi-GPU init requests consistently.🤖 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/webhook.go` around lines 160 - 171, The init-container handling in webhook logic is skipping any container whose GPU request is greater than 1 because of the req == 1 guard, so its memory and CPU never get counted into initMemoryReq/initCoresReq. Update the InitContainers loop in getRequest-based quota accounting to match the regular-container branch by including multi-GPU init containers, or apply the same validation/rejection consistently before this block so the scheduler behavior stays aligned.
🤖 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/webhook.go`:
- Around line 189-195: The quota admission path in webhook.go is double-applying
the memory factor by pre-scaling memoryReq before calling FitQuota. Update the
logic around the memory adjustment and the FitQuota call so memoryReq remains
the raw request value, and rely on FitQuota to apply memoryFactor itself; keep
the existing logging in sync with the unscaled request for the relevant webhook
flow.
- Around line 160-171: The init-container handling in webhook logic is skipping
any container whose GPU request is greater than 1 because of the req == 1 guard,
so its memory and CPU never get counted into initMemoryReq/initCoresReq. Update
the InitContainers loop in getRequest-based quota accounting to match the
regular-container branch by including multi-GPU init containers, or apply the
same validation/rejection consistently before this block so the scheduler
behavior stays aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d31b7e7f-12a3-418e-93ff-38a7331885bb
📒 Files selected for processing (4)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/scheduler/webhook_test.go
- pkg/scheduler/scheduler_test.go
- pkg/scheduler/scheduler.go
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
| if ok { | ||
| if memReq, ok := getRequest(&ctr, memResourceName); ok { | ||
| memoryReq += memReq * req | ||
| appMemoryReq += memReq |
There was a problem hiding this comment.
@DSFans2014 Fixed – the regular container loop now multiplies memory/core requests by the GPU count (req). The init container loop also uses max(..., memReq * req). The test TestFitResourceQuota confirms correct accounting for multi‑GPU pods.
| } | ||
|
|
||
| // stripInitContainerAliasSlots removes the device allocations of init containers from the | ||
| // PodDevices structure, keeping only regular (non-init) container allocations. |
There was a problem hiding this comment.
Here we exclude init containers when calculating resources. If an init container requests more resources than the regular containers, it might cause the quota to be exceeded.
There was a problem hiding this comment.
@DSFans2014 The stripping is safe because the admission quota check already uses max(initReq, appReq), so the worst‑case resource demand is covered. Stripping only happens after all init containers have terminated, so ongoing usage correctly reflects only the running regular containers. The scheduler also validates both stages independently, preventing any oversubscription.
There was a problem hiding this comment.
@maishivamhoo123 reminder: pls adhere to the new rules regarding PRs, descriptions, responses, and codes.
There was a problem hiding this comment.
Currently, AddUsage and AddPod only accounts for resource usage from regular containers. Consider a scenario where the quota is set to 30G. An init container requests 20G, and the regular containers request 10G, but we only count the 10G. When the init container is running, if another pod requests 20G, would that exceed the quota?
usageDevices := stripInitContainerAliasSlots(args.Pod, resourceReqs, m.Devices)
added := s.podManager.AddPod(args.Pod, m.NodeID, usageDevices)
if added {
s.quotaManager.AddUsage(args.Pod, usageDevices)
}
There was a problem hiding this comment.
Stripping only happens after all init containers have terminated
How do we ensure this works?
There was a problem hiding this comment.
Thank you @DSFans2014 for raising this issue i am really sorry. I didn't get this thought ,
i generated this issue on my fake gpu environment ya it is there i will see it and let you know ?
apiVersion: v1
kind: ResourceQuota
metadata:
name: test-quota
namespace: default
spec:
hard:
nvidia.com/gpumem: "30000"
resourcequota/test-quota created
apiVersion: v1
kind: Pod
metadata:
name: test-init-quota-pod1
spec:
initContainers:
- name: init-heavy
image: busybox
command: ["sleep", "120"] # keep init running long enough
env:
- name: CUDA_DISABLE_CONTROL
value: "true"
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: "20000"
containers:
- name: app
image: busybox
command: ["sleep", "3600"]
env:
- name: CUDA_DISABLE_CONTROL
value: "true"
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: "10000"
restartPolicy: Never
pod/test-init-quota-pod1 created
apiVersion: v1
kind: Pod
metadata:
name: test-init-quota-pod2
spec:
initContainers:
- name: init-heavy
image: busybox
command: ["sleep", "120"]
env:
- name: CUDA_DISABLE_CONTROL
value: "true"
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: "20000"
containers:
- name: app
image: busybox
command: ["sleep", "3600"]
env:
- name: CUDA_DISABLE_CONTROL
value: "true"
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: "10000"
restartPolicy: Never
shivam_kumar@LAPTOP-NMKA92EB:~/HAMi$ kubectl get pods test-init-quota-pod1 test-init-quota-pod2
kubectl describe pod test-init-quota-pod2 | grep -A5 Events
NAME READY STATUS RESTARTS AGE
test-init-quota-pod1 0/1 Init:0/1 0 18s
test-init-quota-pod2 0/1 Init:0/1 0 7s
Events:
Type Reason Age From Message
Normal Scheduled 7s hami-scheduler Successfully assigned default/test-init-quota-pod2 to nvml-mock-test-control-plane
Normal FilteringSucceed 8s hami-scheduler find fit node(nvml-mock-test-control-plane), 0 nodes not fit, 1 nodes fit(nvml-mock-test-control-plane:1.17)
Normal BindingSucceed 8s hami-scheduler Successfully binding node [nvml-mock-test-control-plane] to default/test-init-quota-pod2
shivam_kumar@LAPTOP-NMKA92EB:~/HAMi$
i will fix it and let you know?
Thank you!
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
saiyam1814
left a comment
saiyam1814
left a comment
There was a problem hiding this comment.
Thanks for driving this — the max-vs-sum split for init containers is the right core idea, and it fixes a real over-reservation problem (#1942). One design gap and one PR-hygiene note, neither raised in the existing threads:
1. Restartable init containers (sidecars) break the max-pool assumption.
Since Kubernetes 1.28 (stable in 1.33), an init container with restartPolicy: Always is a sidecar: it keeps running for the pod's entire lifetime, concurrently with app containers. Kubernetes' own effective-request accounting therefore sums sidecars with app containers instead of max-pooling them with regular init containers.
This PR treats all entries of pod.Spec.InitContainers uniformly, which breaks in two places:
podInitContainerMaxRequestmax-pools a GPU-requesting sidecar with the other init containers. A sidecar holds its GPU while the app containers run, so its request must be added to the app-container sum. As written, a pod with a GPU sidecar under-reserves and can overcommit the node — the same class of bug this PR is fixing for the opposite direction.stripInitContainerAliasSlotsis gated on everyInitContainerStatusesentry beingTerminatedwith exit code 0. A sidecar never terminates, so for any pod containing one, the strip never fires — the max-pool reservation for the other (terminated) init containers is held for the pod's lifetime.
Suggestion: given sidecar + GPU is likely rare today, the minimal correct v1 is to detect restartPolicy: Always on GPU-requesting init containers and treat them as app containers in both the accounting (podAppContainerTotalRequest) and the strip logic (never strip their slots, and exclude them from the all-terminated gate). Alternatively, explicitly reject GPU requests in restartable init containers at admission with a clear error and a TODO — silent mis-accounting is the only option that shouldn't ship.
Either way, a test case with restartPolicy: Always would lock the chosen semantics in.
2. Consider splitting the unrelated cleanups out of this PR.
The comment deletions in NewScheduler, the klog.Warning → klog.ErrorS change, and the removal of recordScheduleFilterResultEvent for pods with no HAMi resources are all reasonable on their own, but they're invisible inside a 1.7k-line feature diff — and the last one is a user-visible behavior change (pods no longer get a FilteringFailed event) that deserves its own review. A small separate PR would merge quickly and keep this one focused on the init-container semantics.
@saiyam1814 reminder: pls adhere to the new rules regarding responses (and codes, prs, descriptions). |
7 participants
I fixed the resource accounting logic for pods with Init Containers: the scheduler now correctly calculates the effective request as$max(\sum \text{App Containers}, \text{max}(\text{Init Containers}))$ . This prevents incorrect "node full" errors or quota denials for pods that reuse GPU resources during initialization.Technically, I modernized the codebase for Go 1.22+ by:Using the built-in max() function instead of manual if blocks.Implementing range over integers for cleaner loops.Removing redundant type declarations to satisfy staticcheck.Finally, I updated the unit tests in scheduler_test.go and score_test.go. Since the scoring logic now accurately reflects node utilization under the Binpack strategy, I adjusted the expected device UUIDs (e.g., device1 $\rightarrow$ device3) to match the scheduler's optimized selection. These changes ensure the HAMi scheduler is both logically sound and compliant with modern Go standards.
Fixes :- #1667
Summary by CodeRabbit
Bug Fixes
Tests