Cache images in minikube #1881

r2d4 · 2017-08-24T02:05:28Z

This PR uses the containers/image library to pull images while minikube is starting up, cache them in the minikube home directory, and then load them into the docker daemon.

This makes the cluster boot up much quicker and reliably.

Theres probably a bit of work to be done with how I'm caching the images to save space by reusing layers, but I think this is a good first pass.

r2d4 · 2017-08-24T02:06:42Z

pkg/minikube/machine/cache_images.go

+		// By default, the image library will try to look at /etc/docker/certs.d
+		// As a non-root user, this would result in a permissions error,
+		// so, we skip this step by just looking in the minikube home directory.
+		DockerCertPath: constants.MakeMiniPath("cache"),


cc @dlorenc @cftorres this was my trick to get around the /etc/docker/certs.d error.

r2d4 · 2017-08-24T02:08:29Z

Im closing #1671 in favor of this approach. It's probably better not to pollute the ISO with these container images. This also allows us to selectively cache and load images, so we can load localkube's images and kubeadm's images.

We could also modify this to cache addons

This PR introduces caching of localkube images. It makes a best effort to cache the essential images localkube needs as minikube starts up. Currently, the list of cached images is hardcoded, but future work might entail 1. Cached images as a property of the cluster bootstrapper - to allow localkube and kubeadm to cache their respective images. 2. Addons contain image information. Then, we can selectively cache and preload only the addon images that are enabled.

We shouldn't blow away the cache on every integration test. keep .minikube for tests hack/jenkins: Delete VM before manual cleanup

It is useful to be able to have a command runner to call directly in the integration tests

codecov-io · 2017-08-31T06:04:33Z

Codecov Report

Merging #1881 into master will decrease coverage by 0.82%.
The diff coverage is 2.85%.

@@            Coverage Diff             @@
##           master    #1881      +/-   ##
==========================================
- Coverage   31.05%   30.22%   -0.83%     
==========================================
  Files          74       75       +1     
  Lines        4302     4433     +131     
==========================================
+ Hits         1336     1340       +4     
- Misses       2791     2916     +125     
- Partials      175      177       +2

