shim cleanup on exit #1316
Draft
shim cleanup on exit #1316
+78
−78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sad day. Can logrus even write a message after this point? I assume anything that happens in the shim post this call now has no way of being logged anywhere? Maybe thats ok since its such a small window now? |
|
It should be able to, since its normally set up with ETW or a logging binary (via a pipe). Local testing works fine too. I figured closing errors could safely be ignored, but I can log them if you think they're worthwhile |
Fixed bug introduced by earlier PR where graceful shim shutdown prevents containerd from deleting the sandbox bundle since the shim is still using it and containerd errors, because another process is using that directory and the files within it. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
|
Nah, this seems fine. |
| // alive. | ||
| // Change the directory to the parent and close stderr (panic.log) to | ||
| // allow bundle deletion to succeed. | ||
| if err := os.Chdir(".."); err != nil { |
There was a problem hiding this comment.
Something in me is screaming that we shouldn't do this, but I don't know why.
helsaawy
force-pushed
the
he/close
branch
2 times, most recently
from
8101d00 to
b9508da
Compare
Rather than changing the directory, have the ttrpc wait until cleanup is done Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed bug introduced by PR #1289 where graceful shim shutdown prevents containerd from deleting the sandbox bundle since
the shim is still using it and containerd errors, because anotherprocess is using that directory and the files within it.
containerd waits for the shim to close its ttrpc server before marking it as closed and cleaning up resources, such as the sandbox bundle. However, now the shim shuts down the ttrpc server and then exits gracefully, rather than executing an
os.Exit(0), causing a timing error, where the shim is still alive, with the sandbox bundle as its working directory and thepanic.logfile within the bundle as itsstderr, and contaienrd is attempting to delete the sandbox bundle directory that another process (the shim) is using.The PR changes the working directory and closes the
stderrof the shim during exit, before the ttrpc server is closed, so the bundle can be deleted while the shim finishes shutting down.Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com