Code review comments

pkg/minikube/bootstrapper/bootstrapper.go

@@ -23,6 +23,14 @@ import (
 	"k8s.io/minikube/pkg/minikube/constants"
 )
 
+// LogOptions are options to be passed to LogCommands
+type LogOptions struct {
+	// Lines is the number of recent log lines to include, as in tail -n.
+	Lines int
+	// Follow is whether or not to actively follow the logs, as in tail -f.
+	Follow bool
+}
+
 // Bootstrapper contains all the methods needed to bootstrap a kubernetes cluster
 type Bootstrapper interface {
 	// PullImages pulls images necessary for a cluster. Success should not be required.
@@ -32,7 +40,7 @@ type Bootstrapper interface {
 	RestartCluster(config.KubernetesConfig) error
 	DeleteCluster(config.KubernetesConfig) error
 	// LogCommands returns a map of log type to a command which will display that log.
-	LogCommands(int, bool) map[string]string
+	LogCommands(LogOptions) map[string]string
 	SetupCerts(cfg config.KubernetesConfig) error
 	GetKubeletStatus() (string, error)
 	GetApiServerStatus(net.IP) (string, error)

pkg/minikube/bootstrapper/kubeadm/kubeadm.go

@@ -121,13 +121,13 @@ func (k *KubeadmBootstrapper) GetApiServerStatus(ip net.IP) (string, error) {
 }
 
 // LogCommands returns a map of log type to a command which will display that log.
-func (k *KubeadmBootstrapper) LogCommands(len int, follow bool) map[string]string {
+func (k *KubeadmBootstrapper) LogCommands(o bootstrapper.LogOptions) map[string]string {
 	var kcmd strings.Builder
 	kcmd.WriteString("journalctl -u kubelet")
-	if len > 0 {
-		kcmd.WriteString(fmt.Sprintf(" -n %d", len))
+	if o.Lines > 0 {
+		kcmd.WriteString(fmt.Sprintf(" -n %d", o.Lines))
 	}
-	if follow {
+	if o.Follow {
 		kcmd.WriteString(" -f")
 	}
 	return map[string]string{"kubelet": kcmd.String()}

pkg/minikube/exit/exit.go

@@ -18,6 +18,7 @@ limitations under the License.
 package exit
 
 import (
+	"fmt"
 	"os"
 
 	"github.com/golang/glog"
@@ -80,10 +81,10 @@ func WithProblems(msg string, err error, problems map[string][]string) {
 }
 
 func displayError(msg string, err error) {
+	// use Warning because Error will display a duplicate message to stderr
+	glog.Warningf(fmt.Sprintf("%s: %v", msg, err))
 	console.Fatal(msg+": %v", err)
 	console.Err("\n")
 	console.ErrStyle("sad", "Sorry that minikube crashed. If this was unexpected, we would love to hear from you:")
 	console.ErrStyle("url", "https://github.com/kubernetes/minikube/issues/new")
-	// use Warning because Error will display a duplicate message to stderr
-	glog.Warningf(msg+"%s: %v", msg)
 }

pkg/minikube/logs/logs.go

@@ -18,13 +18,14 @@ limitations under the License.
 package logs
 
 import (
+	"bufio"
+	"bytes"
 	"fmt"
 	"os"
 	"regexp"
 	"sort"
 	"strings"
 
-	"github.com/docker/machine/libmachine/log"
 	"github.com/golang/glog"
 	"k8s.io/minikube/pkg/minikube/bootstrapper"
 	"k8s.io/minikube/pkg/minikube/console"
@@ -41,6 +42,10 @@ var importantPods = []string{
 	"k8s_kube-scheduler",
 }
 
+// lookbackwardsCount is how far back to look in a log for problems. This should be large enough to
+// include usage messages from a failed binary, but small enough to not include irrelevant problems.
+const lookBackwardsCount = 200
+
 // Follow follows logs from multiple files in tail(1) format
 func Follow(r cruntime.Manager, bs bootstrapper.Bootstrapper, runner bootstrapper.CommandRunner) error {
 	cs := []string{}
@@ -59,18 +64,19 @@ func IsProblem(line string) bool {
 // FindProblems finds possible root causes among the logs
 func FindProblems(r cruntime.Manager, bs bootstrapper.Bootstrapper, runner bootstrapper.CommandRunner) map[string][]string {
 	pMap := map[string][]string{}
-	cmds := logCommands(r, bs, 200, false)
+	cmds := logCommands(r, bs, lookBackwardsCount, false)
 	for name, cmd := range cmds {
-		log.Infof("Gathering logs for %s ...", name)
-		out, err := runner.CombinedOutput(cmds[name])
+		glog.Infof("Gathering logs for %s ...", name)
+		var b bytes.Buffer
+		err := runner.CombinedOutputTo(cmds[name], &b)
 		if err != nil {
 			glog.Warningf("failed %s: %s: %v", name, cmd, err)
 			continue
 		}
-		log.Infof("log length: %d", len(out))
-
+		scanner := bufio.NewScanner(&b)
 		problems := []string{}
-		for _, l := range strings.Split(out, "\n") {
+		for scanner.Scan() {
+			l := scanner.Text()
 			if IsProblem(l) {
 				glog.Warningf("Found %s problem: %s", name, l)
 				problems = append(problems, l)
@@ -108,13 +114,17 @@ func Output(r cruntime.Manager, bs bootstrapper.Bootstrapper, runner bootstrappe
 	failed := []string{}
 	for _, name := range names {
 		console.OutLn("==> %s <==", name)
-		out, err := runner.CombinedOutput(cmds[name])
+		var b bytes.Buffer
+		err := runner.CombinedOutputTo(cmds[name], &b)
 		if err != nil {
 			glog.Errorf("failed: %v", err)
 			failed = append(failed, name)
 			continue
 		}
-		console.OutLn(out)
+		scanner := bufio.NewScanner(&b)
+		for scanner.Scan() {
+			console.OutLn(scanner.Text())
+		}
 	}
 	if len(failed) > 0 {
 		return fmt.Errorf("unable to fetch logs for: %s", strings.Join(failed, ", "))
@@ -124,15 +134,16 @@ func Output(r cruntime.Manager, bs bootstrapper.Bootstrapper, runner bootstrappe
 
 // logCommands returns a list of commands that would be run to receive the anticipated logs
 func logCommands(r cruntime.Manager, bs bootstrapper.Bootstrapper, length int, follow bool) map[string]string {
-	cmds := bs.LogCommands(length, follow)
+	cmds := bs.LogCommands(bootstrapper.LogOptions{Lines: length, Follow: follow})
 	for _, pod := range importantPods {
 		ids, err := r.ListContainers(pod)
 		if err != nil {
 			glog.Errorf("Failed to list containers for %q: %v", pod, err)
 			continue
 		}
+		glog.Infof("%d containers: %s", len(ids), ids)
 		if len(ids) == 0 {
-			glog.Errorf("No containers found matching %q", pod)
+			cmds[pod] = fmt.Sprintf("No container was found matching %q", pod)
 			continue
 		}
 		cmds[pod] = r.ContainerLogCmd(ids[0], length, follow)