(POOLER-70) Update destroy_vm for VM Provider

Previously the Pool Manager would use vSphere objects directly.  This commit
- Modifies the pool_manager to use the VM provider methods instead
- Splits the destroy_vm function into two.  One function spawns the thread
  while the other actually does the work.  This makes testing much easier.

lib/vmpooler/pool_manager.rb

@@ -229,6 +229,16 @@ module Vmpooler
     # Destroy a VM
     def destroy_vm(vm, pool, provider)
       Thread.new do
+        begin
+          _destroy_vm(vm, pool, provider)
+        rescue => err
+          $logger.log('d', "[!] [#{pool}] '#{vm}' failed while destroying the VM with an error: #{err}")
+          raise
+        end
+      end
+    end
+
+    def _destroy_vm(vm, pool, provider)
       $redis.srem('vmpooler__completed__' + pool, vm)
       $redis.hdel('vmpooler__active__' + pool, vm)
       $redis.hset('vmpooler__vm__' + vm, 'destroy', Time.now)
@@ -236,28 +246,14 @@ module Vmpooler
       # Auto-expire metadata key
       $redis.expire('vmpooler__vm__' + vm, ($config[:redis]['data_ttl'].to_i * 60 * 60))
 
-        host = provider.find_vm(vm)
-
-        if host
       start = Time.now
 
-          if
+      provider.destroy_vm(pool, vm)
-            (host.runtime) &&
-            (host.runtime.powerState) &&
-            (host.runtime.powerState == 'poweredOn')
 
-            $logger.log('d', "[ ] [#{pool}] '#{vm}' is being shut down")
-            host.PowerOffVM_Task.wait_for_completion
-          end
-
-          host.Destroy_Task.wait_for_completion
       finish = '%.2f' % (Time.now - start)
-
       $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
       $metrics.timing("destroy.#{pool}", finish)
     end
-      end
-    end
 
     def create_vm_disk(vm, disk_size, provider)
       Thread.new do

spec/unit/pool_manager_spec.rb

@@ -617,140 +617,105 @@ EOT
     end
   end
 
-  describe "#destroy_vm" do
+  describe '#destroy_vm' do
-    let (:provider) { double('provider') }
-
-    let(:config) {
-      YAML.load(<<-EOT
----
-:redis:
-  data_ttl: 168
-EOT
-      )
-    }
-
     before do
       expect(subject).not_to be_nil
+      expect(Thread).to receive(:new).and_yield
     end
 
+    it 'calls _destroy_vm' do
+      expect(subject).to receive(:_destroy_vm).with(vm,pool,provider)
+
+      subject.destroy_vm(vm,pool,provider)
+    end
+
+    it 'logs a message if an error is raised' do
+      allow(logger).to receive(:log)
+      expect(logger).to receive(:log).with('d',"[!] [#{pool}] '#{vm}' failed while destroying the VM with an error: MockError")
+      expect(subject).to receive(:_destroy_vm).with(vm,pool,provider).and_raise('MockError')
+
+      expect{subject.destroy_vm(vm,pool,provider)}.to raise_error(/MockError/)
+    end
+  end
+
+  describe "#_destroy_vm" do
     before(:each) do
-      expect(Thread).to receive(:new).and_yield
+      expect(subject).not_to be_nil
 
       create_completed_vm(vm,pool,true)
+
+      allow(provider).to receive(:destroy_vm).with(pool,vm).and_return(true)
+
+      # Set redis configuration
+      config[:redis] = {}
+      config[:redis]['data_ttl'] = 168
     end
 
     context 'when redis data_ttl is not specified in the configuration' do
-      let(:config) {
-        YAML.load(<<-EOT
----
-:redis:
-  "key": "value"
-EOT
-      )
-      }
-
       before(:each) do
-        expect(provider).to receive(:find_vm).and_return(nil)
+        config[:redis]['data_ttl'] = nil
       end
 
       it 'should call redis expire with 0' do
         expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to_not be_nil
