Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

--downlaod-only: handle multiple minikube start at same time. #11223

Merged
merged 25 commits into from
May 14, 2021
Merged

--downlaod-only: handle multiple minikube start at same time. #11223

merged 25 commits into from
May 14, 2021

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Apr 28, 2021 •
edited
Loading

fixes #11166.

This prevents multiple minikube start --download-only from repeatedly downloading the same files (and more importantly preventing each other from corrupting downloaded files).

Running minikube start --download-only two times at the same time:

  • Before: both processes download and write files.
  1. Process 1:
$ minikube start --download-only
😄  minikube v1.19.0 on Debian rodete
✨  Automatically selected the docker driver. Other choices: kvm2, ssh
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
💾  Downloading Kubernetes v1.20.2 preload ...
    > preloaded-images-k8s-v10-v1...: 491.71 MiB / 491.71 MiB  100.00% 21.66 Mi
    > kubeadm.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubectl.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubelet.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm: 37.40 MiB / 37.40 MiB [-------------] 100.00% 12.38 MiB p/s 3.2s
    > kubectl: 38.37 MiB / 38.37 MiB [-------------] 100.00% 12.14 MiB p/s 3.4s
    > gcr.io/k8s-minikube/kicbase...: 360.59 MiB / 360.59 MiB  100.00% 11.70 Mi
    > kubelet: 108.73 MiB / 108.73 MiB [-----------] 100.00% 16.23 MiB p/s 6.9s
  1. Process 2:
$ minikube start --download-only
😄  minikube v1.19.0 on Debian rodete
✨  Automatically selected the docker driver. Other choices: kvm2, ssh
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
💾  Downloading Kubernetes v1.20.2 preload ...
    > preloaded-images-k8s-v10-v1...: 491.71 MiB / 491.71 MiB  100.00% 22.65 Mi
    > kubelet.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubectl.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm: 37.40 MiB / 37.40 MiB [--------------] 100.00% 9.14 MiB p/s 4.3s
    > kubectl: 38.37 MiB / 38.37 MiB [--------------] 100.00% 9.13 MiB p/s 4.4s
    > kubelet: 108.73 MiB / 108.73 MiB [-----------] 100.00% 13.40 MiB p/s 8.3s
✅  Download complete!
  • After: one process downloads and writes files, other process blocks (writing a message to output) until other process finishes downloading and immediately skips downloading the file.
  1. Process 1:
$ minikube start --download-only
😄  minikube v1.19.0 on Debian rodete
✨  Automatically selected the docker driver. Other choices: kvm2, ssh
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
💾  Downloading Kubernetes v1.20.2 preload ...
    > preloaded-images-k8s-v10-v1...: 491.71 MiB / 491.71 MiB  100.00% 21.68 Mi
    > gcr.io/k8s-minikube/kicbase...: 357.46 MiB / 357.47 MiB  100.00% 14.30 Mi
    > kubectl.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubelet.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm.sha256: 64 B / 64 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm: 37.40 MiB / 37.40 MiB [-------------] 100.00% 19.44 MiB p/s 2.1s
    > kubectl: 38.37 MiB / 38.37 MiB [-------------] 100.00% 17.28 MiB p/s 2.4s
    > kubelet: 108.73 MiB / 108.73 MiB [-----------] 100.00% 21.18 MiB p/s 5.3s
✅  Download complete!
  1. Process 2 (Note process 2 did download some files, but there is no duplication).
$ minikube start --download-only
😄  minikube v1.19.0 on Debian rodete
✨  Automatically selected the docker driver. Other choices: kvm2, ssh
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
⌛  Another minikube instance is downloading dependencies...
⌛  Another minikube instance is downloading dependencies...
⌛  Another minikube instance is downloading dependencies...
⌛  Another minikube instance is downloading dependencies...
⌛  Another minikube instance is downloading dependencies...
✅  Download complete!

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 28, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @andriyDev. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andriyDev
To complete the pull request process, please assign sharifelgamal after the PR has been reviewed.
You can assign the PR to them by writing /assign @sharifelgamal in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 28, 2021
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

medyagh
medyagh reviewed Apr 28, 2021
View reviewed changes
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this PR, do u mind adding the full output of multiple minikube start --download-only before'after this PR to compare?

@andriyDev
Copy link
Contributor Author

Fixed, I originally thought there wasn't a good way to show it since one of them just blocks, but I realized you can just tell from the lack of duplication.

Also note there is no message for when it blocks. The lock does not provide the ability to check if the lock is held, only attempt to acquire it. A potential workaround was to print a message before hand and then erase it afterwards, but this results in some nonsense where multiple things try to download at once causing multiple lock messages to show up, and then it is unable to erase them. Or a progress bar will be rendered and the lock message permanently shows up to the right of the progress bar. Regardless, ugly UI stuff is not ideal.

medyagh
medyagh requested changes Apr 28, 2021
View reviewed changes
pkg/minikube/node/cache.go Show resolved Hide resolved
pkg/minikube/download/image.go Show resolved Hide resolved
@medyagh
Copy link
Member

medyagh commented Apr 28, 2021

