-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add kvm driver support #242
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
Conversation
4204ac1 to
84a6260
Compare
|
So it seems a bit easier... no need for cgo, no dynamic linking... libmachine checks PATH for drivers & executes them if found if available so all. I've reverted back to building with cgo disabled so this is still portable. This should work the same for any of the other supported plugins too which is pretty cool. |
94b0d3b to
507e148
Compare
|
Hey, I'm trying to follow along - it looks like you're still starting the driver in process in client.go, right? How is the plugin binary itself getting compiled, are you relying on the one you have on your PATH from docker-machine? |
|
Well I'm not 100% sure how it works (yet) but it seems like the plugin architecture uses the plugin binary if available in PATH. It does not rely on |
210e165 to
cc712d1
Compare
|
Cool, I like the overall approach but have a few comments that might make this more difficult ;)
We currently operate in mode 1, where minikube acts both as the CLI and as the driver. libmachine sets some environment variables to tell the binary it calls (in this case minikube) what to act as. The code in minikube that checks what to act as is here: https://github.com/kubernetes/minikube/blob/master/pkg/minikube/machine/client.go So with your PR, I'm unclear on how this could be using the standalone binary. If this boolean is set to True: minikube/pkg/minikube/machine/client.go Line 43 in 905afe9
I think libmachine will always use "minikube" as the driver process: minikube/vendor/github.com/docker/machine/libmachine/drivers/plugin/localbinary/plugin.go Line 102 in d74ce77
|
|
At that makes things clearer. Reading the code what you describe is only applicable to core drivers, not drivers from plugins. Relevant line is minikube/vendor/github.com/docker/machine/libmachine/drivers/plugin/localbinary/plugin.go Line 100 in d74ce77
|
|
Ah ok, I didn't realize there was that distinction. In that case, we probably don't need this change here: https://github.com/kubernetes/minikube/pull/242/files#diff-b9679bde9fd46c8d4f93dd0739ad6d01R39 since minikube will never act as the KVM driver. If you remove that, then I don't think any of the new dependencies need to get vendored in, since all that is packaged up in docker-machine-kvm, right? |
|
Re cgo linking, it is not an issue to include c code if one knows how to compile statically. Maybe the standalone binary solution is better, because @dlorenc mentioned earlier that minikube shouldn't be a direct fork of docker-machine for kubernetes with all the drivers compiled in. |
Agreed - I'll refactor this to try to get closer to that. Thanks for the feedback! |
|
So now this has no new dependencies & only requires |
| ### Requirements | ||
|
|
||
| * [VirtualBox](https://www.virtualbox.org/wiki/Downloads) or [VMware Fusion](https://www.vmware.com/products/fusion) installation | ||
| * [VirtualBox](https://www.virtualbox.org/wiki/Downloads), [VMware Fusion](https://www.vmware.com/products/fusion) |
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.
Can you document how to install the docker-machine-kvm driver? It looks like they publish versioned releases, so it would probably be good to include what version you've tested this with as well.
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.
Done.
|
Nice! This looks ready to go. One small request in the docs, then LGTM. I'm excited to try this out! |
|
Also you'll have to run "go run gen_help_text.go" to update the help text so tests pass. |
db2ecce to
6c8636f
Compare
|
Hopefully should be good to go now. |
|
Hey, I pulled your branch to try it out, and I'm having some trouble. I'm getting this error:
Any idea what I'm doing wrong? I installed |
|
D'oh! I tested with a local build which didn't have those options - fixed & tested against 0.7.0 (definitely!) now. |
|
Nice, I got it working! |
|
Yay! |
Note that this requires cgo & dynamic linking to libvirt - this may be a deal
breaker but I'm happy to discuss if there is a way to get this in as I know of
a lot of users that would prefer kvm over virtualbox when running on Linux.
Fixes #238