Feature request #7604: use native docker bind mounts with driver-mounts option #8136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @g9rga! |
|
Hi @g9rga. 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
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: g9rga 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 |
|
Can one of the admins verify this patch? |
|
It would be nice if we could do something slightly more portable, instead of adding more docker-only flags ? Not really looking forward to duplicating all of these for podman, like we did for docker-env |
medyagh
requested changes
May 13, 2020
cmd/minikube/cmd/start_flags.go
Outdated
| @@ -101,6 +101,7 @@ const ( | |||
| deleteOnFailure = "delete-on-failure" | |||
| forceSystemd = "force-systemd" | |||
| kicBaseImage = "base-image" | |||
| dockerMounts = "docker-mounts" | |||
There was a problem hiding this comment.
any chance we use the same flag as the one we use for the VMs ?
There was a problem hiding this comment.
Do you mean --mount-string option or something else?
There was a problem hiding this comment.
I am open to any solution that doesn't make it docker driver specifi.
--mount-string could be a solution
or maybe --mount-option
There was a problem hiding this comment.
I agree with @afbjorklund statements in the #8159 (comment).
The --docker-mounts has different attributes like the options for read-only, rslave, etc. i think adding this logic behind the --mount-string won't make the implementation more uniform as the options only applies for the container drivers.
and also:
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? |
g9rga
changed the title
|
@medyagh @afbjorklund thank you for your proposal. I refactored my PR to use existing option |
|
Hi! Just wanted to say thank you for doing this work! Is there anything I can do to help this along? I'd love to see this landed! |
|
@medyagh @afbjorklund what about --driver-mounts option? Format will depend on driver type. |
|
@g9rga do you mind adding an example of using this command in the PR description so I could test it? |
Travis tests have failedHey @g9rga, 1st Buildmake testTravisBuddy Request Identifier: 9f7507e0-ab4d-11ea-b056-2f6ef3b381c7 |
Codecov Report
@@ Coverage Diff @@
## master #8136 +/- ##
==========================================
- Coverage 35.27% 34.08% -1.20%
==========================================
Files 146 153 +7
Lines 9326 9844 +518
==========================================
+ Hits 3290 3355 +65
- Misses 5637 6086 +449
- Partials 399 403 +4
|
g9rga
changed the title
|
@g9rga Could you sign the CLA so we can proceed with testing? |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
ok-to-test
|
kvm2 Driver Times for Minikube (PR 8136): [63.57378907499999 63.03435616099999 62.82572350499999] Averages Time Per Log docker Driver Times for Minikube (PR 8136): [26.094641151 26.080890553 27.602857422000003] Averages Time Per Log |
|
@g9rga Can you fix the outstanding merge conflict? That way we can rerun the tests and make sure everything is ok. |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
@sharifelgamal It fixed. Could you rerun tests? |
medyagh
requested changes
Jul 22, 2020
| @@ -592,3 +592,38 @@ func iptablesFileExists(ociBin string, nameOrID string) bool { | |||
| } | |||
| return true | |||
| } | |||
|
|
|||
| func CreateMounts(mountString string) []Mount { | |||
There was a problem hiding this comment.
add comment for exported funcs
|
@g9rga do you mind trying this PR and see if it works ? and put the output in the PR description ? I did a validation and it didn't seem to be working (if i put a file in the mounted folder I didn't see it in the host) |
Did you try it with existing container? Due to docker architecture, mounts only works for new containers(. I added example output in PR with screenshot. This fix I use for my own dev environment. |
g9rga
force-pushed
the
fix-7604
branch
2 times, most recently
from
fbbc7e9
to
b55f3ae
Compare
|
Would be great if it supported "delegated" / "cached" / "consistent" mount flags which greatly improves performance on MacOS systems. |
|
Is there anything that I can do to help with this PR? This functionality is badly needed in the Docker driver. The |
k8s-ci-robot
added
the
needs-rebase
|
Is there anything that I could do to help this along? I hate to just be sitting on the sidelines cheering! But I'd really love to see this landed. This is currently the only thing preventing us from switching to the docker vm-driver. |
k8s-ci-robot
removed
the
needs-rebase
|
@g9rga In order to make use of this PR, is it sufficient to just compile and replace the minikube binary? |
Yep, you can compile it with |
|
I added integration test for new option |
|
I believe this is obsoleted by #8159 - which does the same thing, but using the existing --mount-string. My apologies for the confusion: I didn't realize there were two PR's out for the same feature. Please /reopen or send a new PR for any of the missing functionality. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello everyone! I added new feature for native driver mount --driver-mounts by request #7604.
At this pr, only docker driver supported. Usage example:
minikube start --driver=docker --driver-mounts=/tmp:/opt/tmpHere is test example output:
(⎈ |N/A:default)➜ minikube git:(fix-7604) mkdir /tmp/123
(⎈ |N/A:default)➜ minikube git:(fix-7604) echo "test" > /tmp/123/file.txt
(⎈ |N/A:default)➜ minikube git:(fix-7604) ./out/minikube-linux-amd64 start -p minikube-test --driver=docker --driver-mounts="/tmp/123:/tmp/123:rw"
😄 [minikube-test] minikube v1.12.1 on Arch 20.0.3
▪ KUBECONFIG=/home/eth/.kube/config
✨ Using the docker driver based on user configuration
👍 Starting control plane node minikube-test in cluster minikube-test
🚜 Pulling base image ...
💾 Downloading Kubernetes v1.18.3 preload ...
> preloaded-images-k8s-v4-v1.18.3-docker-overlay2-amd64.tar.lz4: 526.27 MiB
🔥 Creating docker container (CPUs=2, Memory=3900MB) ...
🐳 Preparing Kubernetes v1.18.3 on Docker 19.03.8 ...
🔎 Verifying Kubernetes components...
🌟 Enabled addons: default-storageclass, storage-provisioner
🏄 Done! kubectl is now configured to use "minikube-test"
(⎈ |minikube-test:default)➜ minikube git:(fix-7604) docker inspect -f '{{ .Mounts }}' minikube-test
[{bind /lib/modules /lib/modules ro false rprivate} {volume minikube-test /var/lib/docker/volumes/minikube-test/_data /var local z true } {bind /tmp/123 /tmp/123 true rprivate}]
(⎈ |minikube-test:default)➜ minikube git:(fix-7604) docker exec minikube-test ls /tmp/123
file.txt
(⎈ |minikube-test:default)➜ minikube git:(fix-7604) docker exec minikube-test cat /tmp/123/file.txt
test
(⎈ |minikube-test:default)➜ minikube git:(fix-7604) docker exec minikube-test sh -c "echo 123 > /tmp/123/file.txt"
(⎈ |minikube-test:default)➜ minikube git:(fix-7604) echo /tmp/123/file.txt
/tmp/123/file.txt
(⎈ |minikube-test:default)➜ minikube git:(fix-7604)