Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sandbox: Cleanup shim on Start failure #8255

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

dcantah
Copy link
Member

@dcantah dcantah commented Mar 13, 2023

Made a change a bit ago to cleanup the shim on CreateSandbox failures and noted that we should probably do the same on Start as well as nothing gets cleaned up otherwise, and nothing states that a sandbox server/shim should exit itself if Create/Start fail. Ideally this could be hooked up to some subsystem in containerd that'd do it for us, but for now to allow developing/prototyping sandbox shims this makes things much friendlier.

Made a change a bit ago to cleanup the shim on CreateSandbox failures and
noted that we should probably do the same on Start as well as nothing gets
cleaned up otherwise, and nothing states that a sandbox server/shim should
exit itself if Create/Start fail. Ideally this could be hooked up to
some subsystem in containerd that'd do it for us, but for now to allow
developing prototyping sandbox shims this makes things much friendlier.

Signed-off-by: Danny Canter <danny@dcantah.dev>
Error("failed to shutdown sandbox")
}

ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fuweid I can open an issue to make this configurable as I know you had that feedback in the last change, although perhaps if we explore @dmcgowan's idea about trying to wire this up to garbage collection the solution will be more apparent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC sounds good to me!

@dcantah dcantah added this to the 2.0 milestone Mar 13, 2023
@dcantah
Copy link
Member Author

dcantah commented Mar 13, 2023

Interesting, the k8s flows failed the millisecond I opened this and won't let me click on them to view the status 😭

@dcantah dcantah removed this from the 2.0 milestone Mar 13, 2023
@dmcgowan
Copy link
Member

/retest

@dmcgowan
Copy link
Member

@dcantah prow jobs were having some issues yesterday, lets see if they do better today

@dcantah dcantah added the cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch label Mar 15, 2023
@kzys
Copy link
Member

kzys commented Mar 15, 2023

/retest

@estesp estesp merged commit 974da05 into containerd:main Mar 15, 2023
@dmcgowan dmcgowan added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch kind/enhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants