Add support for linked clones for VirtualBox. #4484
Conversation
|
👍 |
|
This works great for me except after every clone it tries to re-import the master. I believe this may be resolved by returning after determining that the master has already been imported: env[:master_id] = master_id_file.read.chomp if master_id_file.file?
if env[:master_id] && env[:machine].provider.driver.vm_exists?(env[:master_id])
# Master VM already exists -> nothing to do - continue.
return @app.call(env)
end |
| env[:master_id] = master_id_file.read.chomp if master_id_file.file? | ||
| if env[:master_id] && env[:machine].provider.driver.vm_exists?(env[:master_id]) | ||
| # Master VM already exists -> nothing to do - continue. | ||
| @app.call(env) |
There was a problem hiding this comment.
Appears to need a return here to avoid falling through.
|
This is a great idea, we do something similar by default in vagrant-libvirt. Regarding
my suggestion is that for each box you destroy the master VM, if it exists, when |
|
Thanks for the feedback and corrections!
Thats exactly what I thought, but that means I would have to track which VMs are linked to the box/master VM - something that is not done by vagrant at the moment (at least to my knowledge). |
That depends on how Virtualbox handles a request to delete a VM that has others VMs linked to it. If Virtualbox throws an error and prevents you from doing that, vagrant shouldn't have to track the links itself. |
|
That would be a possibility... I'll try to implement this when I have the time. Unfortunately I'm quite busy with other stuff right now. |
|
|
||
| env[:ui].info I18n.t("vagrant.actions.vm.clone.importing", name: env[:machine].box.name) | ||
|
|
||
| #TODO - prevent concurrent creation of master vms for the same box. |
There was a problem hiding this comment.
I think we will want to handle this before we can merge. The easiest solution (although not the most performant) is to lock everything up to line 16 in the global lock. That way a master is created one at a time.
There was a problem hiding this comment.
Another alternative is to print a warning message and we can fix it up in another PR
|
Hi @mpoeter Thank you for the Pull Request. Overall I think this is a step in the right direction, especially for an initial pass. I made a few comments, but I would be in favor of merging this after those comments are fixed up and then we can address individual issues as subsequent PRs. I know you said your time is a bit limited, so I think that's a good compromise as well for you 😄. If you wouldn't mind taking a look at those comments and let me know what you think. |
|
Hi, thanks for the feedback! |
Deleting the lock file can fail when another process is currently trying to acquire it (-> race condition). It is safe to ignore this error since the other process will eventually acquire the lock and again try to delete the lock file.
|
Hi, I (hopefully) fixed all the open issues. What's still missing is removing the master VM when the box is deleted, but we can address this in a subsequent PR. Cheers, |
|
@mpoeter awesome, thank you! Since this is a pretty exciting new feature, would you like to write the documentation for it? All the docs live in the website/ folder of this repository 😄. If not, I can write them, just let me know, since I know time is limited. |
|
Sure, I think I can spare a few minutes in the next days... |
|
I have just pushed my changes - maybe you can take a look at them. |
| <div class="alert alert-info"> | ||
| <strong>Note:</strong> the generated master VMs are currently not removed | ||
| automatically by Vagrant. This has to be done manually. However, a master | ||
| VM can only be remove when there are no linked clones connected to it. |
There was a problem hiding this comment.
Should be "can only be removed", typo?
|
Works admirably well with VirtualBox 5 and a couple of Windows boxes. The first time I ran I worked around it by creating the directory |
|
I'd like to add that I did not get the aforementioned error if |
|
I might have an idea what's causing this. Could you provide a full stack trace? |
|
The (private) box is seen by And here is the full log: |
|
Ah, got another one with I see the pattern here but I know nothing about Vagrant's internals to be of any help. |
|
The problem is the slash in the machine name. I use the machine name as part of the filename for the lock file. With the machine name containing a slash this results in a path with an additional directory that does not exist and therefore creation of the lock file fails. |
… characters for file names.
|
Dang, for some reason I don't get an email when this PR is updated. I'll test it again as soon as I come back home. |
|
@mpoeter Thanks for this PR! This really blows me away. Now Windows VM's make much more fun as they are much heavier in disk size than Linux VM's. How does this PR work with |
|
@mpoeter Looks like the issue with the lock file is gone 👍 |
|
Merged :) |
|
Awsome! :-) |
This allow creation of linked clones using the VirtualBox 4.3 provider:
Instead of creating a fresh machine via importing the ovf file a master VM is created once (via ovf import) and the clones are created based on this master VM. The id of the master VM is stored in a file "master_id" in the directory of the according box.
There are a few bits missing like preventing concurrent creation of master VMs for the same box, as well as maintaining the lifetime of the master VM (when shall it be destroyed and who is responsible for it?).
Also the code might need a few adaptions since this is the first time I wrote some Ruby.
However, I thought this might be a good starting point for a discussion about how this feature could be added to Vagrant.