From cd9e413852bbac80e1f49b7fa169b5a808b504aa Mon Sep 17 00:00:00 2001 From: Medya Gh Date: Tue, 29 Oct 2019 14:11:21 -0700 Subject: [PATCH] resolve code review --- pkg/drivers/none/none.go | 7 ++++--- pkg/minikube/bootstrapper/certs.go | 6 +++--- pkg/minikube/bootstrapper/kubeadm/kubeadm.go | 4 ++-- pkg/minikube/command/command_runner.go | 4 ++-- pkg/minikube/command/exec_runner.go | 3 +-- pkg/minikube/cruntime/containerd.go | 10 +++++----- pkg/minikube/cruntime/crio.go | 4 ++-- pkg/minikube/cruntime/cruntime.go | 4 ++-- 8 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/drivers/none/none.go b/pkg/drivers/none/none.go index 1c367e769a..1e96cc6830 100644 --- a/pkg/drivers/none/none.go +++ b/pkg/drivers/none/none.go @@ -229,10 +229,11 @@ func stopKubelet(cr command.Runner) error { cmd = exec.Command("sudo", "systemctl", "show", "-p", "SubState", "kubelet") cmd.Stdout = &out cmd.Stderr = &out - if rr, err := cr.RunCmd(cmd); err != nil { + rr, err := cr.RunCmd(cmd) + if err != nil { glog.Errorf("temporary error: for %q : %v", rr.Command(), err) } - if !strings.Contains(out.String(), "dead") && !strings.Contains(out.String(), "failed") { + if !strings.Contains(rr.Stdout.String(), "dead") && !strings.Contains(rr.Stdout.String(), "failed") { return fmt.Errorf("unexpected kubelet state: %q", out) } return nil @@ -260,7 +261,7 @@ func checkKubelet(cr command.Runner) error { glog.Infof("checking for running kubelet ...") c := exec.Command("systemctl", "is-active", "--quiet", "service", "kubelet") if _, err := cr.RunCmd(c); err != nil { - return errors.Wrap(err, "checkKubelet") + return errors.Wrap(err, "check kubelet") } return nil } diff --git a/pkg/minikube/bootstrapper/certs.go b/pkg/minikube/bootstrapper/certs.go index acb4230094..6702a6b122 100644 --- a/pkg/minikube/bootstrapper/certs.go +++ b/pkg/minikube/bootstrapper/certs.go @@ -347,19 +347,19 @@ func configureCACerts(cr command.Runner, caCerts map[string]string) error { _, err := cr.RunCmd(exec.Command("sudo", "test", "-f", certStorePath)) if err != nil { if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-s", caCertFile, certStorePath)); err != nil { - return errors.Wrapf(err, "error making symbol link for certificate %s", caCertFile) + return errors.Wrapf(err, "create symlink for %s", caCertFile) } } if hasSSLBinary { subjectHash, err := getSubjectHash(cr, caCertFile) if err != nil { - return errors.Wrapf(err, "error calculating subject hash for certificate %s", caCertFile) + return errors.Wrapf(err, "calculate hash for cacert %s", caCertFile) } subjectHashLink := path.Join(SSLCertStoreDir, fmt.Sprintf("%s.0", subjectHash)) _, err = cr.RunCmd(exec.Command("sudo", "test", "-f", subjectHashLink)) if err != nil { if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-s", certStorePath, subjectHashLink)); err != nil { - return errors.Wrapf(err, "linking caCertFile %s.", caCertFile) + return errors.Wrapf(err, "linking caCertFile %s", caCertFile) } } } diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go index 367ed704d0..e4c7607a17 100644 --- a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go +++ b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go @@ -642,8 +642,8 @@ func (k *Bootstrapper) UpdateCluster(cfg config.KubernetesConfig) error { } } - if _, err := k.c.RunCmd(exec.Command("/bin/bash", "-c", "set -x;sudo systemctl daemon-reload && sudo systemctl start kubelet")); err != nil { - return errors.Wrap(err, "starting kubelet.") + if _, err := k.c.RunCmd(exec.Command("/bin/bash", "-c", "sudo systemctl daemon-reload && sudo systemctl start kubelet")); err != nil { + return errors.Wrap(err, "starting kubelet") } return nil } diff --git a/pkg/minikube/command/command_runner.go b/pkg/minikube/command/command_runner.go index 4ef6815e42..309c8f6133 100644 --- a/pkg/minikube/command/command_runner.go +++ b/pkg/minikube/command/command_runner.go @@ -36,8 +36,8 @@ type RunResult struct { // Runner represents an interface to run commands. type Runner interface { - // RunCmd is a new expermintal way to run commands, takes Cmd interface and returns run result. - // if succesfull will cause a clean up to get rid of older methods. + // RunCmd runs a cmd of exec.Cmd type. allowing user to set cmd.Stdin, cmd.Stdout,... + // not all implementors are guaranteed to handle all the properties of cmd. RunCmd(cmd *exec.Cmd) (*RunResult, error) // Copy is a convenience method that runs a command to copy a file diff --git a/pkg/minikube/command/exec_runner.go b/pkg/minikube/command/exec_runner.go index 66f804d70c..2e9b92e680 100644 --- a/pkg/minikube/command/exec_runner.go +++ b/pkg/minikube/command/exec_runner.go @@ -18,7 +18,6 @@ package command import ( "bytes" - "fmt" "io" "os" "os/exec" @@ -73,7 +72,7 @@ func (*ExecRunner) RunCmd(cmd *exec.Cmd) (*RunResult, error) { rr.ExitCode = exitError.ExitCode() } glog.Infof("(ExecRunner) Non-zero exit: %v: %v (%s)\n%s", rr.Command(), err, elapsed, rr.Output()) - err = errors.Wrapf(err, fmt.Sprintf("stderr: %s", rr.Stderr.String())) + err = errors.Wrapf(err, "command failed: %s\nstdout: %s\nstderr: %s", cmd, rr.Stdout.String(), rr.Stderr.String()) } return rr, err } diff --git a/pkg/minikube/cruntime/containerd.go b/pkg/minikube/cruntime/containerd.go index 85b214dd5d..934f65c416 100644 --- a/pkg/minikube/cruntime/containerd.go +++ b/pkg/minikube/cruntime/containerd.go @@ -161,7 +161,7 @@ func (r *Containerd) Active() bool { // Available returns an error if it is not possible to use this runtime on a host func (r *Containerd) Available() error { - c := exec.Command("command", "-v", "containerd") + c := exec.Command("which", "containerd") if _, err := r.Runner.RunCmd(c); err != nil { return errors.Wrap(err, "check containerd availability.") } @@ -207,7 +207,7 @@ func (r *Containerd) Enable(disOthers bool) error { // Otherwise, containerd will fail API requests with 'Unimplemented' c := exec.Command("sudo", "systemctl", "restart", "containerd") if _, err := r.Runner.RunCmd(c); err != nil { - return errors.Wrap(err, "enable containrd.") + return errors.Wrap(err, "restart containerd") } return nil } @@ -216,7 +216,7 @@ func (r *Containerd) Enable(disOthers bool) error { func (r *Containerd) Disable() error { c := exec.Command("sudo", "systemctl", "stop", "containerd") if _, err := r.Runner.RunCmd(c); err != nil { - return errors.Wrapf(err, "disable containrd.") + return errors.Wrapf(err, "stop containerd") } return nil } @@ -224,9 +224,9 @@ func (r *Containerd) Disable() error { // LoadImage loads an image into this runtime func (r *Containerd) LoadImage(path string) error { glog.Infof("Loading image: %s", path) - c := exec.Command("ctr", "-n=k8s.io", "images", "import", path) + c := exec.Command("sudo", "ctr", "-n=k8s.io", "images", "import", path) if _, err := r.Runner.RunCmd(c); err != nil { - return errors.Wrapf(err, "disable containrd.") + return errors.Wrapf(err, "ctr images import") } return nil } diff --git a/pkg/minikube/cruntime/crio.go b/pkg/minikube/cruntime/crio.go index 0b7b26b359..763e52ac73 100644 --- a/pkg/minikube/cruntime/crio.go +++ b/pkg/minikube/cruntime/crio.go @@ -73,7 +73,7 @@ func (r *CRIO) DefaultCNI() bool { // Available returns an error if it is not possible to use this runtime on a host func (r *CRIO) Available() error { - c := exec.Command("command", "-v", "crio") + c := exec.Command("which", "crio") if _, err := r.Runner.RunCmd(c); err != nil { return errors.Wrapf(err, "check crio available.") } @@ -124,7 +124,7 @@ func (r *CRIO) LoadImage(path string) error { glog.Infof("Loading image: %s", path) c := exec.Command("sudo", "podman", "load", "-i", path) if _, err := r.Runner.RunCmd(c); err != nil { - return errors.Wrap(err, "LoadImage crio.") + return errors.Wrap(err, "crio load image") } return nil } diff --git a/pkg/minikube/cruntime/cruntime.go b/pkg/minikube/cruntime/cruntime.go index e6bc2e753c..6566a8d8cc 100644 --- a/pkg/minikube/cruntime/cruntime.go +++ b/pkg/minikube/cruntime/cruntime.go @@ -133,12 +133,12 @@ func disableOthers(me Manager, cr CommandRunner) error { func enableIPForwarding(cr CommandRunner) error { c := exec.Command("sudo", "modprobe", "br_netfilter") if _, err := cr.RunCmd(c); err != nil { - return errors.Wrap(err, "br_netfilter.") + return errors.Wrap(err, "br_netfilter") } c = exec.Command("sudo", "sh", "-c", "echo 1 > /proc/sys/net/ipv4/ip_forward") if _, err := cr.RunCmd(c); err != nil { - return errors.Wrapf(err, "ip_forward.") + return errors.Wrapf(err, "ip_forward") } return nil }