(POOLER-68) Replace find_vm search mechanism #251
Conversation
lib/vmpooler/providers/vsphere.rb
Outdated
| end | ||
| end | ||
| def find_vm(pool_name, vmname, connection) | ||
| pool_configuration = pool_config(pool_name) |
There was a problem hiding this comment.
this method can return nil so we should protect against that and maybe log an error message if it happens
There was a problem hiding this comment.
The way it's implemented currently the different methods calling find_vm respond to the vm object being nil differently. We could change it to return an error if the VM is not found, but I think it would require a more significant change to the methods trying to find VMs.
There was a problem hiding this comment.
I think it's more about pool_configuration being nil and the next line pool_configuration['folder'] will raise a method error.
- Should either raise an error it pool_configuration is nil
raise("could not find pool_name") if pool_configuration.nil?
or
- Return a nil value
return nil if pool_configuration.nil?
There was a problem hiding this comment.
Should probably document the expected return values in code comments for future-us
There was a problem hiding this comment.
Actually given;
expect(subject.find_vm(poolname,missing_vm,connection)).to be_nil in the spec tests, it should return nil
There was a problem hiding this comment.
Ah ok, I follow now. I'll update this and push a change.
lib/vmpooler/providers/vsphere.rb
Outdated
| def find_vm(pool_name, vmname, connection) | ||
| pool_configuration = pool_config(pool_name) | ||
| folder = pool_configuration['folder'] | ||
| datacenter = get_target_datacenter_from_config(pool_name) |
There was a problem hiding this comment.
same comment here the method can return nil so we should protect against that and return an error message if it happens.
Additionally, we could have find_vm return nil, similarily to when connection.searchIndex.FindByInventoryPath(propSpecs) reutrns nil, but I haven't checked how we handle that upstream
There was a problem hiding this comment.
I'll add something to ensure we return nil if we cannot determine the datacenter.
lib/vmpooler/providers/vsphere.rb
Outdated
| @@ -165,7 +165,7 @@ def get_vm(_pool_name, vm_name) | |||
| vm_hash = nil | |||
| @connection_pool.with_metrics do |pool_object| | |||
| connection = ensured_vsphere_connection(pool_object) | |||
| vm_object = find_vm(vm_name, connection) | |||
| vm_object = find_vm(_pool_name, vm_name, connection) | |||
There was a problem hiding this comment.
The ruby convention of underscore prefix is denote variables which aren't used. As this variable is now being used, the underscore should be removed.
There was a problem hiding this comment.
Thanks, I'll fix this and push a change.
lib/vmpooler/providers/vsphere.rb
Outdated
| @@ -327,7 +327,7 @@ def revert_snapshot(pool_name, vm_name, snapshot_name) | |||
| def destroy_vm(_pool_name, vm_name) | |||
| @connection_pool.with_metrics do |pool_object| | |||
| connection = ensured_vsphere_connection(pool_object) | |||
| vm_object = find_vm(vm_name, connection) | |||
| vm_object = find_vm(_pool_name, vm_name, connection) | |||
There was a problem hiding this comment.
See underscore comment above
lib/vmpooler/providers/vsphere.rb
Outdated
| end | ||
| end | ||
| def find_vm(pool_name, vmname, connection) | ||
| pool_configuration = pool_config(pool_name) |
There was a problem hiding this comment.
I think it's more about pool_configuration being nil and the next line pool_configuration['folder'] will raise a method error.
- Should either raise an error it pool_configuration is nil
raise("could not find pool_name") if pool_configuration.nil?
or
- Return a nil value
return nil if pool_configuration.nil?
lib/vmpooler/providers/vsphere.rb
Outdated
| end | ||
| end | ||
| def find_vm(pool_name, vmname, connection) | ||
| pool_configuration = pool_config(pool_name) |
There was a problem hiding this comment.
Should probably document the expected return values in code comments for future-us
lib/vmpooler/providers/vsphere.rb
Outdated
| end | ||
| end | ||
| def find_vm(pool_name, vmname, connection) | ||
| pool_configuration = pool_config(pool_name) |
There was a problem hiding this comment.
Actually given;
expect(subject.find_vm(poolname,missing_vm,connection)).to be_nil in the spec tests, it should return nil
|
Thanks for the review @glennsarti and @sbeaulie I think I've made appropriate adjustments here. Let me know if you think this requires further changes. |
This commit replaces find_vm and find_vm_heavy with a more performant and reliable mechanism of identifying VM objects. Specifically, FindByInventoryPath is able to leverage known data about a VM, its folder path and datacenter, and use that to identify whether that VM exists by its location. Without this change find_vm_heavy is called each time a VM cannot be found, which is frequent, and in doing so uses PropertyCollector in a manner that is not thread-safe. Additionally, this PropertyCollector usage does not clean up its traces, which can cause vCenter appliance instability issues on VCSA 6.x.
This commit replaces find_vm and find_vm_heavy with a more performant and reliable mechanism of identifying VM objects. Specifically, FindByInventoryPath is able to leverage known data about a VM, its folder path and datacenter, and use that to identify whether that VM exists by its location. Without this change find_vm_heavy is called each time a VM cannot be found, which is frequent, and in doing so uses PropertyCollector in a manner that is not thread-safe. Additionally, this PropertyCollector usage does not clean up its traces, which can cause vCenter appliance instability issues on VCSA 6.x.