Merge pull request #6385 from tstromberg/faster-docker

Batch file manipulation calls to reduce number of redundant commands

Author: tstromberg

pkg/minikube/bootstrapper/bsutil/binaries.go

@@ -18,6 +18,7 @@ limitations under the License.
 package bsutil
 
 import (
+	"os/exec"
 	"path"
 	"runtime"
 
@@ -32,6 +33,12 @@ import (
 
 // TransferBinaries transfers all required Kubernetes binaries
 func TransferBinaries(cfg config.KubernetesConfig, c command.Runner) error {
+	dir := binRoot(cfg.KubernetesVersion)
+	_, err := c.RunCmd(exec.Command("sudo", "mkdir", "-p", dir))
+	if err != nil {
+		return err
+	}
+
 	var g errgroup.Group
 	for _, name := range constants.KubernetesReleaseBinaries {
 		name := name
@@ -41,7 +48,7 @@ func TransferBinaries(cfg config.KubernetesConfig, c command.Runner) error {
 				return errors.Wrapf(err, "downloading %s", name)
 			}
 
-			dst := path.Join(binRoot(cfg.KubernetesVersion), name)
+			dst := path.Join(dir, name)
 			if err := machine.CopyBinary(c, src, dst); err != nil {
 				return errors.Wrapf(err, "copybinary %s -> %s", src, dst)
 			}

pkg/minikube/bootstrapper/certs.go

@@ -341,23 +341,20 @@ func configureCACerts(cr command.Runner, caCerts map[string]string) error {
 	for _, caCertFile := range caCerts {
 		dstFilename := path.Base(caCertFile)
 		certStorePath := path.Join(SSLCertStoreDir, dstFilename)
-		_, 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, "create symlink for %s", caCertFile)
-			}
+		cmd := fmt.Sprintf("test -f %s || ln -s %s %s", caCertFile, certStorePath, caCertFile)
+		if _, err := cr.RunCmd(exec.Command("sudo", "/bin/bash", "-c", cmd)); err != nil {
+			return errors.Wrapf(err, "create symlink for %s", caCertFile)
 		}
 		if hasSSLBinary {
 			subjectHash, err := getSubjectHash(cr, caCertFile)
 			if err != nil {
 				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)
-				}
+
+			cmd := fmt.Sprintf("test -f %s || ln -s %s %s", subjectHashLink, certStorePath, subjectHashLink)
+			if _, err := cr.RunCmd(exec.Command("sudo", "/bin/bash", "-c", cmd)); err != nil {
+				return errors.Wrapf(err, "create symlink for %s", caCertFile)
 			}
 		}
 	}

pkg/minikube/bootstrapper/certs_test.go

@@ -17,9 +17,7 @@ limitations under the License.
 package bootstrapper
 
 import (
-	"fmt"
 	"os"
-	"path"
 	"path/filepath"
 	"testing"
 
@@ -53,21 +51,12 @@ func TestSetupCerts(t *testing.T) {
 		t.Fatalf("error generating certificate: %v", err)
 	}
 
-	cmdMap := map[string]string{
-		"sudo mkdir -p  /var/lib/minikube/certs": "",
-	}
-	certFilenames := map[string]string{"ca.crt": "minikubeCA.pem", "mycert.pem": "mycert.pem"}
-	for _, dst := range certFilenames {
-		certFile := path.Join(CACertificatesDir, dst)
-		certStorePath := path.Join(SSLCertStoreDir, dst)
-		certNameHash := "abcdef"
-		remoteCertHashLink := path.Join(SSLCertStoreDir, fmt.Sprintf("%s.0", certNameHash))
-		cmdMap[fmt.Sprintf("sudo ln -s %s %s", certFile, certStorePath)] = "1"
-		cmdMap[fmt.Sprintf("openssl x509 -hash -noout -in %s", certFile)] = certNameHash
-		cmdMap[fmt.Sprintf("sudo ln -s %s %s", certStorePath, remoteCertHashLink)] = "1"
+	expected := map[string]string{
+		`sudo /bin/bash -c "test -f /usr/share/ca-certificates/mycert.pem || ln -s /etc/ssl/certs/mycert.pem /usr/share/ca-certificates/mycert.pem"`:             "-",
+		`sudo /bin/bash -c "test -f /usr/share/ca-certificates/minikubeCA.pem || ln -s /etc/ssl/certs/minikubeCA.pem /usr/share/ca-certificates/minikubeCA.pem"`: "-",
 	}
 	f := command.NewFakeCommandRunner()
-	f.SetCommandToOutput(cmdMap)
+	f.SetCommandToOutput(expected)
 
 	var filesToBeTransferred []string
 	for _, cert := range certs {

pkg/minikube/bootstrapper/kubeadm/kubeadm.go

@@ -471,6 +471,17 @@ func (k *Bootstrapper) UpdateCluster(cfg config.MachineConfig) error {
 	if err := bsutil.AddAddons(&files, assets.GenerateTemplateData(cfg.KubernetesConfig)); err != nil {
 		return errors.Wrap(err, "adding addons")
 	}
+
+	// Combine mkdir request into a single call to reduce load
+	dirs := []string{}
+	for _, f := range files {
+		dirs = append(dirs, f.GetTargetDir())
+	}
+	args := append([]string{"mkdir", "-p"}, dirs...)
+	if _, err := k.c.RunCmd(exec.Command("sudo", args...)); err != nil {
+		return errors.Wrap(err, "mkdir")
+	}
+
 	for _, f := range files {
 		if err := k.c.Copy(f); err != nil {
 			return errors.Wrapf(err, "copy")

pkg/minikube/cluster/cluster.go

@@ -24,6 +24,7 @@ import (
 	"math"
 	"net"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"regexp"
 	"strconv"
@@ -46,6 +47,7 @@ import (
 	"github.com/shirou/gopsutil/mem"
 	"github.com/spf13/viper"
 
+	"k8s.io/minikube/pkg/minikube/command"
 	"k8s.io/minikube/pkg/minikube/config"
 	cfg "k8s.io/minikube/pkg/minikube/config"
 	"k8s.io/minikube/pkg/minikube/constants"
@@ -54,6 +56,8 @@ import (
 	"k8s.io/minikube/pkg/minikube/localpath"
 	"k8s.io/minikube/pkg/minikube/out"
 	"k8s.io/minikube/pkg/minikube/registry"
+	"k8s.io/minikube/pkg/minikube/sshutil"
+	"k8s.io/minikube/pkg/minikube/vmpath"
 	"k8s.io/minikube/pkg/util/lock"
 	"k8s.io/minikube/pkg/util/retry"
 )
@@ -67,6 +71,17 @@ var (
 	// The maximum the guest VM clock is allowed to be ahead and behind. This value is intentionally
 	// large to allow for inaccurate methodology, but still small enough so that certificates are likely valid.
 	maxClockDesyncSeconds = 2.1
+
+	// requiredDirectories are directories to create on the host during setup
+	requiredDirectories = []string{
+		vmpath.GuestAddonsDir,
+		vmpath.GuestManifestsDir,
+		vmpath.GuestEphemeralDir,
+		vmpath.GuestPersistentDir,
+		vmpath.GuestCertsDir,
+		path.Join(vmpath.GuestPersistentDir, "images"),
+		path.Join(vmpath.GuestPersistentDir, "binaries"),
+	}
 )
 
 // This init function is used to set the logtostderr variable to false so that INFO level log info does not clutter the CLI
@@ -497,6 +512,10 @@ func createHost(api libmachine.API, config cfg.MachineConfig) (*host.Host, error
 		return nil, errors.Wrap(err, "create")
 	}
 
+	if err := createRequiredDirectories(h); err != nil {
+		return h, errors.Wrap(err, "required directories")
+	}
+
 	if driver.BareMetal(config.VMDriver) {
 		showLocalOsRelease()
 	} else if !driver.BareMetal(config.VMDriver) && !driver.IsKIC(config.VMDriver) {
@@ -646,3 +665,41 @@ func IsMinikubeRunning(api libmachine.API) bool {
 	}
 	return true
 }
+
+// createRequiredDirectories creates directories expected by minikube to exist
+func createRequiredDirectories(h *host.Host) error {
+	if h.DriverName == driver.Mock {
+		glog.Infof("skipping createRequiredDirectories")
+		return nil
+	}
+	glog.Infof("creating required directories: %v", requiredDirectories)
+	r, err := commandRunner(h)
+	if err != nil {
+		return errors.Wrap(err, "command runner")
+	}
+
+	args := append([]string{"mkdir", "-p"}, requiredDirectories...)
+	if _, err := r.RunCmd(exec.Command("sudo", args...)); err != nil {
+		return errors.Wrapf(err, "sudo mkdir (%s)", h.DriverName)
+	}
+	return nil
+}
+
+// commandRunner returns best available command runner for this host
+func commandRunner(h *host.Host) (command.Runner, error) {
+	if h.DriverName == driver.Mock {
+		glog.Errorf("commandRunner: returning unconfigured FakeCommandRunner, commands will fail!")
+		return &command.FakeCommandRunner{}, nil
+	}
+	if driver.BareMetal(h.Driver.DriverName()) {
+		return &command.ExecRunner{}, nil
+	}
+	if h.Driver.DriverName() == driver.Docker {
+		return command.NewKICRunner(h.Name, "docker"), nil
+	}
+	client, err := sshutil.NewSSHClient(h.Driver)
+	if err != nil {
+		return nil, errors.Wrap(err, "getting ssh client for bootstrapper")
+	}
+	return command.NewSSHRunner(client), nil
+}

pkg/minikube/cluster/cluster_test.go

@@ -374,6 +374,8 @@ func TestGetHostDockerEnv(t *testing.T) {
 }
 
 func TestGetHostDockerEnvIPv6(t *testing.T) {
+	RegisterMockDriver(t)
+
 	tempDir := tests.MakeTempDir()
 	defer os.RemoveAll(tempDir)
 

pkg/minikube/command/exec_runner.go

@@ -80,9 +80,6 @@ func (*ExecRunner) RunCmd(cmd *exec.Cmd) (*RunResult, error) {
 
 // Copy copies a file and its permissions
 func (*ExecRunner) Copy(f assets.CopyableFile) error {
-	if err := os.MkdirAll(f.GetTargetDir(), os.ModePerm); err != nil {
-		return errors.Wrapf(err, "error making dirs for %s", f.GetTargetDir())
-	}
 	targetPath := path.Join(f.GetTargetDir(), f.GetTargetName())
 	if _, err := os.Stat(targetPath); err == nil {
 		if err := os.Remove(targetPath); err != nil {

pkg/minikube/command/fake_runner.go

@@ -54,7 +54,21 @@ func (f *FakeCommandRunner) RunCmd(cmd *exec.Cmd) (*RunResult, error) {
 
 	start := time.Now()
 
-	out, ok := f.cmdMap.Load(strings.Join(rr.Args, " "))
+	key := rr.Command()
+	out, ok := f.cmdMap.Load(key)
+	if !ok {
+		cmds := f.commands()
+		if len(cmds) == 0 {
+			return rr, fmt.Errorf("asked to execute %s, but FakeCommandRunner has no commands stored", rr.Command())
+		}
+
+		var txt strings.Builder
+		for _, c := range f.commands() {
+			txt.WriteString(fmt.Sprintf("  `%s`\n", c))
+		}
+		return rr, fmt.Errorf("unregistered command:\n  `%s`\nexpected one of:\n%s", key, txt.String())
+	}
+
 	var buf bytes.Buffer
 	outStr := ""
 	if out != nil {
@@ -69,14 +83,9 @@ func (f *FakeCommandRunner) RunCmd(cmd *exec.Cmd) (*RunResult, error) {
 
 	elapsed := time.Since(start)
 
-	if ok {
-		// Reduce log spam
-		if elapsed > (1 * time.Second) {
-			glog.Infof("(FakeCommandRunner) Done: %v: (%s)", rr.Command(), elapsed)
-		}
-	} else {
-		glog.Infof("(FakeCommandRunner) Non-zero exit: %v: (%s)\n%s", rr.Command(), elapsed, out)
-		return rr, fmt.Errorf("unavailable command: %s", rr.Command())
+	// Reduce log spam
+	if elapsed > (1 * time.Second) {
+		glog.Infof("(FakeCommandRunner) Done: %v: (%s)", rr.Command(), elapsed)
 	}
 	return rr, nil
 }
@@ -108,6 +117,7 @@ func (f *FakeCommandRunner) SetFileToContents(fileToContents map[string]string)
 // SetCommandToOutput stores the file to contents map for the FakeCommandRunner
 func (f *FakeCommandRunner) SetCommandToOutput(cmdToOutput map[string]string) {
 	for k, v := range cmdToOutput {
+		glog.Infof("fake command %q -> %q", k, v)
 		f.cmdMap.Store(k, v)
 	}
 }
@@ -121,6 +131,15 @@ func (f *FakeCommandRunner) GetFileToContents(filename string) (string, error) {
 	return contents.(string), nil
 }
 
+func (f *FakeCommandRunner) commands() []string {
+	cmds := []string{}
+	f.cmdMap.Range(func(k, v interface{}) bool {
+		cmds = append(cmds, fmt.Sprintf("%s", k))
+		return true
+	})
+	return cmds
+}
+
 // DumpMaps prints out the list of stored commands and stored filenames.
 func (f *FakeCommandRunner) DumpMaps(w io.Writer) {
 	fmt.Fprintln(w, "Commands:")

pkg/minikube/command/kic_runner.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"strconv"
 	"time"
 
 	"github.com/golang/glog"
@@ -118,7 +119,7 @@ func (k *kicRunner) RunCmd(cmd *exec.Cmd) (*RunResult, error) {
 		if exitError, ok := err.(*exec.ExitError); ok {
 			rr.ExitCode = exitError.ExitCode()
 		}
-		err = errors.Wrapf(err, "command failed: %s", oc.Args)
+		err = fmt.Errorf("%s: %v\nstdout:\n%s\nstderr:\n%s", rr.Command(), err, rr.Stdout.String(), rr.Stderr.String())
 	}
 	return rr, err
 
@@ -148,22 +149,24 @@ func (k *kicRunner) Copy(f assets.CopyableFile) error {
 			return errors.Wrap(err, "close temporary file")
 		}
 		assetName = tmpFile.Name()
-
 	}
+
 	// based of format of "docker cp containerName:destination"
 	destination := fmt.Sprintf("%s:%s/%s", k.nameOrID, f.GetTargetDir(), f.GetTargetName())
-	// make sure dir exists inside the container
-	if _, err := k.RunCmd(exec.Command("mkdir", "-p", f.GetTargetDir())); err != nil {
-		return errors.Wrapf(err, "error making dir %s", f.GetTargetDir())
+
+	perms, err := strconv.ParseInt(f.GetPermissions(), 8, 0)
+	if err != nil {
+		return errors.Wrapf(err, "error converting permissions %s to integer", f.GetPermissions())
 	}
 
-	if out, err := exec.Command(k.ociBin, "cp", assetName, destination).CombinedOutput(); err != nil {
-		return errors.Wrapf(err, "error copying %s into node, output: %s", f.GetAssetName(), string(out))
+	// Rely on cp -a to propagate permissions
+	if err := os.Chmod(assetName, os.FileMode(perms)); err != nil {
+		return errors.Wrapf(err, "chmod")
 	}
-	fp := path.Join(f.GetTargetDir(), f.GetTargetName())
-	if _, err := k.RunCmd(exec.Command("sudo", "chmod", f.GetPermissions(), fp)); err != nil {
-		return errors.Wrapf(err, "failed to chmod file permissions %s", fp)
+	if out, err := exec.Command(k.ociBin, "cp", "-a", assetName, destination).CombinedOutput(); err != nil {
+		return errors.Wrapf(err, "error copying %s into node, output: %s", f.GetAssetName(), string(out))
 	}
+
 	return nil
 }
 

pkg/minikube/command/ssh_runner.go

@@ -196,7 +196,7 @@ func (s *SSHRunner) Copy(f assets.CopyableFile) error {
 		return nil
 	})
 
-	scp := fmt.Sprintf("sudo mkdir -p %s && sudo scp -t %s", f.GetTargetDir(), f.GetTargetDir())
+	scp := fmt.Sprintf("sudo test -d %s && sudo scp -t %s", f.GetTargetDir(), f.GetTargetDir())
 	mtime, err := f.GetModTime()
 	if err != nil {
 		glog.Infof("error getting modtime for %s: %v", dst, err)

pkg/minikube/machine/cache_images.go

@@ -22,6 +22,7 @@ import (
 	"path"
 	"path/filepath"
 	"sync"
+	"time"
 
 	"github.com/docker/docker/client"
 	"github.com/docker/machine/libmachine/state"
@@ -63,7 +64,12 @@ func CacheImagesForBootstrapper(imageRepository string, version string, clusterB
 // LoadImages loads previously cached images into the container runtime
 func LoadImages(cc *config.MachineConfig, runner command.Runner, images []string, cacheDir string) error {
 	glog.Infof("LoadImages start: %s", images)
-	defer glog.Infof("LoadImages end")
+	start := time.Now()
+
+	defer func() {
+		glog.Infof("LoadImages completed in %s", time.Since(start))
+	}()
+
 	var g errgroup.Group
 	cr, err := cruntime.New(cruntime.Config{Type: cc.ContainerRuntime, Runner: runner})
 	if err != nil {