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

Embeds xhyve #27

Merged
merged 4 commits into from
Dec 29, 2015
Merged

Conversation

johanneswuerbach
Copy link
Contributor

Embeds xhyve golang bindings

Uses https://github.com/hooklift/xhyve, which is also used by https://github.com/TheNewNormal/corectl

@zchee
Copy link
Member

zchee commented Dec 25, 2015

@johanneswuerbach Thanks PR!

Yes, I knew that repository.
but, Now c4milo seems to have developed here.
https://github.com/xhyve-xyz/xhyve

I think we should include that repository.

How do you think?

@johanneswuerbach
Copy link
Contributor Author

As far as I understand https://github.com/xhyve-xyz/xhyve has no go bindings, just xhyve and some patches.

Looks like hooklift/xhyve#5 there is already work in progress to update the gobindings from the repo you mentioned.

@zchee
Copy link
Member

zchee commented Dec 26, 2015

@johanneswuerbach
Yes.
But, If the development of the https://github.com/xhyve-xyz/xhyve has been accelerated, not promise that libxhyve is follow up.
Despite the new features have been added to the https://github.com/xhyve-xyz/xhyve, but we might not use it.
I think to fork the code of libxhyve inside docker-machine-driver-xhyve(not all), also to submodule control the xhyve-xyz.
How do you think?

BTW, this PR does not run xhyve VM.
I want to put a comment, but I can not browse due there is an updated vendor of git log.

Could you rebase #28 and separating commit of xhyve.go and vendor?

@johanneswuerbach
Copy link
Contributor Author

You are right, this breaks the driver. I'll revisit when I have time, rebased for now.

I think to fork the code of libxhyve inside docker-machine-driver-xhyve(not all), also to submodule control the xhyve-xyz.

Dunno, I'm usually not really a fan of forking and duplicating, but your call. Maybe we could ask the creators of xhyve-xyz to provide an "offical" golang binding or move https://github.com/hooklift/xhyve into that org? @AntonioMeireles @c4milo @jeremyhu @hooklift

@jeremyhu
Copy link

FWIW, the goal of xhyve-xyz is to have a central place to work on xhyve rather than seeing the proliferation of different forks for different needs. I'd much prefer to see work like this inside the main xhyve repo.

I like the idea of making xhyve into a library that can have bindings for different languages and a thin CLI on top of it.

@c4milo
Copy link

c4milo commented Dec 26, 2015

There are a couple of reasons https://github.com/hooklift/xhyve exists:

  • We needed to flatten out the project in order to statically compile the library using Go tooling. Before this, we tried creating a static library of xhyve, linking it from Go using CGO but xhyve did't work correctly using this strategy.
  • We make some calls from C into Go to clean up resources in Go land, as well as to let Go know about serial consoles created so you can connect to the VM from Go.
  • We are also aiming to keep the Go library very lean, eventually there will be features people wants to get into xhyve upstream that would not make sense to have them in the Go library and vice-versa.

@johanneswuerbach
Copy link
Contributor Author

@zchee updated and now it works for me 🙇 Hello 👋 first dependency free docker-machine driver!

@zchee
Copy link
Member

zchee commented Dec 29, 2015

@johanneswuerbach
mist64, He came back.
I will wait and see it.
Sorry :(

and, I want contact you on twitter.

docker-machine expects the first stdout line to be the plugin address,
patch hooklift/xhyve to log to stderr instead
@@ -55,7 +55,7 @@ test-url:
${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} url ${DOCKER_MACHINE_VM_NAME}

driver-run: clean build install driver-kill
rm -rf ${DOCKER_MACHINE_STORAGEPATH}/machines/${DOCKER_MACHINE_VM_NAME} && ${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} create --driver xhyve --xhyve-disk-size ${DOCKER_MACHINE_VM_DISKSIZE} ${DOCKER_MACHINE_VM_NAME}
${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} rm -f ${DOCKER_MACHINE_VM_NAME} && ${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} create --driver xhyve --xhyve-disk-size ${DOCKER_MACHINE_VM_DISKSIZE} ${DOCKER_MACHINE_VM_NAME}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johanneswuerbach
If does not exists xhyve-test VM, occurred error.

Solution

${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} rm -f ${DOCKER_MACHINE_VM_NAME} || true
${DOCKER_MACHINE_CMD} --storage-path ${DOCKER_MACHINE_STORAGEPATH} create --driver xhyve --xhyve-disk-size ${DOCKER_MACHINE_VM_DISKSIZE} ${DOCKER_MACHINE_VM_NAME}

@zchee
Copy link
Member

zchee commented Dec 29, 2015

I have the discussion with @johanneswuerbach in the twitter.
For now merges.

LGTM.

zchee added a commit that referenced this pull request Dec 29, 2015
@zchee zchee merged commit 03e3abe into machine-drivers:master Dec 29, 2015
@johanneswuerbach johanneswuerbach deleted the embedded-xhyve branch December 29, 2015 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants