Set stderr to empty string when using terminal on Windows#10412
Set stderr to empty string when using terminal on Windows#10412samuelkarp merged 1 commit intocontainerd:mainfrom
Conversation
|
Hi @TinaMor. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
My main concern with this is if we are making it harder to troubleshoot any issues between containerd->shim->runhcs |
pkg/cio/io.go
Outdated
| // TerminalBinaryIO forwards container STDOUT|STDERR directly to a logging binary | ||
| // It also sets the terminal option to true | ||
| func TerminalBinaryIO(binary string, args map[string]string) Creator { | ||
| func TerminalBinaryIO(binary string, args map[string]string, setStderr ...bool) Creator { |
There was a problem hiding this comment.
We should probably just have a model similar to NewFIFOSetInDir where this is already handled. The unix version (and this is implying that we should split this into OS specific versions) doesn't do anything fancy, and the windows version has a special case for the terminal bool we pass in.
Unix:
// NewFIFOSetInDir returns a new FIFOSet with paths in a temporary directory under root
func NewFIFOSetInDir(root, id string, terminal bool) (*FIFOSet, error) {
if root != "" {
if err := os.MkdirAll(root, 0700); err != nil {
return nil, err
}
}
dir, err := os.MkdirTemp(root, "")
if err != nil {
return nil, err
}
closer := func() error {
return os.RemoveAll(dir)
}
return NewFIFOSet(Config{
Stdin: filepath.Join(dir, id+"-stdin"),
Stdout: filepath.Join(dir, id+"-stdout"),
Stderr: filepath.Join(dir, id+"-stderr"),
Terminal: terminal,
}, closer), nil
}Windows:
// NewFIFOSetInDir returns a new set of fifos for the task
func NewFIFOSetInDir(_, id string, terminal bool) (*FIFOSet, error) {
stderrPipe := ""
if !terminal {
stderrPipe = fmt.Sprintf(`%s\ctr-%s-stderr`, pipeRoot, id)
}
return NewFIFOSet(Config{
Terminal: terminal,
Stdin: fmt.Sprintf(`%s\ctr-%s-stdin`, pipeRoot, id),
Stdout: fmt.Sprintf(`%s\ctr-%s-stdout`, pipeRoot, id),
Stderr: stderrPipe,
}, nil), nil
}There was a problem hiding this comment.
For backward compatibility, we want to avoid introducing a new required argument that could potentially disrupt other applications using the method.
cc. @samuelkarp
There was a problem hiding this comment.
If the function always sets terminal to true, and on windows we require that stderr is not filled in, why do we need a new parameter at all? The Windows version of the method can assign stderr to empty string, and unix can get away with whatever it does today I'd think.
|
Can you preface in the title this is mainly for windows? |
The current implementation redirects all output to a single stream, unifying stderr and stdout. Lines 261 to 262 in 1fb1882 This already matches Windows' default behavior, which combines both streams. However, Windows HCSShim requires that stderr is an empty string and is designed to check this specific precondition. If it detects that stderr is not an empty string, it will fail the operation. |
TinaMor
changed the title
TinaMor
force-pushed
the
tinamor/dev
branch
4 times, most recently
from
83d5757 to
5e91c7a
Compare
Windows HCSShim requires that stderr is an empty string when using terminal. Reference: https://github.com/microsoft/hcsshim/blob/200feabd854da69f615a598ed6a1263ce9531676/cmd/containerd-shim-runhcs-v1/service_internal.go#L127 Signed-off-by: Christine Murimi <[email protected]>
samuelkarp
changed the title
|
/ok-to-test |
|
/retest |
[Windows] Set stderr to empty string when using terminal on Windows
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
|
@mikebrow when you are free :-) |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
samuelkarp
changed the title
8 participants
PR Description
Windows requires that stderr is an empty string when running a container with TTY. In Containerd, pkg/cio/io.go#L298:L302, the Stderr is set to a value, e.g.
which causes microsoft/hcsshim service_internal.go#L127 to fail to create a new task.
To address this issue, we refactor
TerminalLogURI()andTerminalBinaryIOfunctions by adding an optional parameter that specifies whether stderr should be set to an empty string when the terminal is used. This change will help avoid the "failed precondition" error when creating a new task on Windows.Bug reference:
#10415 : Refactor TerminalLogURI() to Support Optional Stderr Configuration for Terminal Compatibility on Windows
Related issues:
nerdctl bug #2966