fix image save cmd #12265
fix image save cmd #12265
Conversation
Previously you could only Copy(To), not CopyFrom. Implies that some Assets can be written to instead.
This is the opposite command of "minikube image load", and can be used after doing a "minikube image build". The default is to save images in the cache, but it is also possible to save to files or to standard output.
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
| @@ -184,6 +184,24 @@ func (e *execRunner) Copy(f assets.CopyableFile) error { | |||
| return writeFile(dst, f, os.FileMode(perms)) | |||
| } | |||
|
|
|||
| // CopyFrom copies a file | |||
| func (e *execRunner) CopyFrom(f assets.CopyableFile) error { | |||
There was a problem hiding this comment.
lets add more explanitory comment ,how is this different from Copy func above ?
There was a problem hiding this comment.
It goes in the other direction, Copy is hard-coded to only CopyTo the VM
There was a problem hiding this comment.
It's an alternative implementation of the API #11296, the other PR looked incomplete
|
kvm2 driver with docker runtime Times for minikube start: 48.6s 46.9s 49.2s 46.4s 48.7s Times for minikube ingress: 32.3s 40.3s 39.3s 40.3s 31.8s docker driver with docker runtime Times for minikube (PR 12265) start: 21.9s 22.2s 21.7s 22.3s 22.4s Times for minikube ingress: 35.5s 36.5s 28.5s 36.0s 35.5s docker driver with containerd runtime Times for minikube start: 44.9s 48.1s 44.4s 43.8s 43.5s |
|
@prezha : thanks for updating, the save feature conflicted with the change in the go-containerregistry |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
Think we need an integration test for SaveImage, but that can go in another PR Some tech debt from PR #11128 (which didn't have any tests, and the bug above) |
|
Hmm, can't test the API without a cmd... Also need "pull" and "tag", for testing. Will push some commits to add the commands + fix and test, hope that is OK ? |
Needed for testing, but maybe elsewhere as well
|
@prezha added a fix for the docker runtime now, as well as a test for SaveImage @medyagh now the PR even grew 2 new commands and another 1 new API... :-) |
|
kvm2 driver with docker runtime Times for minikube start: 49.2s 47.9s 47.8s 48.5s 47.1s Times for minikube ingress: 32.3s 40.8s 39.8s 31.8s 30.8s docker driver with docker runtime Times for minikube start: 24.1s 21.6s 22.2s 21.3s 21.2s Times for minikube ingress: 31.5s 37.5s 35.5s 36.5s 35.5s docker driver with containerd runtime Times for minikube start: 26.7s 43.8s 43.8s 43.1s 44.0s |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
afbjorklund
force-pushed
the
fix-image-save-cmd
branch
from
9959bc2 to
be1b9d3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add a test for SaveImage, similar to LoadImage
afbjorklund
force-pushed
the
fix-image-save-cmd
branch
from
be1b9d3 to
ab61a07
Compare
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
@afbjorklund maybe we should check if the image exists (eg using |
|
re: #12265 (comment) example: |
|
maybe worth noting that docker (haven't check others), if the tag is not specified, saves all images that contain given name (not just exact match with the default latest tag!) and then our save image inherits the same behaviour:
but then:
|
|
kvm2 driver with docker runtime Times for minikube start: 48.2s 47.3s 47.2s 46.7s 47.6s Times for minikube ingress: 30.8s 30.8s 31.2s 31.7s 32.8s docker driver with docker runtime Times for minikube start: 21.3s 21.4s 21.1s 22.6s 21.6s Times for minikube ingress: 27.5s 27.0s 27.5s 26.5s 28.5s docker driver with containerd runtime Times for minikube start: 43.5s 43.2s 43.9s 43.7s 43.0s |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
we probably should make it save just one image (at a time), and append a |
i think that could potentially present an "unexpected behaviour" to those that are familiar with it as-is, ie, at least in the case of docker - 'docker save' cmd (that we use) don't assume ':latest' tag if one was not specified and they save multiple images in a single archive? |
|
I'm not sure it even works with all runtimes, I think OCI only allows for one image for instance ? |
|
kvm2 driver with docker runtime Times for minikube ingress: 30.9s 30.8s 31.8s 30.9s 30.3s Times for minikube (PR 12265) start: 47.4s 47.8s 48.1s 48.4s 46.6s docker driver with docker runtime Times for minikube ingress: 35.0s 27.0s 34.5s 33.6s 26.5s Times for minikube start: 21.5s 22.4s 22.6s 22.5s 22.2s docker driver with containerd runtime Times for minikube start: 34.0s 43.1s 43.3s 43.9s 43.5s |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afbjorklund, prezha 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 |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
@prezha please take look at this failure |
@medyagh i guess the issue is in minikube/test/integration/functional_test.go Line 300 in 900e8f4 ie, effectively postfixing it with an extra $INTEGRATION_TESTS_TO_RUN ( ./test/integration/) and so it cannot be found
hmmm, still fails (but not always) - i bet it has something to do with removing the image in other (parallel!) tests yep, 2nd assumption was right - it's rather a race condition, ie, the ref: #12265 (comment) also worth noting that minikube/pkg/minikube/image/cache.go Lines 74 to 78 in 900e8f4 seems it's getting even more interesting - two race conditions in a single pr! :) here we "don't mind" if the image is corrupted: details: but we don't consider it a failure: minikube/pkg/minikube/machine/cache_images.go Lines 240 to 255 in 378850c btw, i believe that "TestFunctional/serial/LogsCmd" test failure on functional_docker_windows is completely unrelated to this pr |
|
kvm2 driver with docker runtime Times for minikube start: 49.5s 47.4s 48.0s 49.4s 51.3s Times for minikube ingress: 31.4s 32.4s 31.8s 41.1s 39.9s docker driver with docker runtime Times for minikube start: 22.4s 23.0s 22.7s 22.3s 23.0s Times for minikube ingress: 35.5s 35.0s 36.5s 35.5s 36.0s docker driver with containerd runtime Times for minikube start: 45.1s 43.7s 43.7s 44.4s 43.0s |
| if err != nil { | ||
| t.Fatalf("failed to load image to file: %v\n%s", err, rr.Output()) | ||
| } | ||
| defer os.Remove(imageFile) |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
kvm2 driver with docker runtime Times for minikube start: 49.1s 48.9s 51.6s 48.8s 51.1s Times for minikube ingress: 32.9s 31.4s 31.9s 39.4s 31.5s docker driver with docker runtime Times for minikube start: 22.7s 23.3s 23.0s 23.6s 22.1s Times for minikube ingress: 36.5s 36.6s 31.5s 35.5s 36.0s docker driver with containerd runtime Times for minikube start: 44.8s 43.7s 43.7s 44.6s 43.7s |
|
kvm2 driver with docker runtime |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
|
thank you very much @prezha good work |
fix original pr #12162 for:
pkg/minikube/image/image.go:237:27: cannot use ref (type name.Reference) as type name.Tag in argument to daemon.Write: need type assertionCloses #11130