-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Support for mounting host volumes on start with docker driver #8159
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @Asarew! |
Hi @Asarew. 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. |
Can one of the admins verify this patch? |
598b6f1
to
1909c4b
Compare
I signed it |
/check-cla |
Don't know why but the cla-bot doesn't re-check my cla @RA489 any idea on what i should do next? |
do you mind verifying that the e-mail address you have on GitHub matches the one you signed the CLA with? |
Yeah, i double checked that both commits have the correct author email address and that it's the same as my github email and the one i used to sign the CLA. |
Same comment as with #8136, does this have to be docker-specific ? |
Currently this is a docker specific things as and podman are the only drivers that would support this feature. or are you talking about making the flag name not docker specific; something like |
We can copy it from docker-volume to podman-volume, just like with did with docker-env and podman-env. Just that it gets a bit boring after a while, with all the duplication and copy/paste. Since 90% of the implementation is exactly the same, except some minor details like "sudo". And ultimately it would also be nice to align this with extra disk images for the hypervisors...
Yeah, but you can go ahead with this feature without it. We can copy/paste it, after this PR. I don't think we will make it generic anyway, so it is "both" rather than "any" - see #7957 |
I agree with @afbjorklund , we should try to make this not docker secific. on these both PRs I wonder if we should the existing "-mount-string" option on start? instead of making it docker-specific flag? |
then again it already is, so this doesn't really make it much worse I mean since we already have "docker-env" and "docker-opt" :
It's easy enough to copy/paste these into podman-env/podman-opt. Even with a new "docker-vol" (or whatever) flag added here as well. |
@afbjorklund @RA489 I can add the ps: I still need some help with my CLA |
@Asarew do you mind putting the PR description eample of using this so I could test it |
I don't love that this effectively duplicates |
I like this PR. Can you make this use |
@medyagh Figured out what i did wrong, CLA is signed! |
thank you :) I will re-review this PR tomorrow theoverall the PR looks good small minior things might need to change |
@Asarew do you mind addressing the review comments and also update the PR descrtiption with the output of example of using it with vadlidaitons ? (mount a folder and then ls it inside minikube) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Asarew I tried this manually and it didnt seem to work
since the output of ls /dir/med1/ was empty but I was expecting a file to be there
medya@~/workspace/minikube (mount_do1) $ ./out/minikube start --driver=docker --mount --mount-string "/Users/medya/workspace/minikube/med:/dir/med1"
😄 minikube v1.12.1 on Darwin 10.15.6
✨ Using the docker driver based on user configuration
👍 Starting control plane node minikube in cluster minikube
🔥 Creating docker container (CPUs=2, Memory=3892MB) ...
🐳 Preparing Kubernetes v1.18.3 on Docker 19.03.2 ...
🔎 Verifying Kubernetes components...
📁 Creating mount /Users/medya/workspace/minikube/med:/dir/med1 ...
🌟 Enabled addons: default-storageclass, storage-provisioner
🏄 Done! kubectl is now configured to use "minikube"
medya@~/workspace/minikube (mount_do1) $ ./out/minikube ssh
docker@minikube:~$ ls /dir/med1/
docker@minikube:~$ exit
logout
medya@~/workspace/minikube (mount_do1) $ ls /Users/medya/workspace/minikube/med
med.txt
…d added same host mounted volumes logic for the Podman driver
@medyagh I resolved all your comments, but the build is failing on some windows hyperv test. I don't see how this PR can impact those tests as there are clear checks that wouldn't allow any of these changes to run in the hyperv test. Would you be able to rerun the test, just to ensure that not something else is going on? You were correct that it didn't seem to work. I fixed the issue, tested, and added the output to the PR description. |
The Hyper-V test failures are not related to this PR at all. This is looking good. /ok-to-test |
kvm2 Driver Times for Minikube (PR 8159): [61.08094720900001 62.28869980199998 64.22172085199999] Averages Time Per Log
docker Driver Times for Minikube (PR 8159): [25.613505958999998 27.683200873999997 26.28687423] Averages Time Per Log
|
@medyagh i was just thinking that i should add a warning when you try to change the mount-string on |
@medyagh @sharifelgamal @tstromberg is there anything i can do to get this merged asap? |
Done. Thank you for your patience! I like your idea about adding a warning if a mount is attempted after creation. |
Thanks! I'll create a new PR in a couple of days for the warning(s). Do you know hen the next release is scheduled? So i can attempt to get it done before then. |
v1.13 will likely ship by tomorrow, with a v1.13.1 likely next Thursday to catch Kubernetes v1.19.1. |
Warning added in: #9172 👍 |
@Asarew Thanks for so nice addition. I am trying to test it a little bit and got the following error:
Any ideas? |
@zamazan4ik you should run |
@Alig1493 not 100% sure anymore on this as I'm not using minikube anymore, but if i remember correctly you also have to pass the |
To use this PR run the
minikube start
with the--mount
option like so:Description:
this will automatically mount your home directory in the minikube host at path
/minikube-host
Or specify a mount path:
$ minikube start --mount --mount-string "/dir/to/share:/dir/where/to/mount"
Usage:
fixes: #7604