Skip to content

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 <[email protected]>
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