search

machine-drivers / docker-machine-driver-xhyve

docker-machine/minikube/minishift driver plugin for xhyve/hyperkit (native macOS hypervisor.framework)
https://godoc.org/github.com/machine-drivers/docker-machine-driver-xhyve
BSD 3-Clause "New" or "Revised" License
888 stars 74 forks source link

Move the NFS and 9p mounting code from Create to Start. #158

Closed dlorenc closed 7 years ago

dlorenc commented 7 years ago

This will re-mount on a re-start.

This should fix https://github.com/kubernetes/minikube/issues/1133

zchee commented 7 years ago

@dlorenc SGTM, also maybe I understand why it happened. Sorry, it's my mistake. I'll check it. Thanks.

dlorenc commented 7 years ago

Hmm, we might need to keep this in both places actually. Trying to figure out the way the API works...

zchee commented 7 years ago

Hm, I have not seen the whole code yet, but that is possibility. libmachine sometimes require redundant, such as implemented root binary check etc.

dlorenc commented 7 years ago

Yeah it looks like we do need this in both places. Refactored a bit. Sorry for the confusion.

zchee commented 7 years ago

@dlorenc np! I'll try to check and test it.

zchee commented 7 years ago

@dlorenc BTW, There are pull requests that support multiple NFS shares. WDTY?

dlorenc commented 7 years ago

Seems reasonable, I'm not sure of the use-case though. Maybe some of that could be better handled with modifications of bootlocal.sh inside the iso? https://github.com/boot2docker/boot2docker/blob/master/doc/FAQ.md#local-customisation-with-persistent-partition

zchee commented 7 years ago

@dlorenc Yeah maybe needs to fix this line, https://github.com/zchee/docker-machine-driver-xhyve/blob/46a94c2450c532fa766698ae8906cdc69933134a/xhyve/xhyve.go#L933-L937 but that pull request seems to already fix it. OK, I'll also check that pull request together.

zchee commented 7 years ago

@dlorenc Ah, sorry, Are you said means had been changed boot2docker bootlocal.sh specification?

zchee commented 7 years ago

@dlorenc Anyway, this pull request is LGTM. The failed CI will fix. Merged.

zchee commented 7 years ago

@dlorenc Thanks!!

justechn commented 7 years ago

@zchee @dlorenc Thanks for getting this fixed so quickly. Can we get a new release so I can use this?

huguesalary commented 6 years ago

I just created a pull request that undoes all this work. Then I saw this issue.

Specifically this commit in the PR reverts part of the work that was done in this thread.

Here's the commit message:

Make sure both Virtio9p shared folders and NFS shared folders can be used at the same time.

In this commit I removed the d.setupMount() from the Start() function: Start() simply starts the docker-machine and isn't supposed to do any other work. For example, you can not add more CPUs, more RAM, or more Disks with the docker-machine start command. Just like CPU, RAM, etc, Shared Folders can not be added after the fact with docker-machine start and as such, the d.setupMounts() shouldn't run.

I, however, changed the Create() function so that it returns an error, and thus prevents the docker-machine from being created, when the d.setupMounts() fails.

The other changes address the bug where you couldn't have both Virtio9p and NFS shared folders. The bug was that both the code setting up Virtio9p shares and the NFS shares would overwrite the bootlocal.sh script.

As I explain in the commit message, it seems to me that Start() shouldn't call setupMounts(). Specifically, all setupMounts() does is fill the bootlocal.sh script with mount commands.

Once bootlocal.sh has been created (at the docker-machine create time), there's no need to ever re-create it (at docker-machine start time).