-
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
feat: add VMware driver support for new minikube ISO #16796
Conversation
Welcome @lbogdan! |
Hi @lbogdan. 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? |
@lbogdan thank you very much for this contribution, do you mind sharing output of "minikube start" Before / After this PR ? |
/ok-to-test |
Sure! Without this PR, latest
With this PR on top of
|
There are quite some lint errors caused by the vendoring of But before I do, please confirm vendoring is the way to go here. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
We are working towards lifting all the drivers out, so it is probably not the way to go (long term). It is of course a perfectly reasonable way to test it, so that one doesn't have to only use "docker-machine" But we made a related change, to stop using the external binary in anticipation of "minikube-machine" I would have expected that this password removal would happen on docker-machine-driver-vmware ? What we do for docker/machine and for the other drivers, is that we replace them with a patched fork. Eventually, we want to start using the new organization: Not sure where it would live, separately or in the main repo ? The reason for forking libmachine and all the drivers, is both that it is dead and that we want to add new API. Related to this, we probably want to lift "minikube-iso" out again since it has become a very big component. @lbogdan : finally, thanks for fixing the vmware driver! I think we want something similar for the parallels driver. |
This would probably make the most sense, but considering there was no activity there for over an year, and the "fix" is unrelated to the actual Another approach would be to fork it, but that would mean managing a new repository. I would suggest to go with vendoring it (is this the same as what you call "forked in the minikube mono-repo"?) short term, and then include it in the process of lifting out the drivers to Unrelated, I see most of the Jenkins integration tests fail (flake?), is this a known issue, and is anyone looking into it? |
If by vendoring you mean copying it to the minikube repository under pkg/drivers, then it is not the same thing. If you include it in the The main difference is that in docker-machine, the drivers were standalone grpc programs and did not share code. The "Config" structure was never exported, and all the settings were transferred using the docker-machine string Flags. Now in minikube, the driver is referenced in the code base (from the "registry") and all the code is vendored in.
* commit 118783c removed k8s.io/minikube/pkg/drivers/kvm
So the main difference was to build the driver in as well, except for those where it makes sense to link it separately:
|
I've now also fixed the lint errors. In the process, I added error checks to the So I went ahead and removed all |
The main outstanding question is if all drivers should be included with minikube-machine/machine, or if some should be in their own repositories still. Either way, they will all be included by minikube (in some way) - just like they were before. i.e. do we create a directory: minikube-machine/machine/pkg/drivers/vmware or do we create a repository: minikube-machine/minikube-machine-driver-vmware |
Oh, OK, then I guess I had a more inclusive understanding of vendoring, meaning "copying a dependency's source files into the project". 🙂 |
I'm not sure the terms are properly defined anywhere, just as long as we understand what we are talking about. The roadmap items would be to move "minikube-machine" (including libmachine, drivers, and minikube-iso) out of the monorepo. With the increased focus on the KIC drivers, it could make more sense to have the VM and ISO separate. There is some bigger picture discussion that should happen on the roadmap planning - or in the Slack channel perhaps |
Well, I'm only interested in fixing VMware support as quick as possible, so let me know if and how I can help in getting this in the next release. |
Patching github.com/machine-drivers/docker-machine-driver-vmware v0.1.5 would be a faster route. i.e. create a fork of the repository, add the needed changes to a branch, and use that with |
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.
Would prefer patching the docker-machine-driver-vmware repository, instead of removing it:
diff --git a/go.mod b/go.mod
index 4637bb9f6f5..4a8c18806c8 100644
--- a/go.mod
+++ b/go.mod
@@ -33,7 +33,6 @@ require (
github.com/johanneswuerbach/nfsexports v0.0.0-20200318065542-c48c3734757f
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
github.com/klauspost/cpuid v1.2.0
- github.com/machine-drivers/docker-machine-driver-vmware v0.1.5
github.com/mattbaird/jsonpatch v0.0.0-20200820163806-098863c1fc24
github.com/mattn/go-isatty v0.0.19
github.com/mitchellh/go-ps v1.0.0
Sounds good! Should I fork it under my GitHub |
While testing on macOS, I've noticed a small issue: the minikube/pkg/minikube/registry/drvs/vmware/vmware.go Lines 59 to 65 in f58582b
I was thinking that we can export |
You actually don't have to tag your fork, Go will "make something up" for a version if you just give it the commit Normally we don't tag forks, since if upstream wakes up of their coma we would suddenly end up with two |
Fixing the duplicate check in the "registry" sounds like the most obvious, if it is currently broken (can't find It is not possible to export new functions in libmachine, which is one of the reasons that we want minikube-machine |
Can't we export (capitalize the first letter of) func status() registry.State {
cmd := vmware.SetVmwareCmd("vmrun")
if cmd == "vmrun" || cmd == "" { // if it can't find it, it just returns the argument (or an empty string, on Windows)
return registry.State{Error: errors.New("vmrun not found"), Fix: "Install VMware", Doc: "https://minikube.sigs.k8s.io/docs/reference/drivers/vmware/"}
}
return registry.State{Installed: true, Healthy: true}
} ? Later edit: tried it and it works fine. |
If you meant removing the With updated check:
Without check:
|
Actual I was thinking more like fixing the check, by adding the missing paths on the missing platforms. missed in commit 3c48985 |
So just go ahead and duplicate all the logic from Windows - reads from the Windows registry: https://github.com/lbogdan/docker-machine-driver-vmware/blob/a391c48b14d523058072d491f0fec2296dd461e4/pkg/drivers/vmware/vmware_windows.go#L40-L70 macOS - looks into an additional path: https://github.com/lbogdan/docker-machine-driver-vmware/blob/a391c48b14d523058072d491f0fec2296dd461e4/pkg/drivers/vmware/vmware_darwin.go#L39-L53 |
Yeah, not great either... Having some feature for it in the new API would be good, I think? Could use the same thing for VBoxManage or qemu-system-x86_64/qemu-system-aarch64 etc |
It is just that at some path in the future we want to restore the feature of decoupling the "registry" and the drivers... So we should try to not import any code from the drivers, outside of the libmachine API... Including the |
thank you @lbogdan, I see @afbjorklund already reviewing the PR, once @afbjorklund approves the PR, you got mine too :) |
36238f4
to
380359c
Compare
@@ -235,7 +235,7 @@ replace ( | |||
github.com/Parallels/docker-machine-parallels/v2 => github.com/minikube-machine/machine-driver-parallels/v2 v2.0.1 | |||
github.com/briandowns/spinner => github.com/alonyb/spinner v1.12.7 | |||
github.com/docker/machine => github.com/minikube-machine/machine v0.0.0-20230610170757-350a83297593 | |||
github.com/machine-drivers/docker-machine-driver-vmware => github.com/minikube-machine/machine-driver-vmware v0.1.5 | |||
github.com/machine-drivers/docker-machine-driver-vmware => github.com/lbogdan/docker-machine-driver-vmware v0.2.0 |
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.
@lbogdan do you mind making this PR to the new Org https://github.com/minikube-machine/machine-driver-vmware ?
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.
Yes, I will get to it on Friday, weekend at the latest.
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.
For getting the release out in time we'll merge this and then change it back to our fork once the change gets merged upstream.
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.
Please don't merge this yet, I want to get back to it on Friday, weekend at the latest. |
kvm2 driver with docker runtime
Times for minikube start: 52.6s 54.8s 53.2s 50.5s 53.1s Times for minikube ingress: 27.8s 29.3s 28.4s 27.9s 28.9s docker driver with docker runtime
Times for minikube start: 26.0s 25.5s 25.8s 26.2s 25.5s Times for minikube ingress: 48.4s 48.4s 49.4s 48.9s 48.4s docker driver with containerd runtime
Times for minikube ingress: 31.6s 31.4s 17.9s 31.4s 31.4s Times for minikube start: 23.7s 21.6s 21.0s 24.0s 23.3s |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbogdan, medyagh, spowelljr 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.
To see the flake rates of all tests by environment, click here. |
Resolves #16221
This PR adds VMware driver support for the new minikube ISO, which removed the password for the
docker
user in #15849.This ISO change broke the VMware driver, because it uses
ssh
andvmrun
commands , which assume that thedocker
user password istcuser
, in order to initialize the VM.It uses the same approach as the QEMU driver: it creates a
tar
stream having the"boot2docker, please format-me"
magic string and the public key in.ssh/authorized_keys
and writes it to the beginning of the VM disk, which will then be detected and used by the automount script during the first boot.To be able to make changes to
docker-machine-driver-vmware
, this vendors itsmaster
branch intopkg/drivers/vmware
, movingconfig/config.go
to the parent folder and to thevmware
package. To check the actual changes, see the second commit.