(POOLER-70) Update migrate_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

lib/vmpooler/pool_manager.rb

@@ -458,42 +458,43 @@ module Vmpooler
       migration_limit if migration_limit >= 1
     end
 
-    def migrate_vm(vm, pool, provider)
+    def migrate_vm(vm_name, pool_name, provider)
       Thread.new do
-        _migrate_vm(vm, pool, provider)
+        begin
+          _migrate_vm(vm_name, pool_name, provider)
+        rescue => err
+          $logger.log('s', "[x] [#{pool_name}] '#{vm_name}' migration failed with an error: #{err}")
+          remove_vmpooler_migration_vm(pool_name, vm_name)
+        end
       end
     end
 
-    def _migrate_vm(vm, pool, provider)
-      begin
-        $redis.srem('vmpooler__migrating__' + pool, vm)
-        vm_object = provider.find_vm(vm)
-        parent_host, parent_host_name = get_vm_host_info(vm_object)
-        migration_limit = migration_limit $config[:config]['migration_limit']
-        migration_count = $redis.scard('vmpooler__migration')
+    def _migrate_vm(vm_name, pool_name, provider)
+      $redis.srem('vmpooler__migrating__' + pool_name, vm_name)
 
-        if ! migration_limit
-          $logger.log('s', "[ ] [#{pool}] '#{vm}' is running on #{parent_host_name}")
+      parent_host_name = provider.get_vm_host(pool_name, vm_name)
+      raise('Unable to determine which host the VM is running on') if parent_host_name.nil?
+      migration_limit = migration_limit $config[:config]['migration_limit']
+      migration_count = $redis.scard('vmpooler__migration')
+
+      if ! migration_limit
+        $logger.log('s', "[ ] [#{pool_name}] '#{vm_name}' is running on #{parent_host_name}")
+        return
+      else
+        if migration_count >= migration_limit
+          $logger.log('s', "[ ] [#{pool_name}] '#{vm_name}' is running on #{parent_host_name}. No migration will be evaluated since the migration_limit has been reached")
           return
         else
-          if migration_count >= migration_limit
-            $logger.log('s', "[ ] [#{pool}] '#{vm}' is running on #{parent_host_name}. No migration will be evaluated since the migration_limit has been reached")
-            return
+          $redis.sadd('vmpooler__migration', vm_name)
+          host_name = provider.find_least_used_compatible_host(vm_name)
+          if host_name == parent_host_name
+            $logger.log('s', "[ ] [#{pool_name}] No migration required for '#{vm_name}' running on #{parent_host_name}")
           else
-            $redis.sadd('vmpooler__migration', vm)
-            host, host_name = provider.find_least_used_compatible_host(vm_object)
-            if host == parent_host
-              $logger.log('s', "[ ] [#{pool}] No migration required for '#{vm}' running on #{parent_host_name}")
-            else
-              finish = migrate_vm_and_record_timing(vm_object, vm, pool, host, parent_host_name, host_name, provider)
-              $logger.log('s', "[>] [#{pool}] '#{vm}' migrated from #{parent_host_name} to #{host_name} in #{finish} seconds")
-            end
-            remove_vmpooler_migration_vm(pool, vm)
+            finish = migrate_vm_and_record_timing(vm_name, pool_name, parent_host_name, host_name, provider)
+            $logger.log('s', "[>] [#{pool_name}] '#{vm_name}' migrated from #{parent_host_name} to #{host_name} in #{finish} seconds")
           end
+          remove_vmpooler_migration_vm(pool_name, vm_name)
         end
-      rescue => err
-        $logger.log('s', "[x] [#{pool}] '#{vm}' migration failed with an error: #{err}")
-        remove_vmpooler_migration_vm(pool, vm)
       end
     end
 

spec/unit/pool_manager_spec.rb

@@ -1487,60 +1487,75 @@ EOT
   end
 
   describe '#migrate_vm' do
-    let(:provider) { double('provider') }
-
-    before do
+    before(:each) do
       expect(subject).not_to be_nil
+      expect(Thread).to receive(:new).and_yield
     end
 
     it 'calls _migrate_vm' do
-      expect(Thread).to receive(:new).and_yield
       expect(subject).to receive(:_migrate_vm).with(vm, pool, provider)
 
       subject.migrate_vm(vm, pool, provider)
     end
