Redirect command Stdout & Stderr for command_runner

[lichen2013]

CombinedOutput waits command complete. When we run a long run command, such as continuously get new log entries, it failed to get run results.

Fixes: #2447

[minikube-bot]

Can one of the admins verify this patch?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lichen2013
We suggest the following additional approver: luxas

Assign the PR to them by writing /assign @luxas in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[dlorenc]

@minikube-bot ok to test

[dlorenc]

CombinedOutput also gets us Stderr, right? I think if you add that this should be fine.

[lichen2013]

Updated with another approach, and redirected both Stdout & Stderr

[lichen2013]

Did a little more check, I have a guess that issue #2447 might not only exists for exec_runner.
Maybe a better fix should be update the function GetClusterLogs in CommandRunner interface ?

• allow os.Stdout or *bytes.Buffer to be set as an input
• no need to have the string return

[dlorenc]

This function is meant to return the combined output. That's the string parameter in the output.

I think we need to do something like you had before, where we capture both stdout and stderr as variables, then run the command and return the combined strings.

[lichen2013]

For a long running command, like continues printing new logs, it is not possible to get the return strings.

I would like to propose the change like this:

func (*ExecRunner) CombinedOutputTo(cmd string, out io.Writer) error {
        glog.Infoln("Run with output:", cmd)
        c := exec.Command("/bin/bash", "-c", cmd)
        c.Stdout = out
        c.Stderr = out
        err := c.Run()
        if err != nil {
                return errors.Wrapf(err, "running command: %s\n.", cmd)
        }
        return nil
}

func (e *ExecRunner) CombinedOutput(cmd string) (string, error) {
        var b bytes.Buffer
        err := e.CombinedOutputTo(cmd,  &b)
        if err != nil {
		return "", errors.Wrapf(err, "running command: %s\n output: %s", cmd, b.Bytes())
        }
        return b.Bytes(), nil
}

For command like minikube logs -f, it should call CombinedOutputTo directly:

CombinedOutputTo(cmd, os.Stdout)

[r2d4]

I actually think the issue is with the SSHRunner implementation rather than the ExecRunner. Both implement the CommandRunner interface. Neither support streaming. I agree the interface should probably be changed to accommodate it.

[lichen2013]

Tested with vm-driver none & virtualbox.

[dlorenc]

Thanks a lot for spending the time on this!

[dlorenc]

@minikube-bot test this please

[dlorenc]

D'oh, looks like a small build error still: pkg/minikube/bootstrapper/localkube/localkube_test.go:209:15: l.GetClusterLogs undefined (type LocalkubeBootstrapper has no field or method GetClusterLogs)

[dlorenc]

@minikube-bot test this please