Load kicbase image from right cache and add log #11346

afbjorklund · 2021-05-08T07:30:41Z

It was silently failing to load from the (wrong) cache directory,
causing the image to be download twice from the network instead.

🚜  Pulling base image ...
    > gcr.io/k8s-minikube/kicbase...: 358.10 MiB / 358.10 MiB  100.00% 15.00 Mi
    > gcr.io/k8s-minikube/kicbase...: 358.10 MiB / 358.10 MiB  100.00% 9.23 MiB
🔥  Creating docker container (CPUs=2, Memory=7900MB) ...

Also the name of the function was misleading.
It takes an image, and not actually a filename.

Closes #11321

k8s-ci-robot · 2021-05-08T07:30:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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:

~~OWNERS~~ [afbjorklund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

medyagh · 2021-05-11T17:33:37Z

/ok-to-test

minikube-pr-bot · 2021-05-11T18:39:36Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 52.7s    | 50.9s               |
| enable ingress | 35.1s    | 38.8s               |
+----------------+----------+---------------------+

Times for minikube ingress: 34.3s 35.7s 34.3s 35.8s 35.3s
Times for minikube (PR 11346) ingress: 43.8s 43.3s 35.7s 35.2s 35.8s

Times for minikube start: 52.3s 48.2s 52.0s 55.2s 55.8s
Times for minikube (PR 11346) start: 50.5s 47.7s 53.7s 53.4s 49.5s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 21.5s    | 24.1s               |
| enable ingress | 34.2s    | 32.8s               |
+----------------+----------+---------------------+

Times for minikube start: 22.3s 20.8s 21.6s 21.2s 21.7s
Times for minikube (PR 11346) start: 24.4s 23.6s 24.1s 23.9s 24.4s

Times for minikube ingress: 30.0s 38.0s 36.5s 32.0s 34.5s
Times for minikube (PR 11346) ingress: 32.5s 32.5s 32.0s 33.5s 33.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 41.5s    | 46.1s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 30.5s 47.6s 43.1s 43.1s 43.0s
Times for minikube (PR 11346) start: 49.3s 44.6s 45.6s 45.1s 45.9s

medyagh · 2021-05-11T19:19:42Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 52.7s    | 50.9s               |
| enable ingress | 35.1s    | 38.8s               |
+----------------+----------+---------------------+

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 21.5s    | 24.1s               |
| enable ingress | 34.2s    | 32.8s               |
+----------------+----------+---------------------+

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 41.5s    | 46.1s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

@afbjorklund could this PR legit made minikube start slower ?

medyagh · 2021-05-11T21:35:53Z

/retest-this-please

medyagh · 2021-05-12T20:10:54Z

/retest-this-please

medyagh · 2021-05-13T18:59:03Z

lets pull upstream after this PR is merged #11223

medyagh · 2021-05-14T01:57:33Z

@afbjorklund other PR merged. Ready for pulling upstream

It was silently failing to load from the (wrong) cache directory, causing the image to be download twice from the network instead. Add new function to get the path in the cache directory, so that we don't have to duplicate that both inside and outside module.

afbjorklund · 2021-05-14T08:24:21Z

@andriyDev
I made the existance check public again, because it looked assymetric in the mini-API.

func ImageExistsInCache(img string) bool
func ImageExistsInDaemon(img string) bool
func ImageToCache(img string) error
func ImageToDaemon(img string) error

For some reason we do download and load separate for the cache, but not for daemon ?
i.e. it is hidden away in the go-containerregistry abstraction, which just calls both of them

https://github.com/google/go-containerregistry/blob/main/pkg/v1/daemon/write.go#L38

Possibly we should move the remaining function into "download" as well, but name is odd.

download.ImagePathInCache
image.LoadFromTarball

Something like:

download.ImageFromCache (?)
download.CacheToDaemon (?)

And we still have Docker hardcoded (as Daemon), so there is no support for Podman yet.

#8577 (~~PR #11063~~ ~~PR #11356~~)

daemon.Write

func ImageExistsInPodman(img string) bool
func ImageToPodman(img string) error

minikube-pr-bot · 2021-05-14T09:03:15Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 46.7s    | 46.3s               |
| enable ingress | 36.5s    | 39.8s               |
+----------------+----------+---------------------+

Times for minikube start: 47.5s 45.6s 45.6s 44.9s 50.1s
Times for minikube (PR 11346) start: 46.9s 44.9s 48.4s 44.8s 46.3s

Times for minikube ingress: 43.2s 35.7s 35.2s 34.3s 33.8s
Times for minikube (PR 11346) ingress: 42.2s 35.7s 35.7s 43.2s 42.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 22.0s    | 21.4s               |
| enable ingress | 33.5s    | 34.1s               |
+----------------+----------+---------------------+

Times for minikube (PR 11346) ingress: 33.5s 33.0s 35.5s 35.0s 33.5s
Times for minikube ingress: 34.5s 32.5s 33.5s 33.0s 34.0s

Times for minikube start: 22.7s 21.7s 22.1s 21.8s 21.8s
Times for minikube (PR 11346) start: 21.4s 21.1s 21.1s 21.5s 21.9s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 42.8s    | 43.7s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube (PR 11346) start: 41.6s 43.2s 43.2s 47.2s 43.3s
Times for minikube start: 31.1s 43.9s 46.8s 43.1s 49.3s

Now we don't have to expose the local cache directory path, and don't risk mixing the kicbase code with the image code. Duplicate the digest code for now, it should use the Digest implementation Tag() from our patched go-containerregistry.

It allows adding a tag to a Digest, by doing a "docker pull" in the daemon.Write function after doing the "docker load" The digest is not stored in the tarball, so it cannot be loaded from it. The alternative is instead using a Tag.

minikube-pr-bot · 2021-05-14T16:11:42Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 51.3s    | 52.8s               |
| enable ingress | 37.9s    | 37.9s               |
+----------------+----------+---------------------+

Times for minikube start: 53.0s 52.2s 48.8s 48.1s 54.5s
Times for minikube (PR 11346) start: 51.7s 52.3s 55.4s 55.9s 48.9s

Times for minikube ingress: 34.8s 35.4s 42.7s 34.3s 42.4s
Times for minikube (PR 11346) ingress: 43.5s 34.8s 33.7s 35.4s 42.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 22.5s    | 22.0s               |
| enable ingress | 32.0s    | 32.2s               |
+----------------+----------+---------------------+

Times for minikube start: 23.5s 22.1s 22.0s 21.7s 23.3s
Times for minikube (PR 11346) start: 21.7s 21.8s 22.3s 22.1s 22.2s

Times for minikube ingress: 32.5s 32.5s 30.5s 33.0s 31.5s
Times for minikube (PR 11346) ingress: 31.0s 34.0s 32.5s 33.1s 30.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 43.1s    | 46.3s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube (PR 11346) start: 49.2s 43.3s 47.9s 43.1s 48.0s
Times for minikube start: 31.4s 43.4s 47.5s 44.3s 49.1s

andriyDev · 2021-05-14T16:31:22Z

@andriyDev
I made the existance check public again, because it looked assymetric in the mini-API.
func ImageExistsInCache(img string) bool
func ImageExistsInDaemon(img string) bool
func ImageToCache(img string) error
func ImageToDaemon(img string) error
For some reason we do download and load separate for the cache, but not for daemon ?
i.e. it is hidden away in the go-containerregistry abstraction, which just calls both of them

https://github.com/google/go-containerregistry/blob/main/pkg/v1/daemon/write.go#L38

Possibly we should move the remaining function into "download" as well, but name is odd.

download.ImagePathInCache
image.LoadFromTarball

Something like:

download.ImageFromCache (?)
download.CacheToDaemon (?)

And we still have Docker hardcoded (as Daemon), so there is no support for Podman yet.

#8577 (~~PR #11063~~ ~~PR #11356~~)

daemon.Write
func ImageExistsInPodman(img string) bool
func ImageToPodman(img string) error

How about instead of making ImageExistsInCache public, we make ImageExistsInDaemon private? Our only use case is in cache.go. We should move that call into download.CacheToDaemon.

I am a little concerned however on the semantics of "download.CacheToDaemon" - there's no download involved there, which is a big reason the ImageToCache/Daemon functions were moved from the image package.

I wonder if our assumption is wrong. Why do we have ImageToDaemon? Perhaps we should force the only route to be to download to the cache, then load the tarball. Else we may end up having people download images to their docker multiple times through restarts (maybe I'm misunderstanding that though).

minikube-pr-bot · 2021-05-14T16:37:04Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 53.9s    | 53.9s               |
| enable ingress | 35.8s    | 36.2s               |
+----------------+----------+---------------------+

Times for minikube (PR 11346) start: 56.1s 52.5s 53.0s 56.3s 51.5s
Times for minikube start: 57.0s 56.4s 52.4s 52.1s 51.5s

Times for minikube ingress: 37.8s 34.9s 34.8s 36.4s 35.3s
Times for minikube (PR 11346) ingress: 36.4s 36.4s 37.8s 35.4s 34.9s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 23.3s    | 22.1s               |
| enable ingress | 30.2s    | 29.2s               |
+----------------+----------+---------------------+

Times for minikube ingress: 27.5s 33.5s 29.0s 27.5s 33.5s
Times for minikube (PR 11346) ingress: 27.5s 28.5s 33.0s 29.0s 28.0s

Times for minikube start: 24.9s 21.5s 23.7s 23.4s 23.2s
Times for minikube (PR 11346) start: 22.5s 21.4s 21.7s 22.6s 22.1s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 43.1s    | 45.3s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 31.2s 44.1s 43.8s 47.5s 48.7s
Times for minikube (PR 11346) start: 45.2s 44.2s 43.1s 48.9s 45.0s

afbjorklund · 2021-05-14T18:11:40Z

I am a little concerned however on the semantics of "download.CacheToDaemon" - there's no download involved there, which is a big reason the ImageToCache/Daemon functions were moved from the image package.

Yeah, the name was not great.

I wonder if our assumption is wrong. Why do we have ImageToDaemon? Perhaps we should force the only route to be to download to the cache, then load the tarball. Else we may end up having people download images to their docker multiple times through restarts (maybe I'm misunderstanding that though).

I opened a story for it: #11411

Restarting the host docker will not delete the images, though. You would have to use rmi or prune, before they are removed.

This can actually be a problem, because currently there are a lot of 1 GB images being added to the code base automatically...

* e8b16a524 Updating kicbase image to v0.0.22-1620785771-11384
* 625a06ce6 Update kicbase to v0.0.22
* c5b65cab5 Update kicbase to v0.0.21
* fff23925e Updating kicbase image to v0.0.20-1619543236-11212
* 0e1fd7204 Updating kicbase image to v0.0.20-1618262794-11055
* f7c204d90 Update kicbase to v0.0.20
* 831c3dfdb Updating kicbase image to v0.0.19-1617905290-11019
* 53babcc15 Update kicbase to v0.0.19

Author: minikube-bot

afbjorklund · 2021-05-14T18:14:47Z

How about instead of making ImageExistsInCache public, we make ImageExistsInDaemon private? Our only use case is in cache.go. We should move that call into download.CacheToDaemon.

I think it currently skips using that function, and instead always tries to load the (possibly non-existent) file and fails silently.

download.ImageToCache
download.ImageExistsInDaemon
#download.ImageExistsInCache
download.CacheToDaemon

At some point it should give up on the image and the registry if they fail to download, and move on to the fallback images.

It should still work the way it is now, just with some extra logging and some calls that are bound to fail in some places.

afbjorklund · 2021-05-14T18:32:36Z

Having "daemon" in the API is not great either, but that was inherited from go-containerregistry

However, it is not portable to other engines. It even has a docker client, importing all of that code.

github.com/docker/docker/client

type imageOpener struct {
	ref      name.Reference
	buffered bool
	client   Client
}

And I guess saving the image to memory or tempfile is going to fail at some point, for any big images.

See #8577 (comment)

medyagh · 2021-05-18T19:53:26Z

pkg/minikube/download/image.go

+}
+
+// ImageExistsInCache if img exist in local cache directory
+func ImageExistsInCache(img string) bool {


I dont see this func be used outside download paockage any reason it is public ?
and also how about we move it to image package image.ExistsInCache(img) reads much better than download.ImageExistsInCache

wdyt @afbjorklund

#11346 (comment)

medyagh · 2021-05-27T16:56:59Z

@afbjorklund would u please (I tried to push to the branch but it didnt let me)

…icbase-load

minikube-pr-bot · 2021-05-27T18:41:12Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 48.2s    | 46.6s               |
| enable ingress | 35.7s    | 34.8s               |
+----------------+----------+---------------------+

Times for minikube (PR 11346) start: 46.9s 46.3s 47.6s 46.5s 46.0s
Times for minikube start: 50.9s 46.3s 50.0s 46.9s 46.8s

Times for minikube ingress: 34.2s 35.2s 41.2s 34.2s 33.8s
Times for minikube (PR 11346) ingress: 34.2s 34.7s 34.8s 34.7s 35.8s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 21.9s    | 21.8s               |
| enable ingress | 30.1s    | 31.5s               |
+----------------+----------+---------------------+

Times for minikube start: 22.6s 21.2s 21.8s 22.4s 21.7s
Times for minikube (PR 11346) start: 22.0s 21.4s 21.3s 21.4s 23.1s

Times for minikube (PR 11346) ingress: 32.5s 34.5s 31.5s 29.0s 30.0s
Times for minikube ingress: 31.5s 27.5s 32.0s 33.5s 26.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11346) |
+----------------+----------+---------------------+
| minikube start | 41.7s    | 43.8s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 30.8s 44.1s 47.0s 42.9s 43.4s
Times for minikube (PR 11346) start: 42.6s 42.9s 43.2s 47.1s 43.3s

medyagh · 2021-05-27T20:07:01Z

the containerd test failures seem to be flake

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2021

k8s-ci-robot requested review from prasadkatti and tstromberg May 8, 2021 07:30

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 8, 2021

afbjorklund force-pushed the kicbase-load branch from e414285 to 95d51cd Compare May 10, 2021 10:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2021

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2021

afbjorklund force-pushed the kicbase-load branch from 95d51cd to b974893 Compare May 11, 2021 20:43

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2021

afbjorklund force-pushed the kicbase-load branch from b974893 to 580161a Compare May 14, 2021 08:20

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 14, 2021

afbjorklund requested review from medyagh and removed request for tstromberg and prasadkatti May 14, 2021 08:24

afbjorklund added 2 commits May 14, 2021 17:08

Move load function from image to download

3bcfe1b

Now we don't have to expose the local cache directory path, and don't risk mixing the kicbase code with the image code. Duplicate the digest code for now, it should use the Digest implementation Tag() from our patched go-containerregistry.

Use patched version of go-containerregistry

748d9cb

It allows adding a tag to a Digest, by doing a "docker pull" in the daemon.Write function after doing the "docker load" The digest is not stored in the tarball, so it cannot be loaded from it. The alternative is instead using a Tag.

afbjorklund mentioned this pull request May 14, 2021

Simplify kicbase cache handling by removing digest #11411

Closed

medyagh reviewed May 18, 2021

View reviewed changes

andriyDev mentioned this pull request May 18, 2021

REQUEST: New membership for andriyDev kubernetes/org#2718

Closed

6 tasks

Merge branch 'master' of github.com:kubernetes/minikube into anders-k…

469d0d5

…icbase-load

medyagh merged commit 0d3c246 into kubernetes:master May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load kicbase image from right cache and add log #11346

Load kicbase image from right cache and add log #11346

afbjorklund commented May 8, 2021 •

edited

Loading

k8s-ci-robot commented May 8, 2021

medyagh commented May 11, 2021

minikube-pr-bot commented May 11, 2021

medyagh commented May 11, 2021

medyagh commented May 11, 2021

medyagh commented May 12, 2021

medyagh commented May 13, 2021

medyagh commented May 14, 2021

afbjorklund commented May 14, 2021 •

edited

Loading

minikube-pr-bot commented May 14, 2021

minikube-pr-bot commented May 14, 2021

andriyDev commented May 14, 2021 •

edited

Loading

minikube-pr-bot commented May 14, 2021

afbjorklund commented May 14, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading

medyagh May 18, 2021

afbjorklund May 19, 2021

medyagh commented May 27, 2021

minikube-pr-bot commented May 27, 2021

medyagh commented May 27, 2021

Load kicbase image from right cache and add log #11346

Load kicbase image from right cache and add log #11346

Conversation

afbjorklund commented May 8, 2021 • edited Loading

k8s-ci-robot commented May 8, 2021

medyagh commented May 11, 2021

minikube-pr-bot commented May 11, 2021

medyagh commented May 11, 2021

medyagh commented May 11, 2021

medyagh commented May 12, 2021

medyagh commented May 13, 2021

medyagh commented May 14, 2021

afbjorklund commented May 14, 2021 • edited Loading

minikube-pr-bot commented May 14, 2021

minikube-pr-bot commented May 14, 2021

andriyDev commented May 14, 2021 • edited Loading

minikube-pr-bot commented May 14, 2021

afbjorklund commented May 14, 2021 • edited Loading

afbjorklund commented May 14, 2021 • edited Loading

afbjorklund commented May 14, 2021 • edited Loading

medyagh May 18, 2021

Choose a reason for hiding this comment

afbjorklund May 19, 2021

Choose a reason for hiding this comment

medyagh commented May 27, 2021

minikube-pr-bot commented May 27, 2021

medyagh commented May 27, 2021

afbjorklund commented May 8, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading

andriyDev commented May 14, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading

afbjorklund commented May 14, 2021 •

edited

Loading