(GH-185) Remove unnecessary checks in check_ready_vm

Previously in check_ready_vm, if the VM is powered off, the VM is moved in
redis however the function doesn't return there, and instead then checks if the
hostname is the same, and then if TCP socket 22 is open. This is unnecessary as
we already know the VM is turned off so of course the hostname is wrong and TCP
22 is unavailable. The same applies for the VM hostname.

This commit instead returns after it is found a VM is no longer ready.  This
commit also amends the spec tests for the correct behaviour.

lib/vmpooler/pool_manager.rb

@@ -102,6 +102,7 @@ def check_ready_vm(vm, pool, ttl, vsphere)
             $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 @@ def check_ready_vm(vm, pool, ttl, vsphere)
               $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 @@ def check_ready_vm(vm, pool, ttl, vsphere)
               $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 @@ def check_ready_vm(vm, pool, ttl, vsphere)
             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,44 +345,88 @@
         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')
           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 multiple times' 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).at_least(2).times
+        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' 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)
           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 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
-         # as the VM is no longer in the ready queue, redis also throws an error
-         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, 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
-          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
+        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 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