(POOLER-70) Refactor clone_vm to take pool configuration object

Previously, the clone_vm method took various VSphere specific parameters e.g.
template folder.  However in order make VMPooler less VSphere specific this
method should just take the pool configuration and then it can determine the
appropriate settings itself.  This commit also moves the threading to a clone_vm
while the actual method which does the work is now _clone_vm as per all other
multithread worker methods in pool_manager.  This commit also updates the spec
tests appropriately.

lib/vmpooler/pool_manager.rb

@@ -185,105 +185,113 @@ module Vmpooler
     end
 
     # Clone a VM
-    def clone_vm(template, folder, datastore, target, vsphere)
+    def clone_vm(pool, vsphere)
       Thread.new do
         begin
-          vm = {}
+          _clone_vm(pool, vsphere)
-
-          if template =~ /\//
-            templatefolders = template.split('/')
-            vm['template'] = templatefolders.pop
-          end
-
-          if templatefolders
-            vm[vm['template']] = vsphere.find_folder(templatefolders.join('/')).find(vm['template'])
-          else
-            fail 'Please provide a full path to the template'
-          end
-
-          if vm['template'].length == 0
-            fail "Unable to find template '#{vm['template']}'!"
-          end
-
-          # Generate a randomized hostname
-          o = [('a'..'z'), ('0'..'9')].map(&:to_a).flatten
-          vm['hostname'] = $config[:config]['prefix'] + o[rand(25)] + (0...14).map { o[rand(o.length)] }.join
-
-          # Add VM to Redis inventory ('pending' pool)
-          $redis.sadd('vmpooler__pending__' + vm['template'], vm['hostname'])
-          $redis.hset('vmpooler__vm__' + vm['hostname'], 'clone', Time.now)
-          $redis.hset('vmpooler__vm__' + vm['hostname'], 'template', vm['template'])
-
-          # Annotate with creation time, origin template, etc.
-          # Add extraconfig options that can be queried by vmtools
-          configSpec = RbVmomi::VIM.VirtualMachineConfigSpec(
-            annotation: JSON.pretty_generate(
-                name: vm['hostname'],
-                created_by: $config[:vsphere]['username'],
-                base_template: vm['template'],
-                creation_timestamp: Time.now.utc
-            ),
-            extraConfig: [
-                { key: 'guestinfo.hostname',
-                  value: vm['hostname']
-                }
-            ]
-          )
-
-          # Choose a clone target
-          if target
-            $clone_target = vsphere.find_least_used_host(target)
-          elsif $config[:config]['clone_target']
-            $clone_target = vsphere.find_least_used_host($config[:config]['clone_target'])
-          end
-
-          # Put the VM in the specified folder and resource pool
-          relocateSpec = RbVmomi::VIM.VirtualMachineRelocateSpec(
-            datastore: vsphere.find_datastore(datastore),
-            host: $clone_target,
-            diskMoveType: :moveChildMostDiskBacking
-          )
-
-          # Create a clone spec
-          spec = RbVmomi::VIM.VirtualMachineCloneSpec(
-            location: relocateSpec,
-            config: configSpec,
-            powerOn: true,
-            template: false
-          )
-
-          # Clone the VM
-          $logger.log('d', "[ ] [#{vm['template']}] '#{vm['hostname']}' is being cloned from '#{vm['template']}'")
-
-          begin
-            start = Time.now
-            vm[vm['template']].CloneVM_Task(
-              folder: vsphere.find_folder(folder),
-              name: vm['hostname'],
-              spec: spec
-            ).wait_for_completion
-            finish = '%.2f' % (Time.now - start)
-
-            $redis.hset('vmpooler__clone__' + Date.today.to_s, vm['template'] + ':' + vm['hostname'], finish)
-            $redis.hset('vmpooler__vm__' + vm['hostname'], 'clone_time', finish)
-
-            $logger.log('s', "[+] [#{vm['template']}] '#{vm['hostname']}' cloned from '#{vm['template']}' in #{finish} seconds")
-          rescue => err
-            $logger.log('s', "[!] [#{vm['template']}] '#{vm['hostname']}' clone failed with an error: #{err}")
-            $redis.srem('vmpooler__pending__' + vm['template'], vm['hostname'])
-            raise
-          end
-
-          $redis.decr('vmpooler__tasks__clone')
-
-          $metrics.timing("clone.#{vm['template']}", finish)
         rescue => err
-          $logger.log('s', "[!] [#{vm['template']}] '#{vm['hostname']}' failed while preparing to clone with an error: #{err}")
+          $logger.log('s', "[!] [#{pool['name']}] failed while cloning VM with an error: #{err}")
           raise
         end
       end
     end
 
