Merge pull request #188 from glennsarti/ticket/master/gh-185

(GH-185) Remove unnecessary checks in check_ready_vm

lib/vmpooler/pool_manager.rb

@@ -102,6 +102,7 @@ module Vmpooler
             $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm)
 
             $logger.log('d', "[!] [#{pool}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue")
+            return
           end
         end
 
@@ -124,6 +125,7 @@ module Vmpooler
               $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm)
 
               $logger.log('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
+              return
             end
 
             if
@@ -134,6 +136,7 @@ module Vmpooler
               $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm)
 
               $logger.log('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue")
+              return
             end
           else
             $redis.srem('vmpooler__ready__' + pool, vm)
@@ -149,6 +152,7 @@ module Vmpooler
             else
               $logger.log('d', "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue")
             end
+            return
           end
         end
       end

spec/unit/pool_manager_spec.rb

@@ -345,7 +345,7 @@ EOT
         end
       end
 
-      context 'but turned off and name mismatch' do
+      context 'is turned off, a name mismatch and not available via TCP' do
         before(:each) do
           allow(host).to receive(:runtime).and_return( double('runtime') )
           allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
@@ -353,14 +353,13 @@ EOT
           allow(subject).to receive(:open_socket).with(vm).and_raise(SocketError,'getaddrinfo: No such host is known')
         end
 
-        it 'should move the VM to the completed queue multiple times' do
+        it 'should move the VM to the completed queue' do
-          # There is an implementation bug which attempts the move multiple times
+          expect(redis).to receive(:smove).with("vmpooler__ready__#{pool}", "vmpooler__completed__#{pool}", vm)
-          expect(redis).to receive(:smove).with("vmpooler__ready__#{pool}", "vmpooler__completed__#{pool}", vm).at_least(2).times
 
           subject.check_ready_vm(vm, pool, ttl, vsphere)
         end
 
-        it 'should move the VM to the completed queue' do
+        it 'should move the VM to the completed queue in Redis' do
           expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
           expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(false)
           subject.check_ready_vm(vm, pool, ttl, vsphere)
@@ -368,21 +367,66 @@ EOT
           expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
         end
 
-        it 'should log messages about being powered off, name mismatch and removed from ready queue' do
+        it 'should log messages about being powered off' do
           expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
-          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue")
 
-         # There is an implementation bug which attempts the move multiple times however
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
-         # as the VM is no longer in the ready queue, redis also throws an error
+        end
-         expect(logger).to receive(:log).with("d", "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue")
+      end
+
+      context 'is turned on, a name mismatch and not available via TCP' do
+        before(:each) do
+          allow(host).to receive(:runtime).and_return( double('runtime') )
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
+          allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return ('')
+          allow(subject).to receive(:open_socket).with(vm).and_raise(SocketError,'getaddrinfo: No such host is known')
+        end
+
+        it 'should move the VM to the completed queue' do
+          expect(redis).to receive(:smove).with("vmpooler__ready__#{pool}", "vmpooler__completed__#{pool}", vm)
 
           subject.check_ready_vm(vm, pool, ttl, vsphere)
         end
 
-        it 'should log a message if it fails to move queues' do
+        it 'should move the VM to the completed queue in Redis' do
-          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(false)
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(false)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
+        end
+
+        it 'should log messages about being misnamed' do
           expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue")
-          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue")
+
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+      end
+
+      context 'is turned on, with correct name and not available via TCP' do
+        before(:each) do
+          allow(host).to receive(:runtime).and_return( double('runtime') )
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
+          allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return (vm)
+          allow(subject).to receive(:open_socket).with(vm).and_raise(SocketError,'getaddrinfo: No such host is known')
+        end
+
+        it 'should move the VM to the completed queue' do
+          expect(redis).to receive(:smove).with("vmpooler__ready__#{pool}", "vmpooler__completed__#{pool}", vm)
+
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+
+        it 'should move the VM to the completed queue in Redis' do
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(false)
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(false)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
+        end
+
+        it 'should log messages about being unreachable' do
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' is unreachable, removed from 'ready' queue")
 
           subject.check_ready_vm(vm, pool, ttl, vsphere)
         end