-
Notifications
You must be signed in to change notification settings - Fork 166
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 provisioning #210
base: master
Are you sure you want to change the base?
Conversation
@taliesins I merged your PR #204 (by way of #212 where I fixed tests, etc), but that has caused the |
0037407
to
06def27
Compare
@michael-brandt-cu I have resolved the conflicts. It would be wonderful if you could help me with the tests. Would love to get this into the main branch. I have added a couple more things into this pull request:
|
lib/vSphere/provider.rb
Outdated
# If the machine ID changed, then we need to rebuild our underlying | ||
# driver. | ||
def machine_id_changed | ||
id = @machine.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taliesins I'm working through the rubocop failures right now, and for this line it says:
lib/vSphere/provider.rb:28:9: W: Useless assignment to variable - id.
id = @machine.id
^^
id
is not used anywhere in this function. Was this supposed to be @id = @machine.id
, setting the id
property on an instance of Provider
? If not, then the line should be deleted to pass rubocop.
lib/vSphere/driver.rb
Outdated
elsif (x = p.find(final, RbVmomi::VIM::ClusterComputeResource)) | ||
x | ||
end | ||
rescue Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubocop for this line says:
lib/vSphere/driver.rb:500:9: W: Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?
rescue Exception
^^^^^^^^^^^^^^^^
We could replace Exception
with StandardError
like rubocop suggests, but I think it would be even better to use the specific exception or exceptions that we anticipate finding here, eg:
rescue TypeError, NameError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taliesins Is there a small number of specific exceptions we expect to encounter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cut and paste this from: https://github.com/nsidc/vagrant-vsphere/blob/master/lib/vSphere/util/vim_helpers.rb
I will see if I can work out why this code was implemented this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. It's too bad that it's difficult to tell in git when code is just moved around like that.
Looks like it passed rubocop before because vim_helpers.rb
was specifically excluded in the rubocop configuration: https://github.com/nsidc/vagrant-vsphere/blob/master/.rubocop.yml#L7
So, no need to worry about this then! I'll just update the config to point to name the correct file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I think we do want to run rubocop on this file. Lets see if we can put in something that it will pass with.
This file basically contains all the logic in it :>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that the whole file wasn't actually excluded, just the Lint/RescueException
cop. So it would still run most of the checks.
Great!
I've created a new branch, I have a 3-day weekend this weekend, but I'll be able to get back to this next week. |
I took a break from fixing unit tests, and I tried using this branch to destroy and recreate a VM I use for development on my current main project at NSIDC. I got these errors when attempting to destroy it:
We use both of these settings in our own infrastructure, and it looks like this branch removes It might be possible to release these changes in their current form as |
@michael-brandt-cu real_nic_ip, vlan are configuration settings that used to be at machine level. To support multiple nics we need to move them to network adapter level. Perhaps we could put in a shim property for vlan and ip_address? It should be trivial to change your vagrant scripts to use the multiple network card approach:
Here is an example snippet from my vagrant file for setting up Juniper FCP for vMX:
|
…ion is now no longer exposed to actions. A driver is created per a machine, and it manages the connection itself, so it is now safe between multiple machine deployments. Removed calling @machine.action, so we don't create a lock on the actions anymore, as they are now safe to call in parallel. Moved static strings into locales/en.yml
Update documentation for multiple network cards
Show debug output for network card configuration
…pens treat it as if there are no changes required for network card.
Allow ip family to detect on management nic to be specified Allow an ip address to be specified for a nic, so that auto detection of ip does not take place (use in situations where you cant install vmware guest tools) Use default nic Vsphere nic detection if we not using a customized approach Use standard variable naming convention
…n can occur on other operating systems. You can choose to wait for customization to complete, as not everyone uses VSphere customizations.
Modifies many files except for driver.rb; rubocop seems to stall out when auto-correcting driver.rb
1861e4e
to
0ce7927
Compare
…inning up machines in parallel we know which machine the ui output is for
Hi everyone, looks like there is lot of potential in this pull request. @michael-brandt-cu: Is backwards compatibility your main concern? Maybe we can split the pull request in multiple different ones so that risk and merge effort is reduced? |
Yes.
Maybe. It's been a while since I've had funding to do much work on vagrant-vsphere, and even longer since I really looked at this PR, so I can't really say how well it could be split up. |
With these changes I have managed to spin up 10 Windows boxes in parallel. I also make use of a few vagrant plugins like: vagrant-berkshelf, vagrant-triggers and vagrant-windows-domain. So there is lots of crazy things happening like reboots and re-initialization of file shares.
This PR is going to need some help on the test side. I am a bit of a Ruby noob, so I am not sure how we should be changing the tests to suite the code changes. So the tests have been left unchanged. Please send me a PR to fix the tests!