+    def _clone_vm(pool, vsphere)
+      template = pool['template']
+      folder = pool['folder']
+      datastore = pool['datastore']
+      target = pool['clone_target']
+      vm = {}
+
+      if template =~ /\//
+        templatefolders = template.split('/')
+        vm['template'] = templatefolders.pop
+      end
+
+      if templatefolders
+        vm[vm['template']] = vsphere.find_folder(templatefolders.join('/')).find(vm['template'])
+      else
+        fail 'Please provide a full path to the template'
+      end
+
+      if vm['template'].length == 0
+        fail "Unable to find template '#{vm['template']}'!"
+      end
+
+      # Generate a randomized hostname
+      o = [('a'..'z'), ('0'..'9')].map(&:to_a).flatten
+      vm['hostname'] = $config[:config]['prefix'] + o[rand(25)] + (0...14).map { o[rand(o.length)] }.join
+
+      # Add VM to Redis inventory ('pending' pool)
+      $redis.sadd('vmpooler__pending__' + vm['template'], vm['hostname'])
+      $redis.hset('vmpooler__vm__' + vm['hostname'], 'clone', Time.now)
+      $redis.hset('vmpooler__vm__' + vm['hostname'], 'template', vm['template'])
+
+      # Annotate with creation time, origin template, etc.
+      # Add extraconfig options that can be queried by vmtools
+      configSpec = RbVmomi::VIM.VirtualMachineConfigSpec(
+        annotation: JSON.pretty_generate(
+            name: vm['hostname'],
+            created_by: $config[:vsphere]['username'],
+            base_template: vm['template'],
+            creation_timestamp: Time.now.utc
+        ),
+        extraConfig: [
+            { key: 'guestinfo.hostname',
+              value: vm['hostname']
+            }
+        ]
+      )
+
+      # Choose a clone target
+      if target
+        $clone_target = vsphere.find_least_used_host(target)
+      elsif $config[:config]['clone_target']
+        $clone_target = vsphere.find_least_used_host($config[:config]['clone_target'])
+      end
+
+      # Put the VM in the specified folder and resource pool
+      relocateSpec = RbVmomi::VIM.VirtualMachineRelocateSpec(
+        datastore: vsphere.find_datastore(datastore),
+        host: $clone_target,
+        diskMoveType: :moveChildMostDiskBacking
+      )
+
+      # Create a clone spec
+      spec = RbVmomi::VIM.VirtualMachineCloneSpec(
+        location: relocateSpec,
+        config: configSpec,
+        powerOn: true,
+        template: false
+      )
+
+      # Clone the VM
+      $logger.log('d', "[ ] [#{vm['template']}] '#{vm['hostname']}' is being cloned from '#{vm['template']}'")
+
+      begin
+        start = Time.now
+        vm[vm['template']].CloneVM_Task(
+          folder: vsphere.find_folder(folder),
+          name: vm['hostname'],
+          spec: spec
+        ).wait_for_completion
+        finish = '%.2f' % (Time.now - start)
+
+        $redis.hset('vmpooler__clone__' + Date.today.to_s, vm['template'] + ':' + vm['hostname'], finish)
+        $redis.hset('vmpooler__vm__' + vm['hostname'], 'clone_time', finish)
+
+        $logger.log('s', "[+] [#{vm['template']}] '#{vm['hostname']}' cloned from '#{vm['template']}' in #{finish} seconds")
+      rescue => err
+        $logger.log('s', "[!] [#{vm['template']}] '#{vm['hostname']}' clone failed with an error: #{err}")
+        $redis.srem('vmpooler__pending__' + vm['template'], vm['hostname'])
+        raise
+      end
+
+      $redis.decr('vmpooler__tasks__clone')
+
+      $metrics.timing("clone.#{vm['template']}", finish)
+    end
+
     # Destroy a VM
     def destroy_vm(vm, pool, vsphere)
       Thread.new do
@@ -710,14 +718,7 @@ module Vmpooler
           if $redis.get('vmpooler__tasks__clone').to_i < $config[:config]['task_limit'].to_i
             begin
               $redis.incr('vmpooler__tasks__clone')
-
+              clone_vm(pool,vsphere)
-              clone_vm(
-                pool['template'],
-                pool['folder'],
-                pool['datastore'],
-                pool['clone_target'],
-                vsphere
-              )
             rescue => err
               $logger.log('s', "[!] [#{pool['name']}] clone failed during check_pool with an error: #{err}")
               $redis.decr('vmpooler__tasks__clone')

spec/unit/pool_manager_spec.rb

@@ -505,13 +505,7 @@ EOT
   end
 
   describe '#clone_vm' do
