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

Parallel virtualbox #202

Merged
merged 2 commits into from
Jul 28, 2017
Merged

Parallel virtualbox #202

merged 2 commits into from
Jul 28, 2017

Conversation

rveznaver
Copy link

Hi,

This patch sets up environmental variables which isolate the VirtualBox environment (configuration file, machine location, and IPC socket) and allow parallel creation of VirtualBox instances with vagrant.

Comments are welcome!

@cheeseplus
Copy link
Contributor

I know there are some reasons why we didn't do this in the past but I'm happy to re-examine parallelizing but before accepting I'd really like some tests to be added to make sure there aren't any side effects.

@cheeseplus
Copy link
Contributor

Still on board with this if you can rebase and we can think of some tests for this functionality and maybe a disable/enable config toggle @rveznaver

@cheeseplus
Copy link
Contributor

I know this one has been here for a long while and the reason we've always been hesitant is that this feature doesn't feel desperately needed and it's subtle enough that it could have some unforeseen consequences/breaking existing behaviour. The other worries step from how this works on Windows and that it's only specific to Virtualbox. Also, with such a change we would want some thorough tests written as well.

Ideally this would be toggle-able behaviour (sequential staying the default) but I'm going to give this some testing across different platforms and if things don't entirely explode we can look at finally merging this one.

@cheeseplus
Copy link
Contributor

After some discussion one of the things that was raised, rather poignantly, is that Vagrant itself should be managing this and not kitchen-vagrant. That is to say this code is managing something (virtualbox) directly at a layer of abstraction below the thing (vagrant) that it is abstracting itself. So while it definitely solves the problem, we've broken the social contract of the abstraction.

@rveznaver
Copy link
Author

I completely agree that Vagrant itself should be managing this, but if I recall correctly, this would require a rewrite of the Vagrant VirtualBox provider (or something similar, I can't find the issue any more on Vagrant's GitHub). So... yeah, I have kind of chosen the lesser of two evils.

@cheeseplus
Copy link
Contributor

cheeseplus commented Jul 28, 2017

Seems like we're definitely on the same page with this one. That really only leaves two issues: Windows and lack of tests. I think we can fairly easily address both of those post-merge but obviously I'd not say so no to a follow up PR with tests.

@cheeseplus cheeseplus merged commit 8470c8c into test-kitchen:master Jul 28, 2017
@cloneluke
Copy link

does line 429 of this commit

a946fb6#diff-746c6817cd7ffe8b6bd9acc333fa5852R429

allow for spaces in path?

also struggling with this related issue:

#210

@cheeseplus
Copy link
Contributor

We've reverted this change and described why here: #325 - essentially the on-by default behaviour breaks both the GUI interaction and Windows users.

For future reference @cloneluke, commenting on merged/closed PRs trying to get help is an easy way to get missed. Unless it's closing a discussion loop, usually as a maintainer, you don't want to do so.

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

Successfully merging this pull request may close these issues.

3 participants