docs: design for init-container GPU resource accounting - #2064
Conversation
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
github-actions
Bot
added
the
kind/documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 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: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
|
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 ? |
…Released Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
@DSFans2014 I've applied all the suggested changes , Could you please take another look? Also, let me know if the design is ready for me to start coding, or if anything else i need to change. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, maishivamhoo123 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
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
Summary by CodeRabbit