-    before do
+    let(:vsphere) { double('vsphere') }
-      expect(subject).not_to be_nil
-    end
-
-    before(:each) do
-      expect(Thread).to receive(:new).and_yield
-    end
 
     let(:config) {
       YAML.load(<<-EOT
@@ -520,9 +514,41 @@ EOT
   prefix: "prefix"
 :vsphere:
   username: "vcenter_user"
+:pools:
+  - name: #{pool}
 EOT
       )
     }
+    let (:pool_object) { config[:pools][0] }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _clone_vm' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_clone_vm).with(pool_object,vsphere)
+
+      subject.clone_vm(pool_object,vsphere)
+    end
+
+    it 'logs a message if an error is raised' do
+      expect(Thread).to receive(:new).and_yield
+      expect(logger).to receive(:log)
+      expect(subject).to receive(:_clone_vm).with(pool_object,vsphere).and_raise('an_error')
+
+      expect{subject.clone_vm(pool_object,vsphere)}.to raise_error(/an_error/)
+    end
+  end
+
+  describe '#_clone_vm' do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      #expect(Thread).to receive(:new).and_yield
+    end
 
     let (:folder) { 'vmfolder' }
     let (:folder_object) { double('folder_object') }
@@ -530,34 +556,47 @@ EOT
     let (:template) { "template/#{template_name}" }
     let (:datastore) { 'datastore' }
     let (:target) { 'clone_target' }
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:config:
+  prefix: "prefix"
+:vsphere:
+  username: "vcenter_user"
+:pools:
+  - name: #{pool}
+    template: '#{template}'
+    folder: '#{folder}'
+    datastore: '#{datastore}'
+    clone_target: '#{target}'
+EOT
+      )
+    }
+
     let (:vsphere) { double('vsphere') }
     let (:template_folder_object) { double('template_folder_object') }
     let (:template_vm_object) { double('template_vm_object') }
     let (:clone_task) { double('clone_task') }
+    let (:pool_object) { config[:pools][0] }
 
     context 'no template specified' do
-      it 'should raise an error' do
+      before(:each) do
-  
+        pool_object['template'] = nil
-        expect{subject.clone_vm(nil,folder,datastore,target,vsphere)}.to raise_error(/Please provide a full path to the template/)
       end
 
-      it 'should log a message' do
+      it 'should raise an error' do
-        expect(logger).to receive(:log).with('s', "[!] [] '' failed while preparing to clone with an error: Please provide a full path to the template")
+        expect{subject._clone_vm(pool_object,vsphere)}.to raise_error(/Please provide a full path to the template/)
-
-        expect{subject.clone_vm(nil,folder,datastore,target,vsphere)}.to raise_error(RuntimeError)
       end
     end
 
     context 'a template with no forward slash in the string' do
-      it 'should raise an error' do
+      before(:each) do
-  
+        pool_object['template'] = template_name
-        expect{subject.clone_vm('vm1',folder,datastore,target,vsphere)}.to raise_error(/Please provide a full path to the template/)
       end
 
-      it 'should log a message' do
+      it 'should raise an error' do
-        expect(logger).to receive(:log).with('s', "[!] [] '' failed while preparing to clone with an error: Please provide a full path to the template")
+        expect{subject._clone_vm(pool_object,vsphere)}.to raise_error(/Please provide a full path to the template/)
-
-        expect{subject.clone_vm('vm1',folder,datastore,target,vsphere)}.to raise_error(RuntimeError)
       end
     end
 
@@ -571,9 +610,9 @@ EOT
     context "Template name does not match pool name (Implementation Bug)" do
       let (:template_name) { 'template_vm' }
 
-      # The implementaion of clone_vm incorrectly uses the VM Template name instead of the pool name.  The VM Template represents the
+      # The implementaion of _clone_vm incorrectly uses the VM Template name instead of the pool name.  The VM Template represents the
       # name of the VM to clone in vSphere whereas pool is the name of the pool in Pooler.  The tests below document the behaviour of
-      # clone_vm if the Template and Pool name differ.  It is expected that these test will fail once this bug is removed.
+      # _clone_vm if the Template and Pool name differ.  It is expected that these test will fail once this bug is removed.
 
       context 'a valid template' do
         before(:each) do
@@ -595,7 +634,7 @@ EOT
             expect(logger).to receive(:log).at_least(:once)
             expect(redis.scard("vmpooler__pending__#{pool}")).to eq(0)
 
-            subject.clone_vm(template,folder,datastore,target,vsphere)
+            subject._clone_vm(pool_object,vsphere)
 
             expect(redis.scard("vmpooler__pending__#{template_name}")).to eq(1)
             # Get the new VM Name from the pending pool queue as it should be the only entry
