Don't cache images when --vmdriver=none #4059
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marcosdiez If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @marcosdiez. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/XS
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
size/S
cmd/minikube/cmd/start.go
Outdated
| `docker load -i ~/.minikube/cache/some_images` is unecessary and takes time. | ||
| As a bonus, we save disk space. | ||
| */ | ||
| console.OutStyle("starting-none", "Caching of container images is disabled when --vm-driver=none") |
There was a problem hiding this comment.
Remove this line - minikube, particularly with the none driver, has enough console output as is.
|
Thanks for the PR! It looks like it does the right thing - just one minor comment. |
|
@minikube-bot OK to test |
There was a problem hiding this comment.
Sounds to good me, it is a little disappointing that it reloads images that are already there.
I suppose that is wanted behaviour when using weird "tags" like latest, but maybe it could be fixed for all scenarios - i.e. if we already have the tag (or digest?) maybe we shouldn't be loading anything anywhere
|
@afbjorklund I believe even with weird tags like Kubernetes itself has a every interesting policy for tags. That means if we cache an image with the latest tag, kubernetes will try to download it again anyway. Thinking about it a little bit further, it's worse! Then kubernetes would launch, check the version and download the newest version (because minikube's version is old) Next time I reboot my laptop and run minikube again, minikube would put back the old version of latest and kubernetes would have to download once more. |
|
Yeah, that makes sense... And I guess even if you did reload the same image it would still reuse the layers - haven't tested |
cmd/minikube/cmd/start.go
Outdated
| @@ -174,6 +174,22 @@ func runStart(cmd *cobra.Command, args []string) { | |||
| exit.WithError("Failed to generate config", err) | |||
| } | |||
|
|
|||
| if viper.GetString(vmDriver) == constants.DriverNone { | |||
| /* | |||
| Caching images means: | |||
There was a problem hiding this comment.
I think we can do with a much smaller comment:
// Optimization: images will be persistently loaded into host Docker, so no need to duplicate work.
There was a problem hiding this comment.
And to be policitally correct it should be "the container runtime", not Docker®
k8s-ci-robot
added
size/XS
Since #3917, images are cached by default.
That is great, except that it does not make sense when
--vmdriver=none.When cache is enabled,
minikubedownload the images to~/.minikube/cacheminikubeloads them (docker load -i ~/.minikube/cache/someimage)When
--vmdriver=none, the machine's docker is used, so any loaded image will be stored there "forever" anyway. Hence they are already cached. And setting the cache enable has actually two downsides:minikubelaunch it will load images from~/.minikube/cache/someimageto docker, even though they are there already.So these 5 lines of code do speedup the launch!