Skip to content

Commit

Permalink
(POOLER-68) Replace find_vm search mechanism
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mattkirby
mattkirby committed May 14, 2018
1 parent 1024532 commit 4700ad2
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 200 deletions.
89 changes: 29 additions & 60 deletions lib/vmpooler/providers/vsphere.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,11 @@ def vm_in_target?(pool_name, parent_host, architecture, target)
return false
end

def get_vm(_pool_name, vm_name)
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)
return vm_hash if vm_object.nil?

vm_folder_path = get_vm_folder_path(vm_object)
Expand Down Expand Up @@ -283,7 +283,7 @@ def create_disk(pool_name, vm_name, disk_size)

@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)
raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?

add_disk(vm_object, disk_size, datastore_name, connection, get_target_datacenter_from_config(pool_name))
Expand All @@ -294,7 +294,7 @@ def create_disk(pool_name, vm_name, disk_size)
def create_snapshot(pool_name, vm_name, new_snapshot_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)
raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?

old_snap = find_snapshot(vm_object, new_snapshot_name)
Expand All @@ -313,7 +313,7 @@ def create_snapshot(pool_name, vm_name, new_snapshot_name)
def revert_snapshot(pool_name, vm_name, snapshot_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)
raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?

snapshot_object = find_snapshot(vm_object, snapshot_name)
Expand All @@ -324,10 +324,10 @@ def revert_snapshot(pool_name, vm_name, snapshot_name)
true
end

def destroy_vm(_pool_name, vm_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)
# If a VM doesn't exist then it is effectively deleted
return true if vm_object.nil?

Expand Down Expand Up @@ -784,60 +784,29 @@ def find_snapshot(vm, snapshotname)
get_snapshot_list(vm.snapshot.rootSnapshotList, snapshotname) if vm.snapshot
end

def find_vm(vmname, connection)
find_vm_light(vmname, connection) || find_vm_heavy(vmname, connection)[vmname]
end

def find_vm_light(vmname, connection)
connection.searchIndex.FindByDnsName(vmSearch: true, dnsName: vmname)
def build_propSpecs(datacenter, folder, vmname)
propSpecs = {
entity => self,
:inventoryPath => "#{datacenter}/vm/#{folder}/#{vmname}"
}
propSpecs
end

def find_vm_heavy(vmname, connection)
vmname = vmname.is_a?(Array) ? vmname : [vmname]
container_view = get_base_vm_container_from(connection)
property_collector = connection.propertyCollector

object_set = [{
obj: container_view,
skip: true,
selectSet: [RbVmomi::VIM::TraversalSpec.new(
name: 'gettingTheVMs',
path: 'view',
skip: false,
type: 'ContainerView'
)]
}]

prop_set = [{
pathSet: ['name'],
type: 'VirtualMachine'
}]

results = property_collector.RetrievePropertiesEx(
specSet: [{
objectSet: object_set,
propSet: prop_set
}],
options: { maxObjects: nil }
)

vms = {}
results.objects.each do |result|
name = result.propSet.first.val
next unless vmname.include? name
vms[name] = result.obj
end
def find_vm(pool_name, vmname, connection)
# Find a VM by its inventory path and return the VM object
# Returns nil when a VM, or pool configuration, cannot be found
pool_configuration = pool_config(pool_name)
return nil if pool_configuration.nil?
folder = pool_configuration['folder']
datacenter = get_target_datacenter_from_config(pool_name)
return nil if datacenter.nil?

while results.token
results = property_collector.ContinueRetrievePropertiesEx(token: results.token)
results.objects.each do |result|
name = result.propSet.first.val
next unless vmname.include? name
vms[name] = result.obj
end
end
propSpecs = {
:entity => self,
:inventoryPath => "#{datacenter}/vm/#{folder}/#{vmname}"
}

vms
connection.searchIndex.FindByInventoryPath(propSpecs)
end

def find_vmdks(vmname, datastore, connection, datacentername)
Expand Down Expand Up @@ -880,8 +849,8 @@ def get_snapshot_list(tree, snapshotname)
snapshot
end

