This repository was archived by the owner on Dec 13, 2018. It is now read-only.
Report child error better (and later)#22
Closed
alexlarsson wants to merge 1 commit intodocker-archive:masterfrom
Closed
Report child error better (and later)#22alexlarsson wants to merge 1 commit intodocker-archive:masterfrom
alexlarsson wants to merge 1 commit intodocker-archive:masterfrom
Conversation
namespaces/init.go
Outdated
Contributor
There was a problem hiding this comment.
Any reason not to let CLOEXEC handle the closing, and really notify the parent that everything has started? This would have the desirable effect of also reporting errors from the following steps to the parent.
FinalizeNamespace would need to be tweaked not to close the pipe to support this, though.
We use a unix domain socketpair instead of a pipe for the sync pipe, which allows us to use two-way shutdown. After sending the context we shut down the write side which lets the child know it finished reading. We then block on a read in the parent for the child closing the file (ensuring we close our version of it too) to sync for when the child is finished initializing. If the read is non-empty we assume this is an error report and fail with an error. Otherwise we continue as before. This also means we're now calling back the start callback later, meaning at that point its more likely to have succeeded, as well as having consumed all the container resources (like volume mounts, making it safe to e.g. unmount them when the start callback is called). Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
Contributor
Author
|
New version that relies on the close-on-exec to close the pipe, which means we report more errors. Also fixed the empty line nit. |
Contributor
Author
|
Note: This works because we merged the change to make FinalizeNamespace set cloexec rather than close fds. |
Contributor
|
@crosbymichael @vmarmol review please :) |
Contributor
There was a problem hiding this comment.
nit: ReadFromChild similar to below for parent
Contributor
|
I'll take this PR and rebase |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
We use a unix domain socketpair instead of a pipe for the sync pipe,
which allows us to use two-way shutdown. After sending the
context we shut down the write side which lets the child know
it finished reading.
We then block on a read in the parent for the child closing the file
(ensuring we close our version of it too) to sync for when the child
is finished initializing. If the read is non-empty we assume this
is an error report and fail with an error. Otherwise we continue as
before.
This also means we're now calling back the start callback later,
meaning at that point its more likely to have succeeded, as well as
having consumed all the container resources (like volume mounts,
making it safe to e.g. unmount them when the start callback is
called).
Docker-DCO-1.1-Signed-off-by: Alexander Larsson alexl@redhat.com (github: alexlarsson)