Merge repositories.json after extracting preloaded tarball so that reference store isn't lost #6985
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: priyawadhwa 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
approved
k8s-ci-robot
removed
the
needs-rebase
Codecov Report
@@ Coverage Diff @@
## master #6985 +/- ##
==========================================
+ Coverage 36.92% 37.13% +0.21%
==========================================
Files 144 145 +1
Lines 9033 9123 +90
==========================================
+ Hits 3335 3388 +53
- Misses 5276 5309 +33
- Partials 422 426 +4
|
There was a problem hiding this comment.
@priyawadhwa
do you mind pasting the in the PR description the After this PR, replicating the steps in the issue, to show it is solving the problem?
While this PR fixes the issue for the users with a new profile, I belive if users are upgrading from an older minikube to a newer minikube, the preload will still overwrite their own docker images.
after seeing the evidence that fixes the issue, I am good merging with this PR and this is a great improvement. (both fixing the bug and also perfomance) however I would like us to create another issue to track the users that will upgrade minikube and have existing user docker files.
the least is we should warn them that if they use preload they will loose their images.
|
I test this locally and it works for me medya@~/workspace/minikube (priyawadhwa-no-overwrite-tar) $ ./out/minikube start 😄 minikube v1.8.1 on Darwin 10.14.6 ✨ Automatically selected the hyperkit driver 🔥 Creating hyperkit VM (CPUs=2, Memory=4096MB, Disk=20000MB) ... 🐳 Preparing Kubernetes v1.17.3 on Docker 19.03.6 ... 🚀 Launching Kubernetes ... 🌟 Enabling addons: default-storageclass, storage-provisioner ⌛ Waiting for cluster to come online ... 🏄 Done! kubectl is now configured to use "minikube" |
|
PR looks like a great optimization and approvable in its own right, but I'm not certain it fully fixes #6981 For instance, if a user has a v1.16.0 Kubernetes cluster with custom images, won't |
k8s-ci-robot
added
size/L
|
ImagesPreloaded(r.Runner, images) should be able to handle based on different type of runtimes, docker, cri-o, containerd. |
|
@medyagh agreed, I'll probably do that in another PR or when I start to look at adding the other container runtimes |
|
PR title could use a more accurate take |
There was a problem hiding this comment.
to look at adding the o
sounds good, I am good with with PR, once @tstromberg 's comments are resolved :)
priyawadhwa
changed the title
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
Error: running mkcmp: exit status 1 |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/ok-to-test |
|
All Times minikube: [ 60.053562 61.222229 63.681110] Average minikube: 61.652300 Averages Time Per Log |
|
Update, that bug bellow is not related to this PR. I will create an issue for it. ~~ to replicate, do and export the vars then minikube start again then |
This PR adds a new
Storagetype which tracks the state of therepositories.jsonfile, or reference store. This file is used by the docker daemon to store image references, and is overwritten when the preload tarball is extracted.The
Storagetype is used to keep the state of the reference store in memory before and after extraction. The two reference stores are merged and then the file is updated.This PR also adds a unit test and integration test to test this functionality.
should fix #6981