Add defer in event of mid-function failures in RunPodSandbox to avoid mount leaks#13399
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds rollback cleanup after a sandbox has been started so failed RunPodSandbox execution does not leave sandbox resources running.
Changes:
- Adds a deferred post-start rollback that attempts to stop the sandbox when
RunPodSandboxreturns an error.
Comments suppressed due to low confidence (2)
internal/cri/server/sandbox_run.go:321
- This logs the outer
errinstead of thederrreturned byStopSandbox, so the rollback failure can be reported with the wrong error (or no error at all after later short-variable returns). That makes diagnosing failed cleanup unreliable.
log.G(ctx).WithError(err).Errorf("failed to stop sandbox %q on rollback", id)
internal/cri/server/sandbox_run.go:320
- Stopping the sandbox is not enough for this rollback path: after this defer runs, the existing cleanup deletes the CRI/core sandbox metadata and releases the lease, so there is no later
RemovePodSandboxpath to callShutdownSandbox. For sandbox controllers whereStoponly stops the instance andShutdowndeletes the shim/sandbox resources, this can still leave an orphaned stopped sandbox/shim even though the metadata has been removed.
if derr := c.sandboxService.StopSandbox(deferCtx, sandbox.Sandboxer, id); derr != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Stop the sandbox on any upcoming NRI hook failures. | ||
| defer func() { |
There was a problem hiding this comment.
I have a gemini written test for this in another branch that is fine, but its very specific to this rollback and I think the NRI tests from the NRI branch in cri-tools tests this more meaningfully, even if its more coarsely. Overall the many rollback variants for this function (e.g. sandbox rollsback but networking doesn't; sandbox doesn't and networking sees the bad cleanupErr and so it doesn't; etc etc) don't have direct unit test coverage. Anyways that's all to say I'm looking for an IRL person to advise on this.
There was a problem hiding this comment.
Discussion offline:
- Could use failpoint integration system to trigger NRI plugin failure(s) and then test if we had a mount leak/rollback properly.
- Can check what mounts there are or read /proc/mount (before/after check is the easiest option)
- Not a super pressing need to have unit tests for this/all of the defer unrolls, as the integration test above is a better behavior check
There was a problem hiding this comment.
Discussed IRL with @samuelkarp not to add these suggested unit tests against this behavior in this PR
50fc5d8 to
a8df1bb
Compare
644fd21 to
e9caa79
Compare
lauralorenz
changed the title
| // Stop the sandbox if we fail before adding it to store. | ||
| rollbackSandbox := true | ||
| defer func() { | ||
| if retErr != nil && rollbackSandbox { | ||
| deferCtx, deferCancel := util.DeferContext() | ||
| defer deferCancel() | ||
| cleanupErr = c.sandboxService.StopSandbox(deferCtx, sandbox.Sandboxer, id) |
There was a problem hiding this comment.
sandboxService.StopSandbox alone will call cleanupSandboxFiles (like this one) which I think is why that solves the problem as described in #13355; I replaced it wholesale for sandboxService.ShutdownSandbox which will both stop the sandbox container and clean up its entire root and state directories as well.
e9caa79 to
c30f478
Compare
lauralorenz
changed the title
|
/retest |
|
@lauralorenz: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Between starting the sandbox and adding it to the sandbox store, there are opportunities for failures including in any NRI RunPodSandbox prehooks. This defer is added to that period so if they fail, this function will try to clean it up itself. If the sandbox is already added to the persistent store, it will not attempt to stop the sandbox as it can now be recognized by other components from the CRI store. ShutdownSandbox is used instead of StopSandbox as it both stops it and cleans up all its directories. Signed-off-by: lauralorenz <[email protected]>
|
The failing CI checks are mostly Vagrant integration (there is one kubernetes node e2e one fwiw), vary on OS/platform, vary by which test is failing, and there is one successful run of this workflow against this branch, so I'm inclined to consider these flakes. I'm sending an empty amend to kick it 
5/13 5/14 2 3 5/15 |
c30f478 to
2b2b80f
Compare
|
/ok-to-test |
|
/retest |
github-project-automation
Bot
moved this from Needs Triage
to Review In Progress
in Pull Request Review
github-project-automation
Bot
moved this from Review In Progress
to Done
in Pull Request Review
samuelkarp
added
cherry-pick/2.2.x
|
/cherrypick release/2.3 |
|
@samuelkarp: new pull request created: #13644 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@samuelkarp: new pull request created: #13645 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
samuelkarp
added
cherry-picked/2.2.x
Add a defer with
StopSandboxShutdownSandbox in it before we start the RunPodSandbox hooks so if there is a failure in one of them, the mounts are still cleaned up. This rollback defer is active for any failures between the StartSandbox it directly follows and the successful addition of the sandbox to the sandbox store.Before this PR / bad
After this PR / good