From 8840ef4a5ca9dc50eca154c390a353000854e742 Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Wed, 27 Jun 2018 17:03:37 -0700
Subject: [PATCH 1/4] Reduce object lookups for get_vm

This commit reduces object lookups used for get_vm. Without this change get_vm looks up the VM folder, which is already used and known to find the vm with find_vm.
---
 lib/vmpooler/providers/vsphere.rb   | 38 +++++++++++++----------------
 spec/unit/providers/vsphere_spec.rb |  2 +-
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb
index 5d241c6..fd7db6e 100644
--- a/lib/vmpooler/providers/vsphere.rb
+++ b/lib/vmpooler/providers/vsphere.rb
@@ -168,18 +168,7 @@ module Vmpooler
             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)
-            # Find the pool name based on the folder path
-            pool_name = nil
-            template_name = nil
-            global_config[:pools].each do |pool|
-              if pool['folder'] == vm_folder_path
-                pool_name = pool['name']
-                template_name = pool['template']
-              end
-            end
-
-            vm_hash = generate_vm_hash(vm_object, template_name, pool_name)
+            vm_hash = generate_vm_hash(vm_object, pool_name)
           end
           vm_hash
         end
@@ -262,7 +251,7 @@ module Vmpooler
               spec: clone_spec
             ).wait_for_completion
 
-            vm_hash = generate_vm_hash(new_vm_object, template_path, pool_name)
+            vm_hash = generate_vm_hash(new_vm_object, pool_name)
           end
           vm_hash
         end
@@ -365,15 +354,22 @@ module Vmpooler
           nil
         end
 
-        def generate_vm_hash(vm_object, template_name, pool_name)
-          hash = { 'name' => nil, 'hostname' => nil, 'template' => nil, 'poolname' => nil, 'boottime' => nil, 'powerstate' => nil }
+        def generate_vm_hash(vm_object, pool_name)
+          pool_configuration = pool_config(pool_name)
+          return nil if pool_configuration.nil?
 
-          hash['name']     = vm_object.name
-          hash['hostname'] = vm_object.summary.guest.hostName if vm_object.summary && vm_object.summary.guest && vm_object.summary.guest.hostName
-          hash['template'] = template_name
-          hash['poolname'] = pool_name
-          hash['boottime']   = vm_object.runtime.bootTime if vm_object.runtime && vm_object.runtime.bootTime
-          hash['powerstate'] = vm_object.runtime.powerState if vm_object.runtime && vm_object.runtime.powerState
+          hostname = vm_object.summary.guest.hostName if vm_object.summary && vm_object.summary.guest && vm_object.summary.guest.hostName
+          boottime = vm_object.runtime.bootTime if vm_object.runtime && vm_object.runtime.bootTime
+          powerstate = vm_object.runtime.powerState if vm_object.runtime && vm_object.runtime.powerState
+
+          hash = {
+            'name' => vm_object.name,
+            'hostname' => hostname,
+            'template' => pool_configuration['template'],
+            'poolname' => pool_name,
+            'boottime' => boottime,
+            'powerstate' => powerstate,
+          }
 
           hash
         end
diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb
index df618d3..7000e96 100644
--- a/spec/unit/providers/vsphere_spec.rb
+++ b/spec/unit/providers/vsphere_spec.rb
@@ -190,7 +190,7 @@ EOT
         expect(result['name']).to eq(vmname)
       end
 
-      ['hostname','template','poolname','boottime','powerstate'].each do |testcase|
+      ['hostname','boottime','powerstate'].each do |testcase|
         it "should return nil for #{testcase}" do
           result = subject.get_vm(poolname,vmname)
 

From 5c857f50f1a50475e6895f4d457f749636619cba Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Thu, 28 Jun 2018 09:44:28 -0700
Subject: [PATCH 2/4] 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

From a6c2ef7bf37de6bdbe0039725610046e5334cfcf Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Thu, 28 Jun 2018 09:47:19 -0700
Subject: [PATCH 3/4] Rename find_folder to find_vm_folder

This commit renames find_folder to find_vm_folder to clarify its intent to retrieve folders from the VM hierarchy. Without this change find_folder implies it may find folders that are not within the VM hierarchy.
---
 lib/vmpooler/providers/vsphere.rb   |  7 +++----
 spec/unit/providers/vsphere_spec.rb | 26 +++++++++++++-------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb
