fix(webhook) : Multi-Container & InitContainer Mutating Webhook Test - #2495
fix(webhook) : Multi-Container & InitContainer Mutating Webhook Test#2495aniket866 wants to merge 9 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aniket866 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe webhook now processes init containers during device admission mutation and quota calculation. Tests cover GPU patches, quota precedence, quota rejection, and CPU-only pods. The change also adds an invalid duplicate ChangesWebhook init-container support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdmissionWebhook
participant Device
participant QuotaManager
AdmissionWebhook->>Device: mutate init container
Device-->>AdmissionWebhook: return patches and detected resources
AdmissionWebhook->>QuotaManager: validate maximum init-container demands
QuotaManager-->>AdmissionWebhook: return quota decision
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/scheduler/webhook_test.go (1)
1153-1179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the patched pod contents.
The path checks pass for any patch below these container paths. They do not verify the required resource requests,
CUDA_VISIBLE_DEVICES, orLD_PRELOAD. They also do not verify thatsidecar-loggingremains unmodified.Apply
resp.Patchesto the input pod and assert the exact mutations forinit-downloaderandapp-main. Assert thatsidecar-logginghas no GPU resources or injected device environment variables.🤖 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_test.go` around lines 1153 - 1179, Extend the test around the webhook response and resp.Patches to apply the patches to the input pod, then assert the exact expected resource requests and CUDA_VISIBLE_DEVICES and LD_PRELOAD environment mutations for init-downloader and app-main. Also verify sidecar-logging remains unchanged, with no GPU resources or injected device environment variables, while retaining the existing patch-path checks.
🤖 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/webhook.go`:
- Around line 154-161: Update fitResourceQuota to track init-container resource
requests separately from application-container totals, retaining the maximum
init-container request per resource instead of adding it to memoryReq and
coresReq. Before calling FitQuota, pass the larger effective request for each
resource: the init-container maximum or the application-container sum.
---
Nitpick comments:
In `@pkg/scheduler/webhook_test.go`:
- Around line 1153-1179: Extend the test around the webhook response and
resp.Patches to apply the patches to the input pod, then assert the exact
expected resource requests and CUDA_VISIBLE_DEVICES and LD_PRELOAD environment
mutations for init-downloader and app-main. Also verify sidecar-logging remains
unchanged, with no GPU resources or injected device environment variables, while
retaining the existing patch-path checks.
🪄 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: b6a7a00a-69fa-47fa-9cbd-2bdebbeb31e9
📒 Files selected for processing (3)
CHANGELOG.mdpkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/webhook_test.go`:
- Around line 1183-1424: Update TestFitResourceQuotaInitContainers to initialize
the NVIDIA backend independently of TestFitResourceQuota, then register
t.Cleanup handlers restoring config.SchedulerName, device.DevicesMap, and
device.DevicesToHandle. Ensure each quota-denial subtest uses an isolated
namespace or restores its cache.Quotas entry so targeted and repeated test runs
do not leak device or quota state.
- Around line 1184-1292: Update the two fitResourceQuota test cases to configure
explicit resource-quota limits that distinguish the aggregation rule: the
init-larger-pod case should pass when limits match the maximum init-container
requirements plus regular-container totals, but fail under summed init
requirements; the container-larger-pod case should fail when limits are below
the regular-container sum despite exceeding the init maximum. Assert the
expected outcomes through the existing fitResourceQuota helper.
🪄 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: 68c46d14-1ce6-466b-a056-96b7524ebf6d
📒 Files selected for processing (1)
pkg/scheduler/webhook_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/device/quota.go`:
- Around line 48-50: Update GetLocalCache’s lazy quota initialization to return
NewQuotaManager() directly when localCache.Quotas is nil, ensuring
initialization remains synchronized through the existing sync.Once in
NewQuotaManager and avoiding an unsynchronized read/write during first
initialization.
🪄 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: 2aae3331-6ac2-4981-8d4b-d13c688492af
📒 Files selected for processing (1)
pkg/device/quota.go
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
…tainers Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
…il map assignment panic Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
27b1e28 to
22ba68f
Compare
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
…and prevent nil quota map access Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
fixed |
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
aniket866
changed the title
|
You can view the relevant rule here. |
2 participants
Closes #2490
MutateValidation/ Webhook Pod PatchingCUDA_VISIBLE_DEVICES,LD_PRELOAD) might only be injected into the first container, leaving secondary or initContainers unconfigured or causing duplicate JSON patches that fail pod creation.webhook_test.gopassing multi-container Pod specs to verify precise JSON patch generation for each container index.Architecture Flow: Before vs After Test Coverage
flowchart LR subgraph Before["Before: Untested & Vulnerable Flow"] direction TB B_Mutate["Mutating Webhook Request\n(Multi-Container / InitContainer)"] B_Parser["MutateValidation\n(webhook.go)"] B_FirstOnly["Injects container[0] Only"] B_DupPatch["Duplicate JSON Patch Conflict"] B_Fail["Pod Startup Failure / Unconfigured GPU"] B_Mutate --> B_Parser B_Parser -->|Single Index Assumption| B_FirstOnly B_Parser -->|Patch Collision| B_DupPatch B_FirstOnly --> B_Fail B_DupPatch --> B_Fail end Before ==>|webhook_test.go Patch Validation| After subgraph After["After: Validated & Hardened Flow"] direction TB A_Mutate["Mutating Webhook Request\n(Multi-Container / InitContainer)"] A_Test["webhook_test.go Suite"] A_Loop["Indexed Container & InitContainer Iteration"] A_Patch["Precise Per-Container JSON Patches"] A_Success["Multi-Container GPU Injection Validated"] A_Mutate --> A_Test A_Test --> A_Loop A_Loop --> A_Patch A_Patch --> A_Success end classDef danger fill:#fee2e2,stroke:#ef4444,stroke-width:2px,color:#991b1b; classDef success fill:#dcfce7,stroke:#22c55e,stroke-width:2px,color:#166534; classDef neutral fill:#f3f4f6,stroke:#4b5563,stroke-width:1.5px,color:#1f2937; class B_FirstOnly,B_DupPatch,B_Fail danger; class A_Patch,A_Success success; class B_Mutate,B_Parser,A_Mutate,A_Test,A_Loop neutral;Summary by CodeRabbit
Bug Fixes
Tests