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

@@ -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),
+            datacenter: datacenter,
-            name: "[#{vmdk_datastore.name}] #{vmdk_file_name}",
+            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(

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)