fix(sandbox): guard against empty containers in inplace update#374
fix(sandbox): guard against empty containers in inplace update#374rakshaak29 wants to merge 2 commits into
Conversation
Signed-off-by: rakshaak29 <rakshaak29@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Welcome @rakshaak29! It looks like this is your first PR to openkruise/agents 🎉 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #374 +/- ##
==========================================
+ Coverage 75.43% 75.64% +0.21%
==========================================
Files 143 144 +1
Lines 10311 10406 +95
==========================================
+ Hits 7778 7872 +94
+ Misses 2197 2194 -3
- Partials 336 340 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…plateRef claims Signed-off-by: rakshaak29 <rakshaak29@gmail.com>
Ⅰ. Describe what this PR does
This PR fixes a critical runtime panic that occurs during Sandbox in-place updates. If a
ClaimSandboxrequest is issued withInplaceUpdateparameters (likeImageorResources), the controller invokesSetImageorSetResources. Previously, these methods assumed that any non-nil template would inherently contain at least one container and blindly accessedContainers[0], leading to an index out of bounds panic if the template's container list was empty.Changes included:
len(s.Spec.Template.Spec.Containers) > 0guards toSetImageandGetImageinsandbox.go.modifyPickedSandbox(claim.go) that returns a clear, human-readable error if an inplace update is attempted on a sandbox without containers.claim_test.go(TestModifyPickedSandboxCPUNilTemplate) to expect the newly introduced validation error rather than falsely assuming a no-op success.Ⅱ. Does this pull request fix one issue?
#373
Ⅲ. Describe how to verify it
/sandboxesorTryClaimSandboxdirectly.InplaceUpdateoption (e.g.imageparameter).cannot apply inplace update: sandbox <ns>/<name> template has no containersinstead of crashing thesandbox-managerprocess.go test ./pkg/sandbox-manager/infra/sandboxcr/... -count=1.Ⅳ. Special notes for reviews
The
TestModifyPickedSandboxCPUNilTemplatetest was updated fromrequire.NoErrortorequire.Errorsince attempting an in-place update on a nil/empty template is structurally invalid and should fail fast rather than silently ignoring the update request.