-        subject.destroy_vm(vm,pool,provider)
+        subject._destroy_vm(vm,pool,provider)
         expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to be_nil
       end
     end
 
     context 'when there is no redis section in the configuration' do
-      let(:config) {}
+      before(:each) do
+        config[:redis] = nil
+      end
       
       it 'should raise an error' do
-        expect{ subject.destroy_vm(vm,pool,provider) }.to raise_error(NoMethodError)
+        expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(NoMethodError)
       end
     end
 
     context 'when a VM does not exist' do
       before(:each) do
-        expect(provider).to receive(:find_vm).and_return(nil)
+        # As per base_spec, destroy_vm will return true if the VM does not exist
+        expect(provider).to receive(:destroy_vm).with(pool,vm).and_return(true)
       end
 
-      it 'should not call any provider methods' do
+      it 'should not raise an error' do
-        subject.destroy_vm(vm,pool,provider)
+        subject._destroy_vm(vm,pool,provider)
       end
     end
 
-    context 'when a VM exists' do
+    context 'when the VM is destroyed without error' do
-      let (:destroy_task) { double('destroy_task') }
-      let (:poweroff_task) { double('poweroff_task') }
-
-      before(:each) do
-        expect(provider).to receive(:find_vm).and_return(host)
-        allow(host).to receive(:runtime).and_return(true)
-      end
-
-      context 'and an error occurs during destroy' do
-        before(:each) do
-          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
-          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
-          expect(destroy_task).to receive(:wait_for_completion).and_raise(RuntimeError,'DestroyFailure')
-          expect(metrics).to receive(:timing).exactly(0).times
-        end
-
-        it 'should raise an error in the thread' do
-          expect { subject.destroy_vm(vm,pool,provider) }.to raise_error(/DestroyFailure/)
-        end
-      end
-
-      context 'and an error occurs during power off' do
-        before(:each) do
-          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
-          expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task)
-          expect(poweroff_task).to receive(:wait_for_completion).and_raise(RuntimeError,'PowerOffFailure')
-          expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down")
-          expect(metrics).to receive(:timing).exactly(0).times
-        end
-
-        it 'should raise an error in the thread' do
-          expect { subject.destroy_vm(vm,pool,provider) }.to raise_error(/PowerOffFailure/)
-        end
-      end
-
-      context 'and is powered off' do
-        before(:each) do
-          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
-          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
-          expect(destroy_task).to receive(:wait_for_completion)
-          expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/)
-        end
-
-        it 'should log a message the VM was destroyed' do
-          expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/)
-          subject.destroy_vm(vm,pool,provider)
-        end
-      end
-
-      context 'and is powered on' do
-        before(:each) do
-          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
-          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
-          expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task)
-          expect(poweroff_task).to receive(:wait_for_completion)
-          expect(destroy_task).to receive(:wait_for_completion)
-          expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/)
-        end
-
-        it 'should log a message the VM is being shutdown' do
-          expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down")
-          allow(logger).to receive(:log)
-
-          subject.destroy_vm(vm,pool,provider)
-        end
-
       it 'should log a message the VM was destroyed' do
         expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/)
         allow(logger).to receive(:log)
 
-          subject.destroy_vm(vm,pool,provider)
+        subject._destroy_vm(vm,pool,provider)
       end
+
+      it 'should emit a timing metric' do
+        expect(metrics).to receive(:timing).with("destroy.#{pool}", String)
+
+        subject._destroy_vm(vm,pool,provider)
+      end
+    end
+
+    context 'when the VM destruction raises an eror' do
+      before(:each) do
+        # As per base_spec, destroy_vm will return true if the VM does not exist
+        expect(provider).to receive(:destroy_vm).with(pool,vm).and_raise('MockError')
+      end
+
+      it 'should not log a message the VM was destroyed' do
+        expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/).exactly(0).times
+        allow(logger).to receive(:log)
+
+        expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(/MockError/)
+      end
+
+      it 'should not emit a timing metric' do
+        expect(metrics).to receive(:timing).with("destroy.#{pool}", String).exactly(0).times
+
+        expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(/MockError/)
       end
     end
   end