def get_vm_details(vm_name, connection)
vm_object = find_vm(vm_name, connection)
def get_vm_details(pool_name, vm_name, connection)
vm_object = find_vm(pool_name, vm_name, connection)
return nil if vm_object.nil?
parent_host_object = vm_object.summary.runtime.host if vm_object.summary && vm_object.summary.runtime && vm_object.summary.runtime.host
raise('Unable to determine which host the VM is running on') if parent_host_object.nil?
Expand All @@ -905,7 +874,7 @@ def migrate_vm(pool_name, vm_name)
@connection_pool.with_metrics do |pool_object|
begin
connection = ensured_vsphere_connection(pool_object)
vm_hash = get_vm_details(vm_name, connection)
vm_hash = get_vm_details(pool_name, vm_name, connection)
migration_limit = @config[:config]['migration_limit'] if @config[:config].key?('migration_limit')
migration_count = $redis.scard('vmpooler__migration')
if migration_enabled? @config
Expand Down
163 changes: 23 additions & 140 deletions spec/unit/providers/vsphere_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@
let(:vm_object) { nil }
before(:each) do
allow(subject).to receive(:connect_to_vsphere).and_return(connection)
expect(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object)
expect(subject).to receive(:find_vm).with(poolname,vmname,connection).and_return(vm_object)
end