Impacted Files	Coverage Δ
pkg/minikube/cluster/cluster.go	`30.34% <ø> (ø)`	⬆️
pkg/minikube/bootstrapper/localkube/localkube.go	`30.85% <0%> (-1.74%)`	⬇️
pkg/minikube/machine/cache_images.go	`2.5% <2.5%> (ø)`
cmd/minikube/cmd/start.go	`10.09% <6.66%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd91fea...3fb711c. Read the comment docs.

r2d4 · 2017-08-31T16:30:08Z

I think this is ready for review @dlorenc @aaron-prindle. As a nice bonus, this makes minikube usable offline after the images have been cached.

dlorenc · 2017-08-31T16:41:34Z

Makefile

@@ -67,7 +67,7 @@ KUBE_CROSS_DOCKER_CMD := docker run -w /go/src/$(REPOPATH) --user $(shell id -u)

 # $(call MINIKUBE_GO_BUILD_CMD, output file, OS)


The file perms changed on here too to 755.

dlorenc · 2017-08-31T16:42:11Z

hack/jenkins/common.sh

 # See the default image
 ./out/minikube-${OS_ARCH} start -h | grep iso

 # see what driver we are using
 which docker-machine-driver-${VM_DRIVER} || true
+tree ~/.minikube || true


How about "find ~/.minikube"? Not all systems have tree.

I ended up putting it on the build slaves, find is probably simpler

dlorenc · 2017-08-31T16:43:12Z

pkg/minikube/machine/cache_images.go

+	"github.com/pkg/errors"
+)
+
+const tempLoadDir = "/tmp"


Why do we use this? Could we use os.Tempdir?

This is in the minikube VM. I think I tried to send the files over stdin with docker load, but I think it was a little tricky. I ended up just scping them into the VM and cleaning them up after the load.

dlorenc · 2017-08-31T16:44:32Z

pkg/minikube/machine/cache_images.go

+	return nil
+}
+
+func CacheImages(images []string, cacheDir string) error {


A comment explaining what the resulting cache directory will look like might help.

dlorenc · 2017-08-31T16:45:42Z

pkg/minikube/machine/cache_images.go

+	return nil
+}
+
+func LoadImages(cmd bootstrapper.CommandRunner, images []string, cacheDir string) error {


This function looks almost exactly the same as the previous one. Any way you could refactor it to take the LoadFromCache/Cache functions as a parameter to avoid the repetition?

Ah yeah good catch. Will do that

The functions have different signatures, so I've been having a little trouble figuring out how to share code here. Any suggestions?

I didn't fix this one. I gave it an attempt, but all my attempts ended up way more verbose than the current solution.

I think this would probably be easiest if Cache and Load were methods on the bootstrapper, so they could have the same signature. However, that limits when we can actually call them. Since CacheImages doesn't actually require the bootstrapper, we can call it as soon as we start minikube up, rather than possibly waiting for SSH to be available.

Sorry, one more quick idea here. Does it make sense to start loading each image as it becomes available in the cache? Right now you're downloading all the images in parallel, then loading all the images in parallel. What if it was "download and load" all images in parallel?

I had that before, but I think this is actually better. Combining cache and load forces us to wait for the machine to be SSH-able before we do anything (since we'll need a bootstrapper before we run load)

This caches in parallel as soon as we run a minikube start, but then as soon as the machine is ready, it tries a blocking load in parallel. So both cache and load start as soon as they can.

Ah, good call.

dlorenc · 2017-08-31T16:46:10Z

test/integration/functional_test.go

@@ -22,6 +22,8 @@ import (
 	"strings"
 	"testing"

+	"k8s.io/minikube/pkg/minikube/constants"


755 on this file too.

dlorenc · 2017-08-31T16:48:15Z

test/integration/functional_test.go

+	if err := machine.CacheImages(integrationTestImages, constants.ImageCacheDir); err != nil {
+		t.Fatalf("caching images: %s", err)
+	}
+	if err := machine.LoadFromCacheBlocking(&minikubeRunner, constants.ImageCacheDir); err != nil {


We don't have this in a loop. Is that just because we only have one image here? It's used in a loop later on.

Yeah we're just calling the single loader directly for busybox, where the other one loads a list of images.

dlorenc

I think it probably makes sense to leave this behind a flag that's turned off by default at first, and enable it in our CI for a little while, WDYT? The code looks good otherwise.

r2d4 · 2017-09-06T17:41:04Z

I'm waiting to upgrade containers/image and remove the certs.d hack until all of the libraries we vendor use lowercase sirupsen/logrus instead of Sirupsen/logrus.

Sirupsen/logrus
Azure/go-ansiterm
codedellemc/goscaleio
docker/distrubution
docker/docker
docker/go-connections
opencontainers/runc

r2d4 · 2017-09-06T17:42:19Z

Why disable it? It won't error or prevent minikube from running. If it fails a cache or a load, it will simply fail silently and log a warning. Seems like a pretty safe optimization

r2d4 · 2017-09-06T20:37:13Z

Added the feature flag for cache-images, enabled by default.

To disable caching and loading, use minikube start --cache-images=false

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 24, 2017

r2d4 commented Aug 24, 2017

View reviewed changes

r2d4 changed the title ~~Cache images in minikube~~ [WIP] Cache images in minikube Aug 24, 2017

r2d4 force-pushed the cache-images-4-minikube branch from 813d69f to 737bc4e Compare August 24, 2017 23:08

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 24, 2017

r2d4 force-pushed the cache-images-4-minikube branch 2 times, most recently from 812fd4f to 58c8bc6 Compare August 25, 2017 06:17

r2d4 added 7 commits August 30, 2017 17:46

Vendor changes

201e5f9

Keep ~/.minikube/cache directory

967ac2c

We shouldn't blow away the cache on every integration test. keep .minikube for tests hack/jenkins: Delete VM before manual cleanup

Make MinikubeRunner implement CommandRunner

6ac177f

It is useful to be able to have a command runner to call directly in the integration tests

Cache busybox in integration test

eb73bad

Use the build tags for containers/image everywhere

57aa3e6

Set imagePullPolicy:IfNotPresent on kube-dns

e7aa059

r2d4 force-pushed the cache-images-4-minikube branch from 5419365 to e7aa059 Compare August 31, 2017 05:36

r2d4 changed the title ~~[WIP] Cache images in minikube~~ Cache images in minikube Aug 31, 2017

dlorenc reviewed Aug 31, 2017

View reviewed changes

r2d4 mentioned this pull request Aug 31, 2017

Kubeadm bootstrapper #1903

Merged

Review feedback

ac65b58

dlorenc reviewed Sep 6, 2017

View reviewed changes

Add feature flag for cache-images

3fb711c

dlorenc approved these changes Sep 6, 2017

View reviewed changes

r2d4 merged commit 0e6e930 into kubernetes:master Sep 6, 2017

r2d4 deleted the cache-images-4-minikube branch September 6, 2017 22:43

r2d4 mentioned this pull request Sep 9, 2017

Preload default images #1660

Closed

huguesalary mentioned this pull request Sep 26, 2017

Commit 7a9f3be reverted vendor change of PR 1798 #2007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache images in minikube #1881

Cache images in minikube #1881

r2d4 commented Aug 24, 2017

r2d4 Aug 24, 2017

r2d4 commented Aug 24, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

r2d4 commented Aug 31, 2017

dlorenc Aug 31, 2017

r2d4 Sep 6, 2017

dlorenc Aug 31, 2017

r2d4 Aug 31, 2017

dlorenc Aug 31, 2017

r2d4 Aug 31, 2017

dlorenc Aug 31, 2017

dlorenc Aug 31, 2017

r2d4 Aug 31, 2017

r2d4 Aug 31, 2017

r2d4 Sep 6, 2017 •

edited

Loading

dlorenc Sep 6, 2017

r2d4 Sep 6, 2017

dlorenc Sep 6, 2017

dlorenc Aug 31, 2017

r2d4 Sep 6, 2017

dlorenc Aug 31, 2017

r2d4 Aug 31, 2017

dlorenc left a comment

r2d4 commented Sep 6, 2017

r2d4 commented Sep 6, 2017

r2d4 commented Sep 6, 2017

		@@ -67,7 +67,7 @@ KUBE_CROSS_DOCKER_CMD := docker run -w /go/src/$(REPOPATH) --user $(shell id -u)

		# $(call MINIKUBE_GO_BUILD_CMD, output file, OS)

Cache images in minikube #1881

Cache images in minikube #1881

Conversation

r2d4 commented Aug 24, 2017

Choose a reason for hiding this comment

r2d4 commented Aug 24, 2017

codecov-io commented Aug 31, 2017 • edited Loading

Codecov Report

r2d4 commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlorenc left a comment

Choose a reason for hiding this comment

r2d4 commented Sep 6, 2017

r2d4 commented Sep 6, 2017

r2d4 commented Sep 6, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

r2d4 Sep 6, 2017 •

edited

Loading