From f4330567342820bbd473c67c58a708d2d159a882 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Wed, 1 Mar 2017 15:26:33 -0800 Subject: [PATCH] (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 | 4 ++ spec/unit/pool_manager_spec.rb | 70 +++++++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index f9103ef..6014fb6 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/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 diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 825d082..f654ecb 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/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 - # 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) @@ -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 - # 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