context 'when VM does not exist' do
Expand Down Expand Up @@ -361,7 +361,7 @@
let(:disk_size) { 10 }
before(:each) do
allow(subject).to receive(:connect_to_vsphere).and_return(connection)
allow(subject).to receive(:find_vm).with(vmname, connection).and_return(vm_object)
allow(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(vm_object)
end

context 'Given an invalid pool name' do
Expand All @@ -382,7 +382,7 @@

context 'when VM does not exist' do
before(:each) do
expect(subject).to receive(:find_vm).with(vmname, connection).and_return(nil)
expect(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(nil)
end

it 'should raise an error' do
Expand Down Expand Up @@ -419,12 +419,12 @@

before(:each) do
allow(subject).to receive(:connect_to_vsphere).and_return(connection)
allow(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object)
allow(subject).to receive(:find_vm).with(poolname, vmname,connection).and_return(vm_object)
end

context 'when VM does not exist' do
before(:each) do
expect(subject).to receive(:find_vm).with(vmname, connection).and_return(nil)
expect(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(nil)
end

it 'should raise an error' do
Expand Down Expand Up @@ -474,12 +474,12 @@

before(:each) do
allow(subject).to receive(:connect_to_vsphere).and_return(connection)
allow(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object)
allow(subject).to receive(:find_vm).with(poolname,vmname,connection).and_return(vm_object)
end

context 'when VM does not exist' do
before(:each) do
expect(subject).to receive(:find_vm).with(vmname,connection).and_return(nil)
expect(subject).to receive(:find_vm).with(poolname,vmname,connection).and_return(nil)
end

it 'should raise an error' do
Expand Down Expand Up @@ -2303,14 +2303,14 @@
end

it 'returns nil when vm_object is not found' do
expect(subject).to receive(:find_vm).with(vmname, connection).and_return(nil)
result = subject.get_vm_details(vmname, connection)
expect(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(nil)
result = subject.get_vm_details(poolname, vmname, connection)
expect(result).to be nil
end

it 'raises an error when unable to determine parent_host' do
expect(subject).to receive(:find_vm).with(vmname, connection).and_return(vm_object)
expect{subject.get_vm_details(vmname, connection)}.to raise_error('Unable to determine which host the VM is running on')
expect(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(vm_object)
expect{subject.get_vm_details(poolname, vmname, connection)}.to raise_error('Unable to determine which host the VM is running on')
end

context 'when it can find parent host' do
Expand All @@ -2320,8 +2320,8 @@
end

it 'returns vm details hash' do
expect(subject).to receive(:find_vm).with(vmname, connection).and_return(vm_object)
result = subject.get_vm_details(vmname, connection)
expect(subject).to receive(:find_vm).with(poolname, vmname, connection).and_return(vm_object)
result = subject.get_vm_details(poolname, vmname, connection)
expect(result).to eq(vm_details)
end
end
Expand Down Expand Up @@ -3291,147 +3291,30 @@
end

describe '#find_vm' do
before(:each) do
allow(subject).to receive(:find_vm_light).and_return('vmlight')
allow(subject).to receive(:find_vm_heavy).and_return( { vmname => 'vmheavy' })
end

it 'should call find_vm_light' do
expect(subject).to receive(:find_vm_light).and_return('vmlight')

expect(subject.find_vm(vmname,connection)).to eq('vmlight')
end

it 'should not call find_vm_heavy if find_vm_light finds the VM' do
expect(subject).to receive(:find_vm_light).and_return('vmlight')
expect(subject).to receive(:find_vm_heavy).exactly(0).times

expect(subject.find_vm(vmname,connection)).to eq('vmlight')
end

it 'should call find_vm_heavy when find_vm_light returns nil' do
expect(subject).to receive(:find_vm_light).and_return(nil)
expect(subject).to receive(:find_vm_heavy).and_return( { vmname => 'vmheavy' })

expect(subject.find_vm(vmname,connection)).to eq('vmheavy')
end
end

describe '#find_vm_light' do
let(:missing_vm) { 'missing_vm' }
let(:folder) { 'Pooler/pool1' }
let(:vm_object) { mock_RbVmomi_VIM_VirtualMachine() }

before(:each) do
allow(connection.searchIndex).to receive(:FindByDnsName).and_return(nil)
allow(connection.searchIndex).to receive(:FindByInventoryPath)
end

it 'should call FindByDnsName with the correct parameters' do
expect(connection.searchIndex).to receive(:FindByDnsName).with({
:vmSearch => true,
dnsName: vmname,
})
it 'should call FindByInventoryPath with the correct parameters' do
expect(connection.searchIndex).to receive(:FindByInventoryPath)

subject.find_vm_light(vmname,connection)
subject.find_vm(poolname,vmname,connection)
end

it 'should return the VM object when found' do
vm_object = mock_RbVmomi_VIM_VirtualMachine()
expect(connection.searchIndex).to receive(:FindByDnsName).with({
:vmSearch => true,
dnsName: vmname,
}).and_return(vm_object)
expect(connection.searchIndex).to receive(:FindByInventoryPath).and_return(vm_object)

expect(subject.find_vm_light(vmname,connection)).to be(vm_object)
expect(subject.find_vm(poolname,vmname,connection)).to be(vm_object)
end

it 'should return nil if the VM is not found' do
expect(connection.searchIndex).to receive(:FindByDnsName).with({
:vmSearch => true,
dnsName: missing_vm,
}).and_return(nil)

expect(subject.find_vm_light(missing_vm,connection)).to be_nil
end
end

describe '#find_vm_heavy' do
let(:missing_vm) { 'missing_vm' }
# Return an empty result by default
let(:retrieve_result) {{}}

before(:each) do
allow(connection.propertyCollector).to receive(:RetrievePropertiesEx).and_return(mock_RbVmomi_VIM_RetrieveResult(retrieve_result))
end

context 'Search result is empty' do
it 'should return empty hash' do
expect(subject.find_vm_heavy(vmname,connection)).to eq({})
end
end

context 'Search result contains VMs but no matches' do
let(:retrieve_result) {
{ :response => [
{ 'name' => 'no_match001'},
{ 'name' => 'no_match002'},
{ 'name' => 'no_match003'},
{ 'name' => 'no_match004'},
]
}
}

it 'should return empty hash' do
expect(subject.find_vm_heavy(vmname,connection)).to eq({})
end
end

context 'Search contains a single match' do
let(:vm_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname })}
let(:retrieve_result) {
{ :response => [
{ 'name' => 'no_match001'},
{ 'name' => 'no_match002'},
{ 'name' => vmname, :object => vm_object },
{ 'name' => 'no_match003'},
{ 'name' => 'no_match004'},
]
}
}
expect(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil)

it 'should return single result' do
result = subject.find_vm_heavy(vmname,connection)
expect(result.keys.count).to eq(1)
end

it 'should return the matching VM Object' do
result = subject.find_vm_heavy(vmname,connection)
expect(result[vmname]).to be(vm_object)
end
end

context 'Search contains a two matches' do
let(:vm_object1) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname })}
let(:vm_object2) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname })}
let(:retrieve_result) {
{ :response => [
{ 'name' => 'no_match001'},
{ 'name' => 'no_match002'},
{ 'name' => vmname, :object => vm_object1 },
{ 'name' => 'no_match003'},
{ 'name' => 'no_match004'},
{ 'name' => vmname, :object => vm_object2 },
]
}
}

it 'should return one result' do
result = subject.find_vm_heavy(vmname,connection)
expect(result.keys.count).to eq(1)
end

it 'should return the last matching VM Object' do
result = subject.find_vm_heavy(vmname,connection)
expect(result[vmname]).to be(vm_object2)
end
expect(subject.find_vm(poolname,missing_vm,connection)).to be_nil
end
end

Expand Down

0 comments on commit 4700ad2

Please sign in to comment.