@@ -610,14 +649,14 @@ EOT
             expect(logger).to receive(:log).with('d',/\[ \] \[#{template_name}\] '(.+)' is being cloned from '#{template_name}'/)
             allow(logger).to receive(:log)
 
-            subject.clone_vm(template,folder,datastore,target,vsphere)
+            subject._clone_vm(pool_object,vsphere)
           end
 
           it 'should log a message that it completed being cloned' do
             expect(logger).to receive(:log).with('s',/\[\+\] \[#{template_name}\] '(.+)' cloned from '#{template_name}' in [0-9.]+ seconds/)
             allow(logger).to receive(:log)
 
-            subject.clone_vm(template,folder,datastore,target,vsphere)
+            subject._clone_vm(pool_object,vsphere)
           end
         end
 
@@ -639,7 +678,7 @@ EOT
 
           it 'should raise an error within the Thread' do
             expect(logger).to receive(:log).at_least(:once)
-            expect{subject.clone_vm(template,folder,datastore,target,vsphere)}.to raise_error(/SomeError/)
+            expect{subject._clone_vm(pool_object,vsphere)}.to raise_error(/SomeError/)
           end
 
           it 'should log a message that is being cloned from a template' do
@@ -648,19 +687,18 @@ EOT
 
             # Swallow the error
             begin
-              subject.clone_vm(template,folder,datastore,target,vsphere)
+              subject._clone_vm(pool_object,vsphere)
             rescue
             end
           end
 
           it 'should log messages that the clone failed' do
             expect(logger).to receive(:log).with('s', /\[!\] \[#{template_name}\] '(.+)' clone failed with an error: SomeError/)
-            expect(logger).to receive(:log).with('s', /\[!\] \[#{template_name}\] '(.+)' failed while preparing to clone with an error: SomeError/)
             allow(logger).to receive(:log)
 
             # Swallow the error
             begin
-              subject.clone_vm(template,folder,datastore,target,vsphere)
+              subject._clone_vm(pool_object,vsphere)
             rescue
             end
           end
@@ -688,7 +726,7 @@ EOT
           expect(logger).to receive(:log).at_least(:once)
           expect(redis.scard("vmpooler__pending__#{pool}")).to eq(0)
 
-          subject.clone_vm(template,folder,datastore,target,vsphere)
+          subject._clone_vm(pool_object,vsphere)
 
           expect(redis.scard("vmpooler__pending__#{pool}")).to eq(1)
           # Get the new VM Name from the pending pool queue as it should be the only entry
@@ -703,7 +741,7 @@ EOT
           redis.incr('vmpooler__tasks__clone')
           redis.incr('vmpooler__tasks__clone')
           expect(redis.get('vmpooler__tasks__clone')).to eq('2')
-          subject.clone_vm(template,folder,datastore,target,vsphere)
+          subject._clone_vm(pool_object,vsphere)
           expect(redis.get('vmpooler__tasks__clone')).to eq('1')
         end
 
@@ -711,14 +749,14 @@ EOT
           expect(logger).to receive(:log).with('d',/\[ \] \[#{pool}\] '(.+)' is being cloned from '#{template_name}'/)
           allow(logger).to receive(:log)
 
-          subject.clone_vm(template,folder,datastore,target,vsphere)
+          subject._clone_vm(pool_object,vsphere)
         end
 
         it 'should log a message that it completed being cloned' do
           expect(logger).to receive(:log).with('s',/\[\+\] \[#{pool}\] '(.+)' cloned from '#{template_name}' in [0-9.]+ seconds/)
           allow(logger).to receive(:log)
 
-          subject.clone_vm(template,folder,datastore,target,vsphere)
+          subject._clone_vm(pool_object,vsphere)
         end
       end
 
@@ -740,7 +778,7 @@ EOT
 
         it 'should raise an error within the Thread' do
           expect(logger).to receive(:log).at_least(:once)
-          expect{subject.clone_vm(template,folder,datastore,target,vsphere)}.to raise_error(/SomeError/)
+          expect{subject._clone_vm(pool_object,vsphere)}.to raise_error(/SomeError/)
         end
 
         it 'should log a message that is being cloned from a template' do
@@ -749,19 +787,18 @@ EOT
 
           # Swallow the error
           begin
-            subject.clone_vm(template,folder,datastore,target,vsphere)
+            subject._clone_vm(pool_object,vsphere)
           rescue
           end
         end
 
         it 'should log messages that the clone failed' do
           expect(logger).to receive(:log).with('s', /\[!\] \[#{pool}\] '(.+)' clone failed with an error: SomeError/)
-          expect(logger).to receive(:log).with('s', /\[!\] \[#{pool}\] '(.+)' failed while preparing to clone with an error: SomeError/)
           allow(logger).to receive(:log)
 
           # Swallow the error
           begin
-            subject.clone_vm(template,folder,datastore,target,vsphere)
+            subject._clone_vm(pool_object,vsphere)
           rescue
           end
         end