Redirect command Stdout & Stderr for command_runner

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

Fixes: https://github.com/kubernetes/minikube/issues/2447

cmd/minikube/cmd/logs.go

@@ -50,12 +50,11 @@ var logsCmd = &cobra.Command{
 			glog.Exitf("Error getting cluster bootstrapper: %s", err)
 		}
 
-		s, err := clusterBootstrapper.GetClusterLogs(follow)
+		err = clusterBootstrapper.GetClusterLogsTo(follow, os.Stdout)
 		if err != nil {
 			log.Println("Error getting machine logs:", err)
 			cmdUtil.MaybeReportErrorAndExit(err)
 		}
-		fmt.Fprintln(os.Stdout, s)
 	},
 }
 

pkg/minikube/bootstrapper/bootstrapper.go

@@ -17,6 +17,7 @@ limitations under the License.
 package bootstrapper
 
 import (
+	"io"
 	"net"
 
 	"k8s.io/minikube/pkg/minikube/constants"
@@ -28,7 +29,7 @@ type Bootstrapper interface {
 	StartCluster(KubernetesConfig) error
 	UpdateCluster(KubernetesConfig) error
 	RestartCluster(KubernetesConfig) error
-	GetClusterLogs(follow bool) (string, error)
+	GetClusterLogsTo(follow bool, out io.Writer) error
 	SetupCerts(cfg KubernetesConfig) error
 	GetClusterStatus() (string, error)
 }

pkg/minikube/bootstrapper/command_runner.go

@@ -18,6 +18,7 @@ package bootstrapper
 
 import (
 	"fmt"
+	"io"
 	"path/filepath"
 
 	"k8s.io/minikube/pkg/minikube/assets"
@@ -28,6 +29,19 @@ type CommandRunner interface {
 	// Run starts the specified command and waits for it to complete.
 	Run(cmd string) error
 
+	// CombinedOutputTo runs the command and stores both command
+	// output and error to out. A typical usage is:
+	//
+	//          var b bytes.Buffer
+	//          CombinedOutput(cmd, &b)
+	//          fmt.Println(b.Bytes())
+	//
+	// Or, you can set out to os.Stdout, the command output and
+	// error would show on your terminal immediately before you
+	// cmd exit. This is useful for a long run command such as
+	// continuously print running logs.
+	CombinedOutputTo(cmd string, out io.Writer) error
+
 	// CombinedOutput runs the command and returns its combined standard
 	// output and standard error.
 	CombinedOutput(cmd string) (string, error)

pkg/minikube/bootstrapper/exec_runner.go

@@ -17,6 +17,7 @@ limitations under the License.
 package bootstrapper
 
 import (
+	"bytes"
 	"io"
 	"os"
 	"os/exec"
@@ -43,16 +44,31 @@ func (*ExecRunner) Run(cmd string) error {
 	return nil
 }
 
-// CombinedOutput runs the command  in a bash shell and returns its
-// combined standard output and standard error.
-func (*ExecRunner) CombinedOutput(cmd string) (string, error) {
+// CombinedOutputTo runs the command and stores both command
+// output and error to out.
+func (*ExecRunner) CombinedOutputTo(cmd string, out io.Writer) error {
 	glog.Infoln("Run with output:", cmd)
 	c := exec.Command("/bin/bash", "-c", cmd)
-	out, err := c.CombinedOutput()
+	c.Stdout = out
+	c.Stderr = out
+	err := c.Run()
 	if err != nil {
-		return "", errors.Wrapf(err, "running command: %s\n output: %s", cmd, out)
+		return errors.Wrapf(err, "running command: %s\n.", cmd)
 	}
-	return string(out), nil
+
+	return nil
+}
+
+// CombinedOutput runs the command  in a bash shell and returns its
+// combined standard output and standard error.
+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.String(), nil
+
 }
 
 // Copy copies a file and its permissions

pkg/minikube/bootstrapper/fake_runner.go

@@ -49,6 +49,21 @@ func (f *FakeCommandRunner) Run(cmd string) error {
 	return err
 }
 
+// CombinedOutputTo runs the command and stores both command
+// output and error to out.
+func (f *FakeCommandRunner) CombinedOutputTo(cmd string, out io.Writer) error {
+	value, ok := f.cmdMap.Load(cmd)
+	if !ok {
+		return fmt.Errorf("unavailable command: %s", cmd)
+	}
+	_, err := fmt.Fprint(out, value)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // CombinedOutput returns the set output for a given command text.
 func (f *FakeCommandRunner) CombinedOutput(cmd string) (string, error) {
 	out, ok := f.cmdMap.Load(cmd)

pkg/minikube/bootstrapper/kubeadm/kubeadm.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"crypto"
 	"fmt"
+	"io"
 	"os"
 	"path"
 	"strings"
@@ -81,7 +82,7 @@ func (k *KubeadmBootstrapper) GetClusterStatus() (string, error) {
 
 // TODO(r2d4): Should this aggregate all the logs from the control plane?
 // Maybe subcommands for each component? minikube logs apiserver?
-func (k *KubeadmBootstrapper) GetClusterLogs(follow bool) (string, error) {
+func (k *KubeadmBootstrapper) GetClusterLogsTo(follow bool, out io.Writer) error {
 	var flags []string
 	if follow {
 		flags = append(flags, "-f")
@@ -89,17 +90,18 @@ func (k *KubeadmBootstrapper) GetClusterLogs(follow bool) (string, error) {
 	logsCommand := fmt.Sprintf("sudo journalctl %s -u kubelet", strings.Join(flags, " "))
 
 	if follow {
-		if err := k.c.Run(logsCommand); err != nil {
-			return "", errors.Wrap(err, "getting shell")
+		if err := k.c.CombinedOutputTo(logsCommand, out); err != nil {
+			return errors.Wrap(err, "getting cluster logs")
 		}
-	}
+	} else {
 
-	logs, err := k.c.CombinedOutput(logsCommand)
-	if err != nil {
-		return "", errors.Wrap(err, "getting cluster logs")
+		logs, err := k.c.CombinedOutput(logsCommand)
+		if err != nil {
+			return errors.Wrap(err, "getting cluster logs")
+		}
+		fmt.Fprint(out, logs)
 	}
-
-	return logs, nil
+	return nil
 }
 
 func (k *KubeadmBootstrapper) StartCluster(k8s bootstrapper.KubernetesConfig) error {

pkg/minikube/bootstrapper/localkube/localkube.go

@@ -18,6 +18,7 @@ package localkube
 
 import (
 	"fmt"
+	"io"
 	"strings"
 
 	"k8s.io/minikube/pkg/minikube/assets"
@@ -57,19 +58,27 @@ func NewLocalkubeBootstrapper(api libmachine.API) (*LocalkubeBootstrapper, error
 	}, nil
 }
 
-// GetClusterLogs If follow is specified, it will tail the logs
-func (lk *LocalkubeBootstrapper) GetClusterLogs(follow bool) (string, error) {
+// GetClusterLogs
+// If follow is specified, it will tail the logs
+func (lk *LocalkubeBootstrapper) GetClusterLogsTo(follow bool, out io.Writer) error {
 	logsCommand, err := GetLogsCommand(follow)
 	if err != nil {
-		return "", errors.Wrap(err, "Error getting logs command")
+		return errors.Wrap(err, "Error getting logs command")
 	}
 
-	logs, err := lk.cmd.CombinedOutput(logsCommand)
-	if err != nil {
-		return "", errors.Wrap(err, "getting cluster logs")
+	if follow {
+		err = lk.cmd.CombinedOutputTo(logsCommand, out)
+		if err != nil {
+			return errors.Wrap(err, "getting cluster logs")
+		}
+	} else {
+		logs, err := lk.cmd.CombinedOutput(logsCommand)
+		if err != nil {
+			return errors.Wrap(err, "getting cluster logs")
+		}
+		fmt.Fprint(out, logs)
 	}
-
-	return logs, nil
+	return nil
 }
 
 // GetClusterStatus gets the status of localkube from the host VM.

pkg/minikube/bootstrapper/localkube/localkube_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package localkube
 
 import (
+	"bytes"
 	"testing"
 
 	"k8s.io/minikube/pkg/minikube/bootstrapper"
@@ -200,13 +201,14 @@ func TestGetHostLogs(t *testing.T) {
 		},
 	}
 
+	var b bytes.Buffer
 	for _, test := range cases {
 		t.Run(test.description, func(t *testing.T) {
 			t.Parallel()
 			f := bootstrapper.NewFakeCommandRunner()
 			f.SetCommandToOutput(test.logsCmdMap)
 			l := LocalkubeBootstrapper{f}
-			_, err := l.GetClusterLogs(test.follow)
+			err := l.GetClusterLogsTo(test.follow, &b)
 			if err != nil && !test.shouldErr {
 				t.Errorf("Error getting localkube logs: %s", err)
 				return

pkg/minikube/bootstrapper/ssh_runner.go

@@ -17,6 +17,7 @@ limitations under the License.
 package bootstrapper
 
 import (
+	"bytes"
 	"fmt"
 	"io"
 	"path"
@@ -63,20 +64,36 @@ func (s *SSHRunner) Run(cmd string) error {
 	return sess.Run(cmd)
 }
 
-// CombinedOutput runs the command on the remote and returns its combined
-// standard output and standard error.
-func (s *SSHRunner) CombinedOutput(cmd string) (string, error) {
+// CombinedOutputTo runs the command and stores both command
+// output and error to out.
+func (s *SSHRunner) CombinedOutputTo(cmd string, out io.Writer) error {
 	glog.Infoln("Run with output:", cmd)
 	sess, err := s.c.NewSession()
 	if err != nil {
-		return "", errors.Wrap(err, "getting ssh session")
+		return errors.Wrap(err, "getting ssh session")
 	}
 	defer sess.Close()
-	out, err := sess.CombinedOutput(cmd)
+
+	sess.Stdout = out
+	sess.Stderr = out
+
+	err = sess.Run(cmd)
 	if err != nil {
-		return "", errors.Wrapf(err, "running command: %s\n output: %s", cmd, out)
+		return errors.Wrapf(err, "running command: %s\n.", cmd)
 	}
-	return string(out), nil
+
+	return nil
+}
+
+// CombinedOutput runs the command on the remote and returns its combined
+// standard output and standard error.
+func (s *SSHRunner) CombinedOutput(cmd string) (string, error) {
+	var b bytes.Buffer
+	err := s.CombinedOutputTo(cmd, &b)
+	if err != nil {
+		return "", errors.Wrapf(err, "running command: %s\n output: %s", cmd, b.Bytes())
+	}
+	return b.String(), nil
 }
 
 // Copy copies a file to the remote over SSH.