From d1ae85c8af3df2ad397f929b2fed40bea3a974f4 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Tue, 29 May 2018 14:53:09 -0700 Subject: [PATCH] Remove propertyCollector from add_disk This commit updates add_disk to remove propertyCollector, which was used to back the find_vmdks method to locate the disk file on datastore and then use its length to name the new disk. Instead, the number of disks on the VM is used to ensure a unique disk resource title. Without this change add_disk can take 10-50x longer due to the propertyCollector method. Additionally, without this change propertyCollector is used in a non threadsafe manner, which may cause stability issues for vsphere provider backends. --- lib/vmpooler/providers/vsphere.rb | 30 +++++------------ spec/unit/providers/vsphere_spec.rb | 51 ----------------------------- 2 files changed, 8 insertions(+), 73 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index cfd087c..d6a429f 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -465,9 +465,12 @@ module Vmpooler vmdk_datastore = find_datastore(datastore, connection, datacentername) raise("Datastore '#{datastore}' does not exist in datacenter '#{datacentername}'") if vmdk_datastore.nil? - vmdk_file_name = "#{vm['name']}/#{vm['name']}_#{find_vmdks(vm['name'], datastore, connection, datacentername).length + 1}.vmdk" + datacenter = connection.serviceInstance.find_datacenter(datacentername) controller = find_disk_controller(vm) + disk_unit_number = find_disk_unit_number(vm, controller) + disk_count = vm.config.hardware.device.grep(RbVmomi::VIM::VirtualDisk).count + vmdk_file_name = "#{vm['name']}/#{vm['name']}_#{disk_count}.vmdk" vmdk_spec = RbVmomi::VIM::FileBackedVirtualDiskSpec( capacityKb: size.to_i * 1024 * 1024, @@ -478,7 +481,7 @@ module Vmpooler vmdk_backing = RbVmomi::VIM::VirtualDiskFlatVer2BackingInfo( datastore: vmdk_datastore, diskMode: DISK_MODE, - fileName: "[#{vmdk_datastore.name}] #{vmdk_file_name}" + fileName: "[#{datastore}] #{vmdk_file_name}" ) device = RbVmomi::VIM::VirtualDisk( @@ -486,7 +489,7 @@ module Vmpooler capacityInKB: size.to_i * 1024 * 1024, controllerKey: controller.key, key: -1, - unitNumber: find_disk_unit_number(vm, controller) + unitNumber: disk_unit_number ) device_config_spec = RbVmomi::VIM::VirtualDeviceConfigSpec( @@ -499,8 +502,8 @@ module Vmpooler ) connection.serviceContent.virtualDiskManager.CreateVirtualDisk_Task( - datacenter: connection.serviceInstance.find_datacenter(datacentername), - name: "[#{vmdk_datastore.name}] #{vmdk_file_name}", + datacenter: datacenter, + name: "[#{datastore}] #{vmdk_file_name}", spec: vmdk_spec ).wait_for_completion @@ -840,23 +843,6 @@ module Vmpooler vms end - def find_vmdks(vmname, datastore, connection, datacentername) - disks = [] - - vmdk_datastore = find_datastore(datastore, connection, datacentername) - - vm_files = connection.serviceContent.propertyCollector.collectMultiple vmdk_datastore.vm, 'layoutEx.file' - vm_files.keys.each do |f| - vm_files[f]['layoutEx.file'].each do |l| - if l.name =~ /^\[#{vmdk_datastore.name}\] #{vmname}\/#{vmname}_([0-9]+).vmdk/ - disks.push(l) - end - end - end - - disks - end - def get_base_vm_container_from(connection) view_manager = connection.serviceContent.viewManager view_manager.CreateContainerView( diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 39c891e..0cf3035 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -3435,57 +3435,6 @@ EOT end end - describe '#find_vmdks' do - let(:datastorename) { 'datastore' } - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, :datastores => [datastorename] } - ] - } - }} - - let(:collectMultiple_response) { {} } - - before(:each) do - allow(connection.serviceContent.propertyCollector).to receive(:collectMultiple).and_return(collectMultiple_response) - end - - context 'Searching all files for all VMs on a Datastore' do - # This is fairly fragile mocking - let(:collectMultiple_response) { { - 'FakeVMObject1' => { 'layoutEx.file' => - [ - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 101, :name => "[#{datastorename}] mock1/mock1_0.vmdk"}) - ]}, - vmname => { 'layoutEx.file' => - [ - # VMDKs which should match - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 1, :name => "[#{datastorename}] #{vmname}/#{vmname}_0.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 2, :name => "[#{datastorename}] #{vmname}/#{vmname}_1.vmdk"}), - # VMDKs which should not match - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 102, :name => "[otherdatastore] #{vmname}/#{vmname}_0.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 103, :name => "[otherdatastore] #{vmname}/#{vmname}.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 104, :name => "[otherdatastore] #{vmname}/#{vmname}_abc.vmdk"}), - ]}, - } } - - it 'should return empty array if no VMDKs match the VM name' do - expect(subject.find_vmdks('missing_vm_name',datastorename,connection,datacenter_name)).to eq([]) - end - - it 'should return matching VMDKs for the VM' do - result = subject.find_vmdks(vmname,datastorename,connection,datacenter_name) - expect(result).to_not be_nil - expect(result.count).to eq(2) - # The keys for each VMDK should be less that 100 as per the mocks - result.each do |fileinfo| - expect(fileinfo.key).to be < 100 - end - end - end - end - describe '#get_base_vm_container_from' do it 'should return a recursive view of type VirtualMachine' do result = subject.get_base_vm_container_from(connection)