docs: design for init-container GPU resource accounting#2064
docs: design for init-container GPU resource accounting#2064maishivamhoo123 wants to merge 2 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
📝 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 ?
2 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.
Fixes #1667
#1773
Summary by CodeRabbit