common: add ssh.ExecWithOutput to allow streaming output#865
common: add ssh.ExecWithOutput to allow streaming output#865danishprakash wants to merge 1 commit into
Conversation
Follow-up to 2681204 that added ExecWithInput allowing executing commands over ssh while passing an optional input reader. For cases where remote > local output needs to be streamed, add a similar ExecWithOutput that returns a ReadCloser. Signed-off-by: Danish Prakash <contact@danishpraka.sh>
|
Thanks! (I didn’t read the code ~at all.) Is there an expected user in the Podman ecosystem? |
|
Ah, yes, I should've mentioned this, but this is a requirement for implementing support for podman-container-tools/podman#28321. For supporting *-dir formats between remote to local/remote ie "save image on remote host and stream it back". I'm yet to raise the PR on podman, which uses this. |
|
|
||
| output := &bytes.Buffer{} | ||
| cmd.Stdout = output | ||
| if err := cmd.Run(); err != nil { |
There was a problem hiding this comment.
AFAICS this could use cmd.Output() without having to implement the “collect stdout” and “include stderr on failure” parts; so nativePrepareSSHCmd can have a simpler interface.
| } | ||
|
|
||
| func (r *nativeExecOutputReader) Close() error { | ||
| err := r.cmd.Wait() |
There was a problem hiding this comment.
This can work but it requires the caller to check errors on a ReadCloser.Close, which most ReadCloser consumers don’t bother to do.
I think doing this in Read at the point r.stdout.Read hits an EOF would be more reliable, admittedly at the cost of some complexity.
If this design remained, at the very least the API would need a fairly loud documentation that the caller must check errors on .Close().
There was a problem hiding this comment.
You're right; I didn't think of usage semantics. I'm a bit unsure of waiting inside Read() that doesn't really scream idiomatic Go to me but I could be wrong. On the other hand, expecting users to check errors on Close() is also fragile as you mentioned.
I found that using io.Pipe might be suitable. With that, we use let the process write stdout to the write end of the pipe and we start the command in a goroutine while instantly returning the read end of the pipe.
func nativeConnectionExecWithOutput(...) (io.ReadCloser, error) {
cmd, _ := nativePrepareSSHCmd(options, input)
stderr := &bytes.Buffer{}
cmd.Stderr = stderr
r, w := io.Pipe()
cmd.Stdout = w
go func() {
err := cmd.Run()
if err != nil {
err = fmt.Errorf("%v: %w", stderr.String(), err)
}
w.CloseWithError(err)
}()
return r, nil
}
CloseWithError ensures that upon an error, the next read on the pipe, the caller would receive the error so effectively moving error propogation from Close to Read.
| if !strings.HasPrefix(options.Host, "ssh://") { | ||
| options.Host = "ssh://" + options.Host | ||
| } | ||
| _, uri, err := Validate(options.User, options.Host, options.Port, options.Identity) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cfg, err := ValidateAndConfigure(uri, options.Identity, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dialAdd, err := ssh.Dial("tcp", uri.Host, cfg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect: %w", err) | ||
| } |
There was a problem hiding this comment.
I think sharing / not duplicating at least this part would be valuable. We already have 5 callers of ssh.Dial here in 3 different patterns — one of the differences looks justifiable, but it’s unclear.
I’m not at all asking for cleaning up the existing differences — but consolidating the existing code where it is common, and calling a shared helper, instead of adding another copy that can diverge over time would be valuable.
| // safe to close the connection now that the session is done | ||
| // and the exit status has been received. |
There was a problem hiding this comment.
I’m not sure what this is saying, is it somehow unusual / unexpected?
Perhaps, instead, the golangExecOutputReader.sess field could be documented to exist within client to make the relationship / lifetime explicit there.
|
|
||
| func (r *golangExecOutputReader) Close() error { | ||
| r.sess.Close() | ||
| err := <-r.waitCh |
There was a problem hiding this comment.
Also probably safer to do this in Read when it hits EOF.
| } | ||
|
|
||
| func (r *nativeExecOutputReader) Close() error { | ||
| err := r.cmd.Wait() |
There was a problem hiding this comment.
If the caller fails due to an unrelated error and calls Close early, couldn’t this deadlock? We never consume the rest of r.stdout, the child process could block on writing to its stdout, Wait will never return.
There was a problem hiding this comment.
Yes, one option is for us to kill the process on Close, or to provide the command under a cancellable context and the caller then calls cancel()s on an early close.
|
|
||
| func (r *golangExecOutputReader) Close() error { | ||
| r.sess.Close() | ||
| err := <-r.waitCh |
There was a problem hiding this comment.
See elsewhere, this could hang on an early Close().
|
Yes, I think it's valuable. |
3 participants
Follow-up to 2681204 that added ExecWithInput, allowing executing commands over SSH while passing an optional input reader. For cases where remote > local output needs to be streamed, add a similar ExecWithOutput that returns a ReadCloser.