KicDriver -- less docker-centric approach for kicBase cache #15491
Conversation
We cannot cache :latest images, this check is currently done inside download.CacheToDaemon which prevents from moving a :latest from minikube cache to docker. Moving this check here to prevent pulling a :latest in the first place
This a simpler version of the ImageToDaemon; here the docker save/load logic is thrown away since its already carried inside the previous calls to ImageToMinikubeCache and CacheToKicdriver. This function's only concern is to pull the specified image from remote thus using the proper digest.
we only return err, because messages are already carried by the KicDriver bin itself
..with some handsome emojis
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: x7upLime The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @x7upLime! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @x7upLime. 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/L
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
This output is a little verbose:
😄 minikube v1.28.0 on Darwin 13.1 (arm64)
✨ Automatically selected the docker driver. Other choices: ssh, qemu2 (experimental)
📌 Using Docker Desktop driver with root privileges
👍 Starting control plane node minikube in cluster minikube
🚜 Pulling base image to minikube cache ...
💾 Downloading Kubernetes v1.25.3 preload ...
> preloaded-images-k8s-v18-v1...: 320.81 MiB / 320.81 MiB 100.00% 5.42 Mi
> gcr.io/k8s-minikube/kicbase...: 347.54 MiB / 347.54 MiB 100.00% 5.14 Mi
⌛ Loading cached image to KicDriver...
f50861bc90c0: Loading layer [==================================================>] 27.2MB/27.2MB
433389cc25d1: Loading layer [==================================================>] 289B/289B
07e2914861aa: Loading layer [==================================================>] 287B/287B
a0d1a19e49c4: Loading layer [==================================================>] 255B/255B
a46401d0288f: Loading layer [==================================================>] 823B/823B
1c5713d62a07: Loading layer [==================================================>] 235B/235B
fbfcc16852c1: Loading layer [==================================================>] 854B/854B
54892a678daa: Loading layer [==================================================>] 8.218kB/8.218kB
6d211df2a76a: Loading layer [==================================================>] 2.503kB/2.503kB
f48bafd925d2: Loading layer [==================================================>] 25.09MB/25.09MB
36bfcfdbcfdd: Loading layer [==================================================>] 19.17MB/19.17MB
89a055c76ca9: Loading layer [==================================================>] 21.76MB/21.76MB
9bc402adc4f0: Loading layer [==================================================>] 25.07MB/25.07MB
45f422cdce67: Loading layer [==================================================>] 100.3MB/100.3MB
cf03548dbfea: Loading layer [==================================================>] 43.5MB/43.5MB
657930c837cd: Loading layer [==================================================>] 43.26MB/43.26MB
754f2d10e7e3: Loading layer [==================================================>] 32.53MB/32.53MB
91ca41ae1714: Loading layer [==================================================>] 26.48MB/26.48MB
f5cbe6ba1b63: Loading layer [==================================================>] 221B/221B
40d7674466ab: Loading layer [==================================================>] 298B/298B
4834ae9df7b4: Loading layer [==================================================>] 367B/367B
46591bfc024c: Loading layer [==================================================>] 232B/232B
8914b2218a77: Loading layer [==================================================>] 312B/312B
ad182ad7b531: Loading layer [==================================================>] 354B/354B
9545b5265460: Loading layer [==================================================>] 308B/308B
fd4f85f3397d: Loading layer [==================================================>] 225B/225B
b1fb4bd006dd: Loading layer [==================================================>] 147B/147B
5a18fcb86088: Loading layer [==================================================>] 217B/217B
5f70bf18a086: Loading layer [==================================================>] 32B/32B
e9eccadd57e0: Loading layer [==================================================>] 120B/120B
bde749fea324: Loading layer [==================================================>] 421B/421B
4f608a6c3670: Loading layer [==================================================>] 1.642kB/1.642kB
be6bbbe835a6: Loading layer [==================================================>] 1.642kB/1.642kB
667d63dc291e: Loading layer [==================================================>] 378B/378B
31ceda500c3a: Loading layer [==================================================>] 3.12kB/3.12kB
d8167dbe35dc: Loading layer [==================================================>] 4.085kB/4.085kB
ceebe240057f: Loading layer [==================================================>] 787B/787B
96f3c1a76303: Loading layer [==================================================>] 790B/790B
3a438e55c324: Loading layer [==================================================>] 792B/792B
47d3fe446e91: Loading layer [==================================================>] 538B/538B
4dc0b7262cd4: Loading layer [==================================================>] 159B/159B
845e0ee3a7a9: Loading layer [==================================================>] 100B/100B
f6d74a1ad40c: Loading layer [==================================================>] 189B/189B
Loaded image ID: sha256:65b91ed6e6f9cd324237a2e78683f4d77eebf78e206b192721c46e622ecbeeec
💾 Downloading digest-specific layer to KicDriver...
🔥 Creating docker container (CPUs=2, Memory=4000MB) ...
🐳 Preparing Kubernetes v1.25.3 on Docker 20.10.21 ...
▪ Generating certificates and keys ...
▪ Booting up control plane ...
▪ Configuring RBAC rules ...
🔎 Verifying Kubernetes components...
▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟 Enabled addons: default-storageclass, storage-provisioner
🏄 Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default
I'd be fine having this if the user specifies --alsologtostderr but by default this add a bit much to the output.
| // IsInCache | ||
| // searches in OCIBIN's cache for the IMG; returns true if found. no error handling | ||
| func IsImageInCache(ociBin, img string) bool { | ||
| res, err := runCmd(exec.Command(ociBin, "images", "--format", "{{.Repository}}:{{.Tag}}@{{.Digest}}")) |
There was a problem hiding this comment.
If this command succeeds this always returns false I'm assuming we should also be doing some strings.Contains logic.
There was a problem hiding this comment.
typo.. it should've been "if err == nil"
we only care to do the strings.Contains check if the command returns no error.
if err is returned or string is not found, it's a false
| } | ||
| // Else, pull it | ||
| return false |
There was a problem hiding this comment.
The return true from above can be removed and this can just be return inCache.
There was a problem hiding this comment.
should we keep logging here, or just blindly return inCache?
Co-authored-by: Steven Powell <[email protected]>
|
a change request made me notice a detail that I totally missed.. The workflow in this pr is as follows:
In the docker case the end result is that the image has ok name, no tag and ok digest (--digests has to be specified, tho). If the direct pull is omitted, we got: The current workflow in minikube:master involves go-containerregistry and some hack that in the end preserves tag/digest of the resulting docker image, by pulling the image manifest.. |
|
Dropping the requirement of the digest and accepting a tag is the only sane way of handling the images, that makes them also offline (from the registry) Especially when there is only one digest anyway (release tag) and not a whole list of different images ("latest" tag) |
PS. Don't forget containerd I found that nerdctl goes totally crazy and uses something like "overlayfs" for the image name... Maybe not relevant for driver |
|
Verifying by image "digest" a.k.a. "distribution digest" seems at least tricky to implement in minikube, supporting all the different kicdrivers. One identifier that persists across all the pull/save/load is the "image id" a.k.a. "content digest" and from the below issue I think I understand that it is recommend over "distribution digest" for provenance/content check, since the former is computed locally based on image layer's content, and the latter is computed by the repo at the time the image is received (plus volatile and not usable locally..). content id should be treated equally among all container engines.. ("content digest" || "ditribution digest") --> https://windsock.io/explaining-docker-image-ids/ |
k8s-ci-robot
added
the
needs-rebase
|
@x7upLime: PR needs rebase. 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. |
x7upLime
changed the title
|
Sorry for the fuss.. I thought that in order to accomplish less docker-centric interactions with the kicBase cache, This solves the "not yet implemented, see issue #8426" error message when starting with driver=podman. |
|
/reopen |
|
@x7upLime: Failed to re-open PR: state cannot be changed. The docker-agnostic branch was force-pushed or recreated. In response to this:
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. |
Some code refactoring to make the driver(podman/docker.. now KicDriver) interactions less docker centric.
Tries to simplify the ..DownloadkicBaseImage code in the startup process.
Adds distinction from minikube cache to kicDriver cache in comments/signatures
Adds output steps for transparency on the kicbase download process:
-- before
-- after
(fixes = "it-tries-to-fix")
fixes #13509
fixes #8426 // even if already closed..
fixes #14664