docs: design for sidecar container GPU resource accounting - #2584
Conversation
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a design document for HAMi support of Kubernetes native sidecar containers. It defines sidecar classification, resource accounting, admission, scheduling, usage recording, shrink behavior, and ResourceQuota interaction. ChangesNative sidecar support
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to This change adds design documentation for sidecar-container GPU accounting and does not alter product behavior or runtime configuration; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related issues
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/sidecarsContainer-design.md`:
- Around line 52-54: Update the definition of init_peak in the sidecarsContainer
design to explicitly set init_peak[uuid] to 0 when a pod has only sidecar init
containers, and treat missing resource entries as zero before applying max() or
addition.
- Around line 39-40: Add language identifiers to every fenced code block in the
sidecar design documentation, including the blocks near the effective
calculation and the other referenced sections. Use text or another appropriate
identifier after each opening fence while preserving the documented content.
- Around line 37-54: Update the ResourceQuota section to explicitly document
that HAMi’s conservative sidecar simplification can over-reserve compared with
Kubernetes admission when sidecars start after the peak non-sidecar init.
Include the example values showing Kubernetes at 20Gi versus HAMi at 22Gi, and
remove any claim that both systems always agree at admission.
- Around line 79-81: Define and document a positional container resolver before
sidecar classification: map each annotation position to pod.Spec.InitContainers
or pod.Spec.Containers after subtracting the init-container count, then inspect
the resolved container’s RestartPolicy. Update CollapseInitContainerUsage, the
device plugin Allocate flow, and WebUI classification to use this resolver
rather than position alone, and document how WebUI obtains pod.Spec because
annotations do not identify sidecars.
🪄 Autofix
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: c7f21ae3-8050-46ab-9830-7e80699a5043
📒 Files selected for processing (1)
docs/develop/sidecarsContainer-design.md
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
This is being closed because it does not comply with the contribution guidelines. |
|
@mesutoezdil i edited my pr description according to the contribution guidelines , i added the AI disclosure . |
|
@archlitchi @Shouren @DSFans2014 can you please review this PR? |
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:
|
|
/lgtm |
|
[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 |
What type of PR is this?
this is a design docs for HAMi supporting the sidecar Container.
What this PR does / why we need it:
This PR adds a file docs/develop/sidecarContainer-design.md, which contains design for supporting sidecar containers (init containers with
restartPolicy: Always) in vGPU accounting.Right now HAMi treats a sidecar like a normal init container that runs andexits. But a sidecar keeps running next to the app containers for the whole pod lifetime. Because of this, its usage is counted with max instead of being added to the app containers, so the pod's real GPU usage is higher than what HAMi records. Also, the shrink after init never happens for pods
that have a sidecar, so their init container memory is never released.
Which issue(s) this PR fixes:
Part of : #2563
AI disclosure: I used Claude to help find relevant upstream Kubernetes documentation, to improve the formatting of this design document, and to fix spelling and grammar mistakes. The design itself and the analysis of HAMi's current behavior are my own work
Summary by CodeRabbit
Documentation