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.

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

Add parallels machine driver to supported list #2807

Closed
wants to merge 3 commits into from

Conversation

bassam
Copy link
Contributor

@bassam bassam commented May 14, 2018

This adds support for parallels machine driver.

It still needs CI tests. Can someone please point me in the right direction to add those?

Ref #2756 and #2606

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 14, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bassam
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dlorenc

Assign the PR to them by writing /assign @dlorenc in a comment when ready.

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

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@bassam
Copy link
Contributor Author

bassam commented May 18, 2018

Hi, we're happy to drive CI changes to enable this support. Can someone please point us in the right direction?

@bassam
Copy link
Contributor Author

bassam commented Jun 1, 2018

@dlorenc @aaron-prindle @r2d4 any input here would be appreciated.

@dlorenc
Copy link
Contributor

dlorenc commented Jun 1, 2018

@praveenkumar where should the CI for this live?

@praveenkumar
Copy link
Contributor

@dlorenc The driver changes CI should live to drivers repo and once we have that than minikube should have driver test as part of CI test like it has for hyperkit / virtualbox ..etc.

@bassam
Copy link
Contributor Author

bassam commented Jun 2, 2018

@dlorenc
Copy link
Contributor

dlorenc commented Jun 4, 2018

@dlorenc The driver changes CI should live to drivers repo and once we have that than minikube should have driver test as part of CI test like it has for hyperkit / virtualbox ..etc.

SGTM. @bassam I think @praveenkumar is saying the driver changes from this PR should be reverted and moved to a repo in the drivers org. Does that make sense?

@bassam
Copy link
Contributor Author

bassam commented Jun 4, 2018

@dlorenc the parallels driver itself is in github.com/machine-drivers/docker-machine-parallels. The only changes in this repo is enabling minikube to use it just like virtualbox, etc.

@bassam
Copy link
Contributor Author

bassam commented Jun 19, 2018

@dlorenc ping.

@gbraad
Copy link
Contributor

gbraad commented Jun 19, 2018 via email

@bassam
Copy link
Contributor Author

bassam commented Jun 19, 2018

@gbraad thanks. I believe I've followed the same model as @anfernee VMware fusion implementation. I'd appreciate it if someone can take a deeper look at the changes and let me know what I'm missing.

@dlorenc
Copy link
Contributor

dlorenc commented Jun 21, 2018

@gbraad does this look right to you?

@gbraad
Copy link
Contributor

gbraad commented Jun 21, 2018 via email

@fenos
Copy link

fenos commented Jun 29, 2018

I've tried this PR on my local MAC and I've been successfully able to run minikube with parallels driver. 🎉 well done.

The cluster came up fine. The only issue i've seen is that when running minikube status command, it throws the following error:

E0629 13:32:18.945720   17096 status.go:85] Error cluster status: Error: Unrecognized output from ClusterStatus: Running
E0629 13:32:18.946837   17096 util.go:151] Error formatting error message: Error msg with no stack trace cannot be reported

same when trying to mount a volume with minikube mount [path]:[path]:

E0629 13:34:12.490030   17173 mount.go:113] Error getting the host IP address to use from within the VM:  Error, attempted to get host ip address for unsupported driver

@bassam Well done!

@bassam bassam force-pushed the pr-add-parallels-support branch from a0ff97c to eb9ea85 Compare June 29, 2018 16:43
@bassam
Copy link
Contributor Author

bassam commented Jun 29, 2018

@fenos I rebased and pushed new changes. I'm not seeing the issue with minikube status:

>out/minikube status
minikube: Running
cluster: Running
kubectl: Correctly Configured: pointing to minikube-vm at 10.211.55.17

I wonder what's different about our environments. Is it possible that you're running an older different version of minikube when running status vs. the one in the out/ dir?

@bassam
Copy link
Contributor Author

bassam commented Jul 9, 2018

any update on this? I'm unclear as to what's needed to get this merged.

@bassam
Copy link
Contributor Author

bassam commented Jul 27, 2018

Hi everyone, I realize you're all busy but this PR is at a standstill at this point and has been open for 2+ months. Can someone please point me in the right direction so that we can get this merged?

@lewisbenge
Copy link

@bassam I've merged this PR locally and run it against the most recent version of Parallels. Although a cluster provisions it never starts. It appears there is an issue when the scripts attempt to configure the VM. Have you had any similar issues? (Verbose log below)

Starting cluster components...
E0730 19:18:58.179095 77227 start.go:295] Error starting cluster: kubeadm init error
sudo /usr/bin/kubeadm init --config /var/lib/kubeadm.yaml --ignore-preflight-errors=DirAvailable--etc-kubernetes-manifests --ignore-preflight-errors=DirAvailable--data-minikube --ignore-preflight-errors=Port-10250 --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-scheduler.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-apiserver.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-controller-manager.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-etcd.yaml --ignore-preflight-errors=Swap --ignore-preflight-errors=CRI &&
sudo /usr/bin/kubeadm alpha phase addon kube-dns
running command: : running command:
sudo /usr/bin/kubeadm init --config /var/lib/kubeadm.yaml --ignore-preflight-errors=DirAvailable--etc-kubernetes-manifests --ignore-preflight-errors=DirAvailable--data-minikube --ignore-preflight-errors=Port-10250 --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-scheduler.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-apiserver.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-kube-controller-manager.yaml --ignore-preflight-errors=FileAvailable--etc-kubernetes-manifests-etcd.yaml --ignore-preflight-errors=Swap --ignore-preflight-errors=CRI &&
sudo /usr/bin/kubeadm alpha phase addon kube-dns

.: Process exited with status 1

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 28, 2018
@bassam
Copy link
Contributor Author

bassam commented Oct 28, 2018

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 28, 2018
@bassam
Copy link
Contributor Author

bassam commented Nov 5, 2018

this PR has been open for 6 months and I'm still unsure what needs to be done for it to get merged. Maintainers please help.

/cc @dlorenc @luxas @aaron-prindle @jimmidyson @balopat

@tstromberg
Copy link
Contributor

tstromberg commented Jan 16, 2019

Do you mind addressing the merge conflict? I think it's just due to us moving away from Godeps.

I'd like to see about merging this if it's still viable. My apologies for the unnecessarily long delay in reviewing this PR. I feel that this would be a great addition to the codebase as an initially experimental VM driver.

@bassam
Copy link
Contributor Author

bassam commented Feb 9, 2019

I assume this is no longer needed after #953 was merged?

@bassam
Copy link
Contributor Author

bassam commented Feb 11, 2019

closing in favor of #3650

@bassam bassam closed this Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants