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

NFS and Virtio9p mounts setup refactoring #195

Merged
merged 4 commits into from
Aug 17, 2017

Conversation

huguesalary
Copy link
Contributor

I have been working those past few days on xhyve's ability to mount multiple folders from the Host to the Guest, either via NFS or via Virtio9p.

At this point, the NFS work has already been merged into master: #121

Since this last PR (#121), I have been working on allowing multiple mount point using Virtio9p, and have submitted a PR currently waiting for approval: #194. Until that point, xhyve's was "hardcodingly" mounting the Host /Users folder into the Guest via Virtio9p, and the user had no choice in the matter.

At the same time, I have been working on modifying minikube (https://github.com/kubernetes/minikube) to allow minikube users to create minikube machines with multiple mounts (either NFS or Virtio9p).

However, while working on minikube I ran into issues that ultimately were caused by xhyve's use of the bootlocal.sh script for setting up NFS and Virtio9p mount points: when I would create a new minikube machine, my mount points would not show up at creation time. They would only show up after a minikube stop followed by a minikube start.

After debugging I realized that the bootlocal.sh script wasn't being executed when the minikube machine was created the very first time. After looking around, I found this boot2docker FAQ explaining the role of the bootlocal.sh script, which made me realize that xhyve's use of this script was 1- not proper, 2- superfluous.

This Pull Request addresses this issue and will also allow me to submit a PR to the minikube project allowing minikube users to make use of multiple, user-defined, NFS and/or Virtio9p shared folders.

Using --xhyve-experimental-nfs-shares to set up NFS shares implies --xhyve-experimental-nfs-share-enable, which now becomes redundant.

Removing --xhyve-experimental-nfs-share-enable makes the command line easier to use.
Previously, only "/Users" was being shared inside the guest at "/Users".

With this commit, it is possible to mount any Host folder inside the Guest at "/xhyve-virtio9p". This default can be changed via the `--xhyve-virtio-9p-root` command line argument.

This commit changes `--xhyve-virtio-9p` from a bool flag to a string slice representing the absolute path to the Host folder to be mounted inside the guest.

There is currently, however, a major caveat, that is that using Virtio9p shares and NFS Shares at the same time will not work. After restarting the machine, the NFS mounts will disappear from the Guest. This issue will be addressed in the next commit.
…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.
Instead, simply prepare and execute the `mount` commands directly with `driver.RunSSHCommandFromDriver()` when `Start()` is called.

When reading https://github.com/boot2docker/boot2docker/blob/master/doc/FAQ.md#local-customisation-with-persistent-partition it appears that the intent of the `bootlocal.sh` script is to be a script created and managed by the user.

Which means that this driver, by overwriting the `bootlocal.sh` script, is going against what has been defined as the way of using this file.

This commit fixes this issue.
@zchee
Copy link
Member

zchee commented Aug 10, 2017

@huguesalary Thanks pull request :)
I'll check tonight.

@huguesalary
Copy link
Contributor Author

huguesalary commented Aug 10, 2017

Great, thanks! Let me know if you need some more clarification.

@zchee zchee merged commit a2060c0 into machine-drivers:master Aug 17, 2017
@zchee
Copy link
Member

zchee commented Aug 17, 2017

@huguesalary merged. Thanks!

@huguesalary
Copy link
Contributor Author

Awesome, thanks!

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.

2 participants