is there a way to have a Spinner in the UI that tells users waiting on the other terminal to finish ?
the spainner could simply say
🚜 Another minikube instance is downloading depdenencies ...(spinner library)

@andriyDev
Copy link
Contributor Author

is there a way to have a Spinner in the UI that tells users waiting on the other terminal to finish ?
the spainner could simply say
🚜 Another minikube instance is downloading depdenencies ...(spinner library)

Oh I just realized how to do this! The mutex API has no way to check whether the lock is present, but we can run the lock in a goroutine and then select between it and a timer and once the timer expires print the wait message and continue waiting. I need to practice thinking with Go haha.

@andriyDev andriyDev requested a review from medyagh April 29, 2021 18:27
@andriyDev andriyDev requested a review from medyagh May 3, 2021 21:44
@andriyDev
Copy link
Contributor Author

Rebased onto master.

@andriyDev
Copy link
Contributor Author

Rebased onto master x2.

andriyDev added 20 commits May 13, 2021 11:57
@andriyDev
Add tests for ImageToCache/Daemon that they prevent multiple downloads.
458d1d5
@andriyDev
Add locks to binary, image, and preload downloads.
0a44408
@andriyDev
Make functions not used outside the download package private.
b57022d
@andriyDev
Move locking code to download.go.
1cc80e1
@andriyDev
Add message saying when download is waiting for a file lock.
e0a6887
@andriyDev
Use spinner for lock message.
5af0dcf
@andriyDev
Refactor download.go to allow more complex download mocking. Modify d…
fa0803d 
…ownload_test to prevent using logging.
@andriyDev
Make download_test run tests in serial.
4fc0ac5
@andriyDev
Refactor repeated mock sleep function into a single function.
a7cbc76
@andriyDev
Move defer lock release to before error check so the lock is guarante…
ad356d5 
…ed to be released.
@andriyDev
Remove SetDownloadMock and rename mockDownload to DownloadMock.
5e48738
@andriyDev
Ensure directory exists in CreateDstDownloadMock and add comments.
fbc041d
@andriyDev
Rename downloads to downloadNum.
f4b142c
@andriyDev
Reword test fail message.
248d030
@andriyDev
Add function comment to lockDownload.
118eae4
@andriyDev
Remove no longer relevant comment.
e9c4c93
@andriyDev
Check releaser is valid before deferring release.
9191d03
@andriyDev
Add comment to explain lock wait timeout.
0626bb0
@andriyDev
Improve comments for ImageToCache and ImageToDaemon.
c0a492b
@andriyDev
Make download.imageExistsInDaemon public to fix bad compilation.
268e91f
@medyagh medyagh mentioned this pull request May 13, 2021
Merged
@andriyDev
Copy link
Contributor Author

Rebased onto master x5. @medyagh

@medyagh
Copy link
Member

medyagh commented May 13, 2021

/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@andriyDev: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-minikube-build 268e91f link /test pull-minikube-build

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11223) |
+----------------+----------+---------------------+
| minikube start | 51.5s    | 52.8s               |
| enable ingress | 43.5s    | 38.9s               |
+----------------+----------+---------------------+

Times for minikube start: 51.8s 51.4s 50.0s 51.2s 52.9s
Times for minikube (PR 11223) start: 53.9s 51.6s 48.5s 56.1s 54.0s

Times for minikube ingress: 76.8s 34.3s 36.3s 33.4s 36.7s
Times for minikube (PR 11223) ingress: 55.2s 35.3s 35.8s 34.3s 33.7s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11223) |
+----------------+----------+---------------------+
| minikube start | 22.0s    | 21.7s               |
| enable ingress | 37.0s    | 38.1s               |
+----------------+----------+---------------------+

Times for minikube start: 22.2s 21.5s 22.2s 22.4s 21.6s
Times for minikube (PR 11223) start: 22.2s 21.9s 20.7s 22.4s 21.4s

Times for minikube (PR 11223) ingress: 42.5s 38.6s 31.0s 47.0s 31.5s
Times for minikube ingress: 28.5s 65.0s 29.0s 29.0s 33.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11223) |
+----------------+----------+---------------------+
| minikube start | 43.4s    | 45.0s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 43.7s 43.5s 43.2s 43.1s 43.4s
Times for minikube (PR 11223) start: 43.6s 43.6s 47.1s 43.4s 47.1s

@medyagh medyagh merged commit 0132878 into kubernetes:master May 14, 2021
@medyagh medyagh changed the title Add locks to download functions. --downlaod-only: handle multiple minikube start at same time. May 14, 2021
@andriyDev andriyDev deleted the DownloadLocks branch May 14, 2021 16:52
@andriyDev andriyDev mentioned this pull request May 18, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afbjorklund afbjorklund Awaiting requested review from afbjorklund

@tstromberg tstromberg Awaiting requested review from tstromberg

@ilya-zuyev ilya-zuyev Awaiting requested review from ilya-zuyev

@medyagh medyagh Awaiting requested review from medyagh

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

minikube start should wailt till "download-only" is finishing
7 participants
@andriyDev @k8s-ci-robot @minikube-bot @medyagh @sharifelgamal @minikube-pr-bot @ilya-zuyev