docs: design for init-container GPU resource accounting#2064
docs: design for init-container GPU resource accounting#2064maishivamhoo123 wants to merge 3 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 |
github-actions
Bot
added
the
kind/documentation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdded a design document defining HAMi’s init-container GPU accounting model, including admission checks, scheduler fit and scoring, usage aggregation, post-init shrinking, and idempotency handling. ChangesInit container accounting design
Estimated code review effort: 2 (Simple) | ~10 minutes 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 a comprehensive design document and supporting SVG diagrams detailing HAMi's support for init container GPU resource accounting. The design addresses the issue of simultaneous resource allocation for sequential containers by proposing a formula that calculates the effective GPU footprint as the maximum of the app container requests and the single largest init container request. The review feedback focuses on correcting several typos, grammatical errors, and phrasing issues within the design document to improve its overall readability and professionalism.
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.
|
@DSFans2014 @archlitchi and @Shouren can i added the docs for #1773 can you please review this ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/develop/initContainer-design.md (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced code blocks.
The document has multiple untyped fences, triggering markdownlint MD040. Use
gofor Go snippets andtext(or another appropriate language) for formulas/pseudocode.Also applies to: 27-31, 46-50, 70-72, 134-138, 152-156, 173-175, 200-204
🤖 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 `@docs/develop/initContainer-design.md` around lines 15 - 20, Update all fenced code blocks in initContainer-design.md, including the sections around the shown loop and the additional referenced ranges, to include explicit language identifiers; use go for Go snippets and text (or another suitable identifier) for formulas or pseudocode, without changing the block contents.Source: Linters/SAST tools
🤖 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 `@docs/develop/initContainer-design.md`:
- Around line 208-210: In the idempotency description for PodManager, correct
the typo by changing “never re-applie” to “never re-applied.”
- Around line 76-79: Update the resource-dimension sentence to use “per-device
UUID (a multi-GPU pod...” and replace “those devices usage merged together” with
clearer wording such as “usage on those devices merged.”
- Around line 200-204: Update the initContainer usage-collapse pseudocode so
new_usage[uuid] aggregates the same per-container actual usage fields consumed
by AddUsage and getNodesUsage, including fields such as Usedmem, rather than
app-container requests. Preserve request-based calculations for admission and
scheduling, then compute delta from the usage values and apply it to
QuotaManager and PodManager.
- Around line 76-82: Clarify the GPU-count behavior in the admission quota
design around FitQuota: either specify how GPU count is passed through and
checked as an independent quota dimension, or explicitly document that it is
incorporated into the memory/core multiplier and is not checked separately.
Ensure the documented formula matches the actual admission behavior.
---
Nitpick comments:
In `@docs/develop/initContainer-design.md`:
- Around line 15-20: Update all fenced code blocks in initContainer-design.md,
including the sections around the shown loop and the additional referenced
ranges, to include explicit language identifiers; use go for Go snippets and
text (or another suitable identifier) for formulas or pseudocode, without
changing the block contents.
🪄 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: e77d858d-8e5e-44bd-8cad-029d0e179a4b
⛔ Files ignored due to path filters (3)
docs/develop/imgs/before-after-math.svgis excluded by!**/*.svgdocs/develop/imgs/pod-lifecycle-journey.svgis excluded by!**/*.svgdocs/develop/imgs/timeline-init-vs-app.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
docs/develop/initContainer-design.md
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/develop/initContainer-design.md (1)
202-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBase shrinking on init completion, not
Phase == Running.Succeeded/Failedpods can still be observed after all init containers have finished, so requiringRunningcan skip the shrink and leave init peak usage charged. UseInitialized=True(or an equivalent init-status predicate) as the primary gate and keep phase only as a consistency check.🤖 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 `@docs/develop/initContainer-design.md` around lines 202 - 205, Update the documented init-container completion condition so Initialized=True, or an equivalent predicate confirming all init containers terminated successfully, is the primary gate for base shrinking. Do not require pod.Status.Phase == Running, since Succeeded or Failed pods may still need the shrink; retain phase only as an optional consistency check.
🤖 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 `@docs/develop/initContainer-design.md`:
- Around line 225-227: Update the shrink transition design for both QuotaManager
and PodManager so retries and concurrent execution cannot apply the same delta
twice. Persist a durable shrink operation/version keyed by pod UID and reconcile
absolute desired usage, or have both managers deduplicate the operation before
marking initContainersShrunk complete; retain initContainersShrunk only as
completion state, not the sole idempotency mechanism.
- Line 5: Update the GPU allocation design for init containers to distinguish
classic init containers from native sidecars identified by restartPolicy:
Always. Exclude native sidecars from the sequential init-container assumption
and define their separate resource accounting and shrink behavior when they
overlap app containers; retain the existing sequential handling for classic init
containers.
---
Outside diff comments:
In `@docs/develop/initContainer-design.md`:
- Around line 202-205: Update the documented init-container completion condition
so Initialized=True, or an equivalent predicate confirming all init containers
terminated successfully, is the primary gate for base shrinking. Do not require
pod.Status.Phase == Running, since Succeeded or Failed pods may still need the
shrink; retain phase only as an optional consistency check.
🪄 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: f2eb52d7-4ba9-4995-9bdb-7943eb4944ce
📒 Files selected for processing (1)
docs/develop/initContainer-design.md
|
|
||
| ### Scheduler Fit & Scoring | ||
|
|
||
| 1. **Init pass:** fit each init container independently against a fresh |
There was a problem hiding this comment.
If the reqNum value for all initContainers is 1, will all initContainers be allocated on the same cardID?
There was a problem hiding this comment.
@DSFans2014 thank you for review yes when all init containers have reqNum=1, each independent fresh-copy fit will select the same physical card whose score will be highest on a clean node state.because init containers are sequential, they can genuinely reuse the same device. The per-UUID max formula handles both outcomes correctly: if all land on GPU0, effective_mem[GPU0] = max(IC1, IC2, and so on) .
if they land on different devices, each gets its own independent peak. Neither case over-counts.
am i correct ?
|
|
||
|  | ||
|
|
||
| **Condition:** `pod.Status.Phase == Running` **and** every init container |
There was a problem hiding this comment.
Is there any issue if the main container exits immediately after being started?
There was a problem hiding this comment.
No i think there is no any problem because if the app container exits immediately and the pod goes straight to Succeeded/failed state before our shrink logic runs that's fine. because When the pod gets deleted, we always remove exactly whatever number is stored at that point. So even if the shrink never fired and we still have the full max(app, init) value stored, deletion will remove exactly that. End result is still zero. Nothing leaks. The initContainersShrunk flag just makes sure the shrink can't accidentally fire after deletion has already cleaned everything up.
am i correct ?
There was a problem hiding this comment.
Condition: pod.Status.Phase == Running and every init container has Status.Terminated{ExitCode: 0}
My concern is whether the Running state will appear in this case. I'm not sure whether Kubernetes merges transient intermediate states and sends a single final state.
There was a problem hiding this comment.
@DSFans2014 thank you for review The Running phase will always appear, even if the app container exits immediately after init containers finish. Kubernetes does not merge transient phases.
as we can see in this function
// updateStatusInternal updates the internal status cache, and queues an update to the api server if
// necessary.
// This method IS NOT THREAD SAFE and must be called from a locked function.
func (m *manager) updateStatusInternal(logger klog.Logger, pod *v1.Pod, status v1.PodStatus, forceUpdate, podIsFinished bool) (bool, *podStatusNotification) {
var oldStatus v1.PodStatus
cachedStatus, isCached := m.podStatuses[pod.UID]
if isCached {
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/status/status_manager.go#L849
In updateStatusInternal, the new status is cached, its version is incremented, and a message is pushed onto podStatusChannel to trigger a sync. There is no any logic that skips intermediate phases. So whenever the kubelet starts the first app container, it computes a Running phase and calls SetPodStatus, which immediately triggers a patch to the API. If the container then terminates, a second status (Succeeded) is computed and sent in the same way. Both updates are visible to any informer or controller.
and i verified it locally also :-
The sleep 1 just makes the phase long enough to observe manually.
does it make sense ?
There was a problem hiding this comment.
We can test it as follows. First, start the monitoring script
NS="phase-race"
rv=$(kubectl get pods -n "$NS" -o jsonpath='{.metadata.resourceVersion}' 2>/dev/null)
kubectl get --raw "/api/v1/namespaces/$NS/pods?watch=1&resourceVersion=$rv&allowWatchBookmarks=true" \
| jq -r --unbuffered 'select(.type != "BOOKMARK" and .object != null) | [.type, .object.metadata.name, .object.status.phase] | @tsv' \
| while IFS=$'\t' read -r type name phase; do
timestamp=$(date '+%H:%M:%S.%3N')
echo "[$timestamp] $type | $name | $phase"
done
then start test pod as below:
NS=phase-race
ITER=5
kubectl create ns "$NS" --dry-run=client -o yaml | kubectl apply -f -
for i in $(seq 1 "$ITER"); do
P="race-$i"
cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: Pod
metadata:
name: $P
namespace: $NS
labels:
race: phase
spec:
restartPolicy: Never
initContainers:
- name: init-1
image: busybox:latest
command: ["sh","-c","sleep 0.2"]
- name: init-2
image: busybox:latest
command: ["sh","-c","sleep 0.3"]
containers:
- name: main
image: busybox:latest
command: ["sh","-c","echo start; sleep 0.01; exit 0"]
EOF
kubectl -n "$NS" wait --for=jsonpath='{.status.phase}'=Succeeded pod/"$P" --timeout=120s >/dev/null || true
done
we may see the output as below:
If there are any issues with the testing process, please let me know
There was a problem hiding this comment.
@DSFans2014 thank you for pointing this out , I checked it properly with the help of the above conditions . I ran the test again and watched both Phase and initContainerStatuses together while creating pods live.
For race-1: both init containers show Terminated{ExitCode:0} while the pod is still Pending. Then it jumps straight to Succeeded. Running never shows up at all.
But for race-2 in the same run, Running does show up before Succeeded.
So it's random — same pod spec, but one pod skips Running and the next one doesn't. It depends on timing, not something we can control. That means we can't rely on Phase == Running in the condition, since it might just never happen for a given pod.
so i am thinking to remove pod.Status.Phase == Running so the new condition will now become : all init containers show Terminated{ExitCode:0}. That field is stable once it's set and doesn't depend on this timing issue.
am i correct? can i move forward with this?
There was a problem hiding this comment.
Yeah, looks like Running status doesn’t always show up from this result. We should change the condition
|
|
||
| ## Problem Summary | ||
|
|
||
| When a pod has both init containers and app containers requesting GPU resources, HAMi allocates the resources simultaneously/parallelly. But Kubernetes runs the init container sequentially to completion before any app container starts, so init and app containers never execute at the same time. |
There was a problem hiding this comment.
this thread got marked resolved but sidecar containers are still not mentioned anywhere in the doc, same gap i found in pr 1773
There was a problem hiding this comment.
@mesutoezdil This is totally intentional i will create a different pr for the sidecar container as it have a lot of difference compared to the init container so i am thinking to raise a different issue and pr for sidecar container.
is it ok ?
|
|
||
| Only ever runs **after** `pod.Status` confirms completion. | ||
|
|
||
| **Idempotency:** `PodManager` stores a boolean `initContainersShrunk` flag |
There was a problem hiding this comment.
this thread got marked resolved too but the doc still only describes one boolean flag, no crash safe delta reconciliation across the two managers
There was a problem hiding this comment.
@mesutoezdil the boolean flag is only there to avoid running the same shrink twice while the controller is still running. It’s not the crash-safety mechanism.For crashes, the real safety net is:- every time the controller checks the pod, it will recalculate the correct usage directly from the pod annotations, spec, and status. So if we crash mid-shrink, on restart the next check will recompute the right numbers and apply only the difference (delta) to both managers. The flag is just a temporary inmemory guard that will disappears on restart, so the recalculation fixes everything automatically.
does it make sence?
|
issue 1667 was about wrong device assignment which pr 1650 already fixed, this doc only changes accounting and does not touch device assignment, might be better to change fixes 1667 to part of 1667 |
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@DSFans2014 i resolved all your comments can you please look in to this once and let me know is everything is perfect or not ? |
| So the correct formula for a pod's GPU footprint at any instant is: | ||
|
|
||
| ``` | ||
| effective = max( sum(app container requests), max(single init container request) ) |
There was a problem hiding this comment.
I'm not sure whether a single machine can take different devices. Should we also consider the scenario where a request contains multiple devices? @archlitchi
There was a problem hiding this comment.
yes and no, a single request shouldn't contain multiple kinds of devices, but a single node can have multiple kinds of devices
There was a problem hiding this comment.
a single request shouldn't contain multiple kinds of devices
Can different containers (including init containers ) in the same Pod request different devices? @archlitchi
There was a problem hiding this comment.
@DSFans2014 @archlitchi Can we simplify the scenario for init containers by assuming, for now, that the resources requested by the init container will always be the same as one of the app containers? If other needs come up later, we can implement them then.
There was a problem hiding this comment.
@DSFans2014 @archlitchi Can we simplify the scenario for init containers by assuming, for now, that the resources requested by the init container will always be the same as one of the app containers? If other needs come up later, we can implement them then.
yeah, we can assume this for now
| - **All init containers exited `ExitCode: 0`, pod not yet terminal:** | ||
| app containers have started or are starting → shrink to | ||
| app-containers-only usage. | ||
| - **Any init container exited non-zero, pod not yet terminal:** pod may still restart (depending on restartPolicy). It still holds its allocated GPU devices, so usage is not shrunk at this point. The pod will either restart and eventually succeed, or be terminated permanently (phase → Failed), which will then trigger a shrink. |
There was a problem hiding this comment.
spec:
restartPolicy: OnFailure
initContainers:
- name: init-1
image: busybox
command: ["sh","-c","sleep 0.2"]
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: 1024
- name: init-2
image: busybox
command: ["sh","-c","sleep 0.3; exit 1"]
resources:
limits:
nvidia.com/gpu: 1
nvidia.com/gpumem: 512
containers:
...
Considering the above scenario—where init-1 has completed successfully and init-2 is stuck in a restart loop due to failures (may be waiting for some condition to be met before it can succeed) —should the extra resources claimed by init-1 be released?
There was a problem hiding this comment.
@DSFans2014 currently we are using the all-or-nothing approach: if any init container fails, we don't shrink usage at all. But you're right we are not releasing the extra resource use by init-1. I think we can improve this with the watermark approach track the highest index of an init container that finished with exit 0 (since they run in order). After that, we only count the requests of the remaining init containers. In your example, once init-1 succeeds, the effective peak drops from 1024Mi to 512Mi (only init-2 matters). It's a bit more state to track (an index instead of a simple enum), but it's a utilization improvement, not a correctness fix — we'd never over-admit either way.
@archlitchi @Shouren @DSFans2014 What are your thoughts? Should we include this now, or keep it as a future improvement?
Thank you!
There was a problem hiding this comment.
I don't think this a critical problem. We can record a TODO item and address it in a future iteration.
| Only ever runs **after** `pod.Status` confirms completion. | ||
|
|
||
| **Idempotency:** `PodManager` stores a `PodShrinkState` per pod — | ||
| `NotShrunk`, `AppOnly`, or `Zero` — replacing a single boolean, since the |
There was a problem hiding this comment.
What are these statuses used for? Also, the names are a bit confusing
There was a problem hiding this comment.
@DSFans2014 PodShrinkState guards against two specific problems. First, idempotency: the system checks the same pod over and over, and once it shrinks the usage, it must recognize the shrink is already done on subsequent checks so it doesn't subtract the same memory twice. Second, one-directional transitions: usage should only ever go down from full to app-only, or full straight to zero but never back up.
A simple hasShrunk boolean cannot handle this because it only tracks whether any shrink happened. Once the first shrink (full to app-only) occurs, the boolean flips to true. Later, when the pod dies and a second shrink (app-only to zero) is needed, the system looks at the true boolean, assumes everything is already handled, and skips the second shrink leaving the pod's memory permanently stuck at the app-only level even after the pod is gone.
What each value means
NotShrunk : still the original max(app_sum, init_peak) from admission.
AppOnly : init containers all exited 0 recorded usage = app containers only.
Zero : pod hit a terminal phase recorded usage = 0.
i guess now it is clear ? there is a scope of changing name from Zero to Released other than this is looking fine i am open for any suggestions if you have any suggestions please let me know?
Thank you !
There was a problem hiding this comment.
First, idempotency: the system checks the same pod over and over, and once it shrinks the usage, it must recognize the shrink is already done on subsequent checks so it doesn't subtract the same memory twice.
If we address the issue mentioned in #2064 (comment) in the future, how should the state transition?
There was a problem hiding this comment.
@DSFans2014 in that condition Instead of using a fixed three state flag, we can track a single number called the watermark , basically a counter that keeps track of how many init containers have successfully finished in the order.
this NotShrunk is just watermark 0, and AppOnly is just the final watermark number (when all inits have succeeded). The big win is the middle ground , lets take your example only when init‑1 (1024Mi) succeeds, the watermark moves from 0 to 1, and we immediately release that 1024Mi the effective usage drops to only what the remaining init containers (like init‑2) are asking for. If init‑2 keeps failing and retrying, the watermark stays at 1 and no further shrink happens until either init‑2 finally succeeds (moving the watermark to the top, which then includes app containers) or the whole pod hits a terminal phase and jumps straight to Zero/Released, releasing everything. This watermark only ever moves forward, one step at a time, never backward, and since it's recomputed from the pod's actual status on every check, repeated reconciliations without any status change naturally produce zero delta — so our idempotency stays intact. so we can safely keep it as a future TODO without reworking anything in this PR.
is it a good approach or do you have any further suggestions?
There was a problem hiding this comment.
From my point of view, we only need one bool variable to track whether the init container's resources have been reclaimed. Moreover, the problems in #2064 (comment) can be resolved by the watermark number described above.
// We use initContainerResourceReleased to indicate whether the init container's resources have been released
// with an initial value of false
// If all init containers have completed and initContainerResourceReleased remains false
// we reclaim the init container resources
if all initContainer terminated and !initContainerResourceReleased:
// release the resource of initContainer
initContainerResourceReleased = true
// If the pod terminates, we free up the app containers resources.
if pod.Status.Phase in (Succeeded, Failed):
// release the resource of app containers
If there are any issues with the above solution, feel free to point them out.
5 participants
Currently HAMi sums init and app container requests as if they run together, causing false quota passes, scheduler rejections, and inflated usage that never shrinks. This PR corrects all three layers – admission, scheduling, and usage recording – to use the correct formula effective = max(sum(app), max(init)). Usage is stored as the peak value and automatically reduced to app‑only once init containers terminate. Pod annotations remain untouched.
Related to #1667
#1773
Summary by CodeRabbit