index dfe072a..ad1e9cf 100644
--- a/lib/vmpooler/providers/vsphere.rb
+++ b/lib/vmpooler/providers/vsphere.rb
@@ -46,8 +46,7 @@ module Vmpooler
           vms = []
           @connection_pool.with_metrics do |pool_object|
             connection = ensured_vsphere_connection(pool_object)
-            foldername = pool_config(pool_name)['folder']
-            folder_object = find_folder(pool_name, connection)
+            folder_object = find_vm_folder(pool_name, connection)
 
             return vms if folder_object.nil?
 
@@ -232,7 +231,7 @@ module Vmpooler
             )
 
             begin
-              vm_target_folder = find_folder(pool_name, connection)
+              vm_target_folder = find_vm_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
@@ -581,7 +580,7 @@ module Vmpooler
         # +pool_name+:: the pool to find the folder for
         # +connection+:: the vsphere connection object
         # returns a ManagedObjectReference for the folder found or nil if not found
-        def find_folder(pool_name, connection)
+        def find_vm_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)
diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb
index e1ac9cb..24c1e87 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(poolname,connection).and_return(nil)
+        expect(subject).to receive(:find_vm_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(poolname,connection).and_return(folder_object)
+        expect(subject).to receive(:find_vm_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(poolname,connection).and_return(folder_object)
+        expect(subject).to receive(:find_vm_folder).with(poolname,connection).and_return(folder_object)
       end
 
       it 'should get a connection' do
@@ -344,7 +344,7 @@ EOT
         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)
+        allow(subject).to receive(:find_vm_folder).and_return(folder_object)
       end
 
       it 'should return a hash' do
@@ -1467,14 +1467,14 @@ EOT
     end
   end
 
-  describe '#find_folder' do
+  describe '#find_vm_folder' do
     let(:foldername) { 'folder'}
 
     context 'with no folder hierarchy' do
 
       it 'should return nil if the folder is not found' do
         allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil)
-        expect(subject.find_folder(poolname,connection)).to be_nil
+        expect(subject.find_vm_folder(poolname,connection)).to be_nil
       end
     end
 
@@ -1484,13 +1484,13 @@ EOT
       it 'should return the folder when found' do
         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)
+        result = subject.find_vm_folder(poolname,connection)
         expect(result.name).to eq(foldername)
       end
 
       it 'should return nil if the folder is not found' do
         allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil)
-        expect(subject.find_folder(poolname,connection)).to be_nil
+        expect(subject.find_vm_folder(poolname,connection)).to be_nil
       end
     end
 
@@ -1500,13 +1500,13 @@ EOT
       it 'should return the folder when found' do
         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)
+        result = subject.find_vm_folder(poolname,connection)
         expect(result.name).to eq(foldername)
       end
 
       it 'should return nil if the folder is not found' do
         allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil)
-        expect(subject.find_folder(poolname,connection)).to be_nil
+        expect(subject.find_vm_folder(poolname,connection)).to be_nil
       end
     end
 
@@ -1514,7 +1514,7 @@ EOT
 
       it 'should not return a VM' do
         pending('https://github.com/puppetlabs/vmpooler/issues/204')
-        result = subject.find_folder(foldername,connection,datacenter_name)
+        result = subject.find_vm_folder(foldername,connection,datacenter_name)
         expect(result).to_not be_nil
         expect(result.name).to eq(foldername)
         expect(result.is_a? RbVmomi::VIM::VirtualMachine).to be false
@@ -1528,13 +1528,13 @@ EOT
       it 'should return the folder when found' do
         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)
+        result = subject.find_vm_folder(poolname,connection)
         expect(result.name).to eq(foldername)
       end
 
       it 'should return nil if the folder is not found' do
         allow(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil)
-        expect(subject.find_folder(poolname,connection)).to be_nil
+        expect(subject.find_vm_folder(poolname,connection)).to be_nil
       end
     end
   end

From 170d2090f80dc239048f41564d07f9523831d1bd Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Tue, 3 Jul 2018 09:12:05 -0700
Subject: [PATCH 4/4] Add some documentation to get_vm_hash

---
 lib/vmpooler/providers/vsphere.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb
index ad1e9cf..62848aa 100644
--- a/lib/vmpooler/providers/vsphere.rb
+++ b/lib/vmpooler/providers/vsphere.rb
@@ -353,6 +353,8 @@ module Vmpooler
           nil
         end
 
+        # Return a hash of VM data
+        # Provides vmname, hostname, template, poolname, boottime and powerstate information
         def generate_vm_hash(vm_object, pool_name)
           pool_configuration = pool_config(pool_name)
           return nil if pool_configuration.nil?