+
+    context 'When an error is raised' do
+      before(:each) do
+        expect(subject).to receive(:_migrate_vm).with(vm, pool, provider).and_raise('MockError')
+      end
+
+      it 'logs a message' do
+        allow(logger).to receive(:log)
+        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: MockError")
+
+        subject.migrate_vm(vm, pool, provider)
+      end
+
+      it 'should attempt to remove from vmpooler_migration queue' do
+        expect(subject).to receive(:remove_vmpooler_migration_vm).with(pool, vm)
+
+        subject.migrate_vm(vm, pool, provider)
+      end
+    end
   end
 
   describe "#_migrate_vm" do
-    let(:provider) { double('provider') }
     let(:vm_parent_hostname) { 'parent1' }
     let(:config) {
       YAML.load(<<-EOT
 ---
 :config:
   migration_limit: 5
-:pools:
-  - name: #{pool}
 EOT
       )
     }
 
-    before do
+    before(:each) do
       expect(subject).not_to be_nil
+      allow(provider).to receive(:get_vm_host).with(pool, vm).and_return(vm_parent_hostname)
     end
 
-    context 'when an error occurs' do
-      it 'should log an error message and attempt to remove from vmpooler_migration queue' do
-        expect(provider).to receive(:find_vm).with(vm).and_raise(RuntimeError,'MockError')
-        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: MockError")
-        expect(subject).to receive(:remove_vmpooler_migration_vm)
-        subject._migrate_vm(vm, pool, provider)
+    context 'when an error occurs trying to retrieve the current host' do
+      before(:each) do
+        expect(provider).to receive(:get_vm_host).with(pool, vm).and_raise(RuntimeError,'MockError')
+      end
+
+      it 'should raise an error' do
+        expect{ subject._migrate_vm(vm, pool, provider) }.to raise_error('MockError')
       end
     end
 
-    context 'when VM does not exist' do
-      it 'should log an error message when VM does not exist' do
-        expect(provider).to receive(:find_vm).with(vm).and_return(nil)
-        # This test is quite fragile.  Should refactor the code to make this scenario easier to detect
-        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: undefined method `summary' for nil:NilClass")
-        subject._migrate_vm(vm, pool, provider)
+    context 'when the current host can not be determined' do
+      before(:each) do
+        expect(provider).to receive(:get_vm_host).with(pool, vm).and_return(nil)
+      end
+
+      it 'should raise an error' do
+        expect{ subject._migrate_vm(vm, pool, provider) }.to raise_error(/Unable to determine which host the VM is running on/)
       end
     end
 
     context 'when VM exists but migration is disabled' do
       before(:each) do
-        expect(provider).to receive(:find_vm).with(vm).and_return(host)
-        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
         create_migrating_vm(vm, pool)
       end
 
@@ -1564,8 +1579,6 @@ EOT
 
     context 'when VM exists but migration limit is reached' do
       before(:each) do
-        expect(provider).to receive(:find_vm).with(vm).and_return(host)
-        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
 
         create_migrating_vm(vm, pool)
         redis.sadd('vmpooler__migration', 'fakevm1')
@@ -1589,9 +1602,6 @@ EOT
 
     context 'when VM exists but migration limit is not yet reached' do
       before(:each) do
-        expect(provider).to receive(:find_vm).with(vm).and_return(host)
-        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
-
         create_migrating_vm(vm, pool)
         redis.sadd('vmpooler__migration', 'fakevm1')
         redis.sadd('vmpooler__migration', 'fakevm2')
@@ -1599,7 +1609,7 @@ EOT
 
       context 'and host to migrate to is the same as the current host' do
         before(:each) do
-          expect(provider).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
+          expect(provider).to receive(:find_least_used_compatible_host).with(vm).and_return(vm_parent_hostname)
         end
 
         it "should not migrate the VM" do
@@ -1628,8 +1638,8 @@ EOT
       context 'and host to migrate to different to the current host' do
         let(:vm_new_hostname) { 'new_hostname' }
         before(:each) do
-          expect(provider).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_new_hostname}, vm_new_hostname])
-          expect(subject).to receive(:migrate_vm_and_record_timing).with(host, vm, pool, Object, vm_parent_hostname, vm_new_hostname, provider).and_return('1.00')
+          expect(provider).to receive(:find_least_used_compatible_host).with(vm).and_return(vm_new_hostname)
+          expect(subject).to receive(:migrate_vm_and_record_timing).with(vm, pool, vm_parent_hostname, vm_new_hostname, provider).and_return('1.00')
         end
 
         it "should migrate the VM" do