From 5c857f50f1a50475e6895f4d457f749636619cba Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Thu, 28 Jun 2018 09:44:28 -0700 Subject: [PATCH] Replace find_folder with searchindex inventorypath This commit replaces find_folder to use information already known about folders to determine their location and quickly fail when it does not exist without traversing the hierarchy. Without this change find_folder is very inneficient and can take a long time to return depending on how deep in the folder tree the folder exists. --- lib/vmpooler/providers/vsphere.rb | 36 ++++--- spec/unit/providers/vsphere_spec.rb | 162 +++++----------------------- 2 files changed, 47 insertions(+), 151 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index fd7db6e..dfe072a 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -47,7 +47,7 @@ module Vmpooler @connection_pool.with_metrics do |pool_object| connection = ensured_vsphere_connection(pool_object) foldername = pool_config(pool_name)['folder'] - folder_object = find_folder(foldername, connection, get_target_datacenter_from_config(pool_name)) + folder_object = find_folder(pool_name, connection) return vms if folder_object.nil? @@ -232,7 +232,7 @@ module Vmpooler ) begin - vm_target_folder = find_folder(target_folder_path, connection, target_datacenter_name) + vm_target_folder = find_folder(pool_name, connection) if vm_target_folder.nil? and @config[:config].key?('create_folders') and @config[:config]['create_folders'] == true vm_target_folder = create_folder(connection, target_folder_path, target_datacenter_name) end @@ -576,24 +576,28 @@ module Vmpooler available_unit_numbers.sort[0] end - # Finds the first reference to and returns the folder object for a foldername and an optional datacenter + # Finds a folder object by inventory path # Params: - # +foldername+:: the folder to find (optionally with / in which case the foldername will be split and each element searched for) + # +pool_name+:: the pool to find the folder for # +connection+:: the vsphere connection object - # +datacentername+:: the datacenter where the folder resides, or nil to return the first datacenter found - # returns a ManagedObjectReference for the first folder found or nil if none found - def find_folder(foldername, connection, datacentername) - datacenter = connection.serviceInstance.find_datacenter(datacentername) - raise("Datacenter #{datacentername} does not exist") if datacenter.nil? - base = datacenter.vmFolder + # returns a ManagedObjectReference for the folder found or nil if not found + def find_folder(pool_name, connection) + # Find a folder by its inventory path and return the object + # Returns nil when the object found is not a folder + 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? - folders = foldername.split('/') - folders.each do |folder| - raise("Unexpected object type encountered (#{base.class}) while finding folder") unless base.is_a? RbVmomi::VIM::Folder - base = base.childEntity.find { |f| f.name == folder } - end + propSpecs = { + :entity => self, + :inventoryPath => "#{datacenter}/vm/#{folder}" + } - base + folder_object = connection.searchIndex.FindByInventoryPath(propSpecs) + return nil unless folder_object.class == RbVmomi::VIM::Folder + folder_object end # Returns an array containing cumulative CPU and memory utilization of a host, and its object reference diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 7000e96..e1ac9cb 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -97,7 +97,7 @@ EOT context 'Given a pool folder that is missing' do before(:each) do - expect(subject).to receive(:find_folder).with(pool_config['folder'],connection,datacenter_name).and_return(nil) + expect(subject).to receive(:find_folder).with(poolname,connection).and_return(nil) end it 'should get a connection' do @@ -115,7 +115,7 @@ EOT context 'Given an empty pool folder' do before(:each) do - expect(subject).to receive(:find_folder).with(pool_config['folder'],connection,datacenter_name).and_return(folder_object) + expect(subject).to receive(:find_folder).with(poolname,connection).and_return(folder_object) end it 'should get a connection' do @@ -144,7 +144,7 @@ EOT folder_object.childEntity << mock_vm end - expect(subject).to receive(:find_folder).with(pool_config['folder'],connection,datacenter_name).and_return(folder_object) + expect(subject).to receive(:find_folder).with(poolname,connection).and_return(folder_object) end it 'should get a connection' do @@ -338,11 +338,13 @@ EOT end context 'Given a successful creation' do + let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => 'pool1'}) } before(:each) do template_vm = new_template_object allow(subject).to receive(:find_template_vm).and_return(new_template_object) allow(template_vm).to receive(:CloneVM_Task).and_return(clone_vm_task) allow(clone_vm_task).to receive(:wait_for_completion).and_return(new_vm_object) + allow(subject).to receive(:find_folder).and_return(folder_object) end it 'should return a hash' do @@ -1467,100 +1469,48 @@ EOT describe '#find_folder' do let(:foldername) { 'folder'} - let(:missing_foldername) { 'missing_folder'} context 'with no folder hierarchy' do - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name } - ] - } - }} it 'should return nil if the folder is not found' do - expect(subject.find_folder(missing_foldername,connection,datacenter_name)).to be_nil + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil) + expect(subject.find_folder(poolname,connection)).to be_nil end end context 'with a single layer folder hierarchy' do - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, - :vmfolder_tree => { - 'folder1' => nil, - 'folder2' => nil, - foldername => nil, - 'folder3' => nil, - } - } - ] - } - }} + let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => foldername}) } it 'should return the folder when found' do - result = subject.find_folder(foldername,connection,datacenter_name) - expect(result).to_not be_nil + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(folder_object) + allow(folder_object).to receive(:class).and_return(RbVmomi::VIM::Folder) + result = subject.find_folder(poolname,connection) expect(result.name).to eq(foldername) end it 'should return nil if the folder is not found' do - expect(subject.find_folder(missing_foldername,connection,datacenter_name)).to be_nil + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil) + expect(subject.find_folder(poolname,connection)).to be_nil end end context 'with a single layer folder hierarchy in many datacenters' do - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => 'AnotherDC', - :vmfolder_tree => { - 'folder1' => nil, - 'folder2' => nil, - 'folder3' => nil, - } - }, - { :name => datacenter_name, - :vmfolder_tree => { - 'folder4' => nil, - 'folder5' => nil, - foldername => nil, - 'folder6' => nil, - } - } - ] - } - }} + let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => foldername}) } it 'should return the folder when found' do - result = subject.find_folder(foldername,connection,datacenter_name) - expect(result).to_not be_nil + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(folder_object) + allow(folder_object).to receive(:class).and_return(RbVmomi::VIM::Folder) + result = subject.find_folder(poolname,connection) expect(result.name).to eq(foldername) end it 'should return nil if the folder is not found' do - expect(subject.find_folder(missing_foldername,connection,'AnotherDC')).to be_nil + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil) + expect(subject.find_folder(poolname,connection)).to be_nil end end context 'with a VM with the same name as a folder in a single layer folder hierarchy' do - # The folder hierarchy should include a VM with same name as folder, and appear BEFORE the - # folder in the child list. - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, - :vmfolder_tree => { - 'folder1' => nil, - 'vm1' => { :object_type => 'vm', :name => foldername }, - foldername => nil, - 'folder3' => nil, - } - } - ] - } - }} it 'should not return a VM' do pending('https://github.com/puppetlabs/vmpooler/issues/204') @@ -1572,77 +1522,19 @@ EOT end context 'with a multi layer folder hierarchy' do - let(:end_folder_name) { 'folder'} - let(:foldername) { 'folder2/folder4/' + end_folder_name} - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, - :vmfolder_tree => { - 'folder1' => nil, - 'folder2' => { - :children => { - 'folder3' => nil, - 'folder4' => { - :children => { - end_folder_name => nil, - }, - } - }, - }, - 'folder5' => nil, - } - } - ] - } - }} + let(:foldername) { 'folder2/folder4/folder' } + let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => foldername}) } it 'should return the folder when found' do - result = subject.find_folder(foldername,connection,datacenter_name) - expect(result).to_not be_nil - expect(result.name).to eq(end_folder_name) + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(folder_object) + allow(folder_object).to receive(:class).and_return(RbVmomi::VIM::Folder) + result = subject.find_folder(poolname,connection) + expect(result.name).to eq(foldername) end it 'should return nil if the folder is not found' do - expect(subject.find_folder(missing_foldername,connection,datacenter_name)).to be_nil - end - end - - context 'with a VM with the same name as a folder in a multi layer folder hierarchy' do - # The folder hierarchy should include a VM with same name as folder mid-hierarchy (i.e. not at the end level) - # and appear BEFORE the folder in the child list. - let(:end_folder_name) { 'folder'} - let(:foldername) { 'folder2/folder4/' + end_folder_name} - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, - :vmfolder_tree => { - 'folder1' => nil, - 'folder2' => { - :children => { - 'folder3' => nil, - 'vm1' => { :object_type => 'vm', :name => 'folder4' }, - 'folder4' => { - :children => { - end_folder_name => nil, - }, - } - }, - }, - 'folder5' => nil, - } - } - ] - } - }} - - it 'should not return a VM' do - pending('https://github.com/puppetlabs/vmpooler/issues/204') - result = subject.find_folder(foldername,connection,datacenter_name) - expect(result).to_not be_nil - expect(result.name).to eq(foldername) - expect(result.is_a? RbVmomi::VIM::VirtualMachine,datacenter_name).to be false + allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil) + expect(subject.find_folder(poolname,connection)).to be_nil end end end