vSphere usability improvements #592
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch changes how the `list_all_virtual_machine_mobs` method searches for virtual machines. Previously the search was based on children of the `dc.vmFolder` object. This makes the false assumption that all children are virtual machines. Now the search is based on the entire inventory of the `dc.vmFolder` object, which gives us more control over what we consider a virtual machine. This patch also adds a new `find_all_in_inventory` method; a generic method that can be used to search an inventory for any VMware object exposed by RbVmomi. Using `find_all_in_inventory` we are able to traverse VMware folders and pickout specific VMware object, in this case `RbVmomi::VIM::VirtualMachine`
This patch adds a new `path` attribute to the vm_mob_ref hash returned by the `convert_vm_mob_ref_to_attr_hash` method. In order to support the new `path` attribute, this patch also adds a new `get_folder_path` method to the `list_virtual_machines` vsphere module.
Without this patch, `Fog::Compute::Vsphere#convert_vm_mob_ref_to_attr_hash` method produces unhandled exceptions during VMware cloning and listing operations. The root cause of these exceptions are based on the fact that some VMware virtual machine attributes: hypervisor name, and macaddress, are not available until the cloning process has finished. These exceptions can be triggered when external events take place within the VMware infrastructure such as end-users cloning machines via some other VMware management tool. This patch solves the problem by catching any exceptions that occur during attribute lookups for both the hypervisor name and the virtual machine macaddress, and setting them to nil. This patch also removes the host attribute from the hash generated by the `Fog::Compute::Vsphere#convert_vm_mob_ref_to_attr_hash` method. The hypervisor attribute is added instead, which is an alias to host. This patch changes the behaviour of the `Fog::Compute::Vsphere#convert_vm_mob_ref_to_attr_hash` method by catching exceptions for missing attributes, and setting them to nil.
|
I'd merge this, but the spec tests need to be updated to pass after this change in behavior. Here's what I'm getting now: |
|
Let me take a look that, fix it, and add the additional commit to this pull request. |
|
@kelseyhightower Here's the full run of vsphere tests with FOG_MOCK turned on. |
|
I may need to mock fake_vm to bring it a little closer to actual vm_mob_ref, we are mainly not responding to the collect! method, so it seems that vm_mob_ref provides an Array like interface. |
Update `Fog::Compute::Vsphere` tests to reflect the way the `Fog::Compute::Vsphere#convert_vm_mob_ref_to_attr_hash` method currently converts a `RbVmomi::VIM::ManagedObject` object to a hash. Without this patch, tests related to converting an instance of `RbVmomi::VIM::ManagedObject` to a hash will raise an exception: NoMethodError: undefined method `collect!' for Hash; causing the test to fail. This patch solves the problem by mocking `RbVmomi::VIM::ManagedObject` with a new `MockManagedObject` class, which provides a `collect!` method, and the `_ref` and `parent` attributes. This patch renames fake_vm to fake_vm_mob_ref in order to provide a more descriptive name for what's actually being tested. The `Fog::Compute::Vshpere::Mock` class has been updated to require the rbvmomi library, which prevents an `NameError` exception from being raised due to the `Fog::Compute::Vsphere::Shared::RbVmomi` constant not being initialized. The `Fog::Compute::Vshpere::Mock` class has been updated with a `get_folder_path` method, which prevents a `NoMethodError` exception from being raised due to the `get_folder_path` method being undefined.
|
All vSphere tests are passing now \o/ |
|
Great, this looks good Kelsey. @geemus there's a change in behavior here that requires the rbvmomi gem to run the spec tests now. Previously, when using FOG_MOCK=true all of the mocked requests returned data structures that didn't require the rbvmomi gem to run. Are you OK with this change in behavior? -Jeff |
|
@jeffmccune - Yeah, I think it is ok to have more requirements for dev than release. I think there may be a couple other places where this already appears. It sounds like that is the best way to provide this kind of stuff, so go for it. |
|
Sweet! |
|
Cool, merging this request then as I don't have any other outstanding comments. This also brings Fog in line with Puppet Enterprise. @geemus, if you do a release this week we'll likely include that release in PE rather than v1.0.0. -Jeff |
jeffmccune
pushed a commit
that referenced
this pull request
Nov 8, 2011
…nil_for_missing_attributes vSphere usability improvements
|
Cool, I was hoping to get a release done this week anyway. So On Tue, Nov 8, 2011 at 15:57, Jeff McCune
|
|
Hey guys, I'm getting YAML parse errors when I try to run the mocked tests under 1.9.2. Here is a stack trace: I looked and it wasn't obvious to me what was wrong (and I didn't want to invalidate the tests by just changing something if it would then be less reflective of reality). It might be simpler to just remove the yaml and have the resulting ruby hash there directly (but this might also make it harder to keep up to date. I leave how to fix it in your hands, but if you could get it going for 1.9.2 that would be awesome, thanks! |
|
Taking a look at this now. |
|
So I have ran the vSphere tests in isolation on ruby 1.9.2p290 Can you provide the command you are running so I can attempt to reproduce this? |
|
I have also ran the tests following the instructions at fog.io: It seems I am getting another set of errors. |
|
Hmm. Now when I run them they succeed :( I must have either not been totally up to date or accidentally introduced something locally or something. Sorry about the confusion, but thanks for the quick response. I'll keep plugging away at the rest of the tests and release process. |
|
Cool, let us know. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
commit b53416a
Author: Carl Caum carl@carlcaum.com
Date: Fri Oct 28 09:03:02 2011 -0400
commit 762aa62
Author: Carl Caum carl@carlcaum.com
Date: Fri Oct 28 10:10:37 2011 -0400
commit bc74e06
Author: Kelsey Hightower kelsey@puppetlabs.com
Date: Mon Nov 7 07:08:54 2011 -0500
commit a65db7f
Author: Kelsey Hightower kelsey@puppetlabs.com
Date: Tue Nov 8 00:47:27 2011 -0500