internal/cmd: Close individual IO pipes when the relay finishes #1025
Conversation
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
dcantah
reviewed
May 12, 2021
dcantah
reviewed
May 12, 2021
dcantah
reviewed
May 12, 2021
dcantah
reviewed
May 12, 2021
msscotb
approved these changes
May 13, 2021
Contributor
msscotb
left a comment
msscotb
left a comment
There was a problem hiding this comment.
lgtm pending the jobcontainer and localprocess commnet
kevpar
force-pushed
the
close-stdio
branch
3 times, most recently
from
84bd2fa to
81f652e
Compare
Member
Author
|
I added a change to set |
dcantah
reviewed
May 13, 2021
internal/jobcontainers/process.go
Outdated
| return p.stdout.Close() | ||
| } | ||
|
|
||
| // CloseStderr closes the stdin pipe of the process. |
The shim is expected to close its end of the IO pipes from the gcs when it is done using them. This is done to ensure that no data is left buffered in the pipes on the gcs's end. Previously, this was accomplished via the ioChannel closing its underlying connection if Read returned EOF. However, this is not sufficiently robust, as it will not work in cases where the shim's IO relay breaks on the write end (e.g. if CRI has gone away). To resolve this, we now expose individual methods on cow.Process to close each IO pipe (in/out/err), and call those from the Cmd implementation once the IO relay completes. This should be a good first-pass fix here, until we can apply some more focused cleanup to the IO relay code in the future. Some minor renaming/cleanup as well. Signed-off-by: Kevin Parsons <[email protected]>
Contributor
|
lgtm again, thanks! |
anmaxvl
pushed a commit
to anmaxvl/hcsshim
that referenced
this pull request
Aug 19, 2021
Related work items: microsoft#388, microsoft#389, microsoft#393, microsoft#394, microsoft#395, microsoft#396, microsoft#397, microsoft#398, microsoft#399, microsoft#400, microsoft#401, microsoft#402, microsoft#403, microsoft#404, microsoft#405, microsoft#931, microsoft#973, microsoft#1001, microsoft#1003, microsoft#1004, microsoft#1005, microsoft#1006, microsoft#1007, microsoft#1009, microsoft#1010, microsoft#1012, microsoft#1013, microsoft#1014, microsoft#1015, microsoft#1016, microsoft#1017, microsoft#1019, microsoft#1021, microsoft#1022, microsoft#1024, microsoft#1025, microsoft#1027, microsoft#1028, microsoft#1029, microsoft#1030, microsoft#1031, microsoft#1033
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
internal/cmd: Close individual IO pipes when the relay finishes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
The shim is expected to close its end of the IO pipes from the gcs when
it is done using them. This is done to ensure that no data is left
buffered in the pipes on the gcs's end. Previously, this was
accomplished via the ioChannel closing its underlying connection if Read
returned EOF.
However, this is not sufficiently robust, as it will not work in cases
where the shim's IO relay breaks on the write end (e.g. if CRI has gone
away).
To resolve this, we now expose individual methods on cow.Process to
close each IO pipe (in/out/err), and call those from the Cmd
implementation once the IO relay completes.
This should be a good first-pass fix here, until we can apply some more
focused cleanup to the IO relay code in the future.
Some minor renaming/cleanup as well.
Signed-off-by: Kevin Parsons [email protected]