Ensure VirtualBox virtual disks are stored in the correct directory on Windows hosts #4133

Merged
merged 3 commits into from Jul 7, 2014

Projects

None yet

4 participants

@keiths-osc
Contributor

This change is a fix for #3584.

@sneal sneal commented on an outdated diff Jul 1, 2014
plugins/providers/virtualbox/driver/version_4_2.rb
@@ -185,7 +185,11 @@ def import(ovf)
disk_params << "--unit"
disk_params << unit_num
disk_params << "--disk"
- disk_params << path.sub("/#{suggested_name}/", "/#{specified_name}/")
+ if Gem.win_platform?
@sneal
sneal Jul 1, 2014 Collaborator

I think this should be Vagrant::Util::Platform.windows?

@sneal
Collaborator
sneal commented Jul 1, 2014

Does the sub call need to include the path separator?

@keiths-osc
Contributor

@sneal, thanks for your response. Before I update the platform check, I want to address your question about the path separators and make sure you agree.

I think the path separators are required. Here are the relevant lines from an example of the output from vboxmanage import -n:

...omitted some lines...

 1: Suggested VM name "centos-6.4"
    (change with "--vsys 0 --vmname <name>")

...omitted some lines...

 9: Hard disk image: source image=box-disk1.vmdk, target path=D:\VirtualBox\VMs\centos-6.4\box-disk1.vmdk, controller=8;channel=0
    (change target path with "--vsys 0 --unit 9 --disk path";
    disable with "--vsys 0 --unit 9 --ignore")

In this example, the machine's original name at the time it was exported was centos-6.4, and that name is used as the subdirectory that the VM and its disks will be place into inside of your VirtualBox base VM directory (unless you change them with the appropriate flags to the import command, which is what this vagrant code is trying to do). In this case, the Vagrant code would attempt to sub out the \centos-6.4\ substring within the D:\VirtualBox\VMs\centos-6.4\box-disk1.vmdk string with whatever name the VM is actually being given. If the path separators aren't included and one of the directories prior to the VM directly level happens to contain the VM name, then the sub would sub that out instead. Of course, if instead of my base VM directory being D:\VirtualBox\VMs\ it had instead been D:\centos-6.4\VMs so that the full path to the vmdk would have been D:\centos-6.4\VMs\centos-6.4\box-disk1.vmdk, then the sub would actually replace the base path, which is not the intention. I believe this pull request attempts to address that particular case.

Since it appears that the correct fix is actually a windows platform check combined with the code from this pull request, what would be your preference on how I proceed?

@sneal
Collaborator
sneal commented Jul 1, 2014

I was doubting anyone would run into the box name and parent dir name matching problem, but obviously someone did. I say merge them.

@keiths-osc
Contributor

I merged in the changes from #4092 (since #4124 was a duplicate of that) and then hit the sync button in my github app a little bit before I was ready. There is one more commit coming soon (which will include changing the platform check). I'll post again once I'm finished.

@keiths-osc
Contributor

@sneal I've made all the changes you asked for. This is ready unless you have any other concerns.

@sneal
Collaborator
sneal commented Jul 2, 2014

LGTM. I'd like to extract that code into another class or method to reduce duplication but more importantly ensure there's some unit tests around it. I can take that on unless you'd like to do it.

@keiths-osc
Contributor

@sneal I'd be glad to let you take on extracting the duplicated code into a common place

@sneal
Collaborator
sneal commented Jul 7, 2014

@keiths-osc That would be great!

@keiths-osc
Contributor

@sneal I think we may have had a bit of a miscommunication. I meant to convey that I would take you up on your offer to do the refactoring yourself instead of me doing it. It's not that I am unwilling, but you offered, and I figured since you are probably way more familiar with Vagrant and its tests than I am that you'd probably come up with an acceptable solution faster than I would. :)

My hope was that my pull request could be accepted to go ahead and resolve this issue for myself and the other users that are running into it, and perhaps some later pass could take care of removing the duplication and enhancing the tests (especially since my pull request is small, and the code was already duplicated to begin with).

There appears to be a base class for the VirtualBox driver along with version-specific subclasses. There are currently version specific sub classes for 4.0, 4.1, 4.2, and 4.3. There are only minor differences between 4.0 and 4.1. There are also only minor differences between 4.2 and 4.3. My first guess is that the majority of the differences exist because certain fixes have been made only for 4.3 when they would also have applied to 4.2 (and the same for 4.1 vs 4.0). One idea that comes to mind since the code is nearly identical is to combine the 4.0 and 4.1 subclasses into a single class, combine the 4.2 and 4.3 subclasses into another single class, and then add version-specific "if" checks around the small areas of code that are truly different (if any). That of course would be a fairly big change that I'd hate to take on as part of a small bug fix.

@sneal
Collaborator
sneal commented Jul 7, 2014

OK, gotcha - and thanks! We'll get this in soon.

@sneal
Collaborator
sneal commented Jul 7, 2014

I verified (manually) the non-Windows code path on OS X still works correctly, merging.

@sneal sneal merged commit bfe50e2 into mitchellh:master Jul 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@keiths-osc
Contributor

Thanks!

@keiths-osc keiths-osc deleted the unknown repository branch Jul 8, 2014
@keiths-osc keiths-osc restored the unknown repository branch Jul 8, 2014
@charliepank

This bug still exists in vagrant version: 1.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment