From e55a8825af0fed962d3c33229c36c74f5ca93c36 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Mon, 10 Jul 2017 17:21:27 -0700 Subject: [PATCH] (maint) Remove phatom VMs and ensure inventory is successful Previously, if inventory failed for some reason, it would return an incomplete set of VMs which could then cause the pool to perform off behaviours such as fill the pool high than it should, or remove VMs which exist. Also, if the redis cache of VMs in a pool had a VM but it did not actually exist in the inventory it would never be removed. This commit: - Immediately exits the check_pool if an error occurs during inventory collection - Will mark a VM as completed if it exists in Redis, but does not exist in inventory - Adds tests for these behaviours --- lib/vmpooler/pool_manager.rb | 5 +++ spec/unit/pool_manager_spec.rb | 78 ++++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index f1c5586..999308f 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -603,6 +603,7 @@ module Vmpooler end rescue => err $logger.log('s', "[!] [#{pool['name']}] _check_pool failed with an error while inspecting inventory: #{err}") + return pool_check_response end # RUNNING @@ -615,6 +616,8 @@ module Vmpooler rescue => err $logger.log('d', "[!] [#{pool['name']}] _check_pool with an error while evaluating running VMs: #{err}") end + else + move_vm_queue(pool['name'], vm, 'running', 'completed', 'is a running VM but is missing from inventory. Marking as completed.') end end @@ -627,6 +630,8 @@ module Vmpooler rescue => err $logger.log('d', "[!] [#{pool['name']}] _check_pool failed with an error while evaluating ready VMs: #{err}") end + else + move_vm_queue(pool['name'], vm, 'ready', 'completed', 'is a ready VM but is missing from inventory. Marking as completed.') end end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 11e5529..139794a 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2347,12 +2347,13 @@ EOT task_limit: 10 :pools: - name: #{pool} - size: 0 + size: 10 EOT ) } let(:pool_object) { config[:pools][0] } let(:new_vm) { 'newvm'} + let(:pool_name) { pool_object['name'] } before do expect(subject).not_to be_nil @@ -2378,6 +2379,46 @@ EOT subject._check_pool(pool_object,provider) end + it 'should not perform any other actions if an error occurs' do + # Add VMs into redis + create_running_vm(pool_name, 'vm1') + create_ready_vm(pool_name, 'vm2') + create_completed_vm('vm3', pool_name) + create_discovered_vm('vm4', pool_name) + create_migrating_vm('vm5', pool_name) + + expect(subject).to receive(:move_vm_queue).exactly(0).times + expect(subject).to receive(:check_running_vm).exactly(0).times + expect(subject).to receive(:check_pending_vm).exactly(0).times + expect(subject).to receive(:destroy_vm).exactly(0).times + expect(redis).to receive(:srem).exactly(0).times + expect(redis).to receive(:del).exactly(0).times + expect(redis).to receive(:hdel).exactly(0).times + expect(redis).to receive(:smove).exactly(0).times + expect(subject).to receive(:migrate_vm).exactly(0).times + expect(redis).to receive(:set).exactly(0).times + expect(redis).to receive(:incr).exactly(0).times + expect(subject).to receive(:clone_vm).exactly(0).times + expect(redis).to receive(:decr).exactly(0).times + + expect(provider).to receive(:vms_in_pool).and_raise(RuntimeError,'Mock Error') + subject._check_pool(pool_object,provider) + end + + it 'should return that no actions were taken' do + expect(provider).to receive(:vms_in_pool).and_raise(RuntimeError,'Mock Error') + + result = subject._check_pool(pool_object,provider) + + expect(result[:discovered_vms]).to be(0) + expect(result[:checked_running_vms]).to be(0) + expect(result[:checked_ready_vms]).to be(0) + expect(result[:checked_pending_vms]).to be(0) + expect(result[:destroyed_vms]).to be(0) + expect(result[:migrated_vms]).to be(0) + expect(result[:cloned_vms]).to be(0) + end + it 'should log the discovery of VMs' do expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue") @@ -2428,11 +2469,17 @@ EOT create_running_vm(pool,vm,token) end - it 'should not do anything' do + it 'should not call check_running_vm' do expect(subject).to receive(:check_running_vm).exactly(0).times subject._check_pool(pool_object,provider) end + + it 'should move the VM to completed queue' do + expect(subject).to receive(:move_vm_queue).with(pool,vm,'running','completed',String).and_call_original + + subject._check_pool(pool_object,provider) + end end context 'Running VM in the inventory' do @@ -2487,11 +2534,17 @@ EOT create_ready_vm(pool,vm,token) end - it 'should not do anything' do + it 'should not call check_ready_vm' do expect(subject).to receive(:check_ready_vm).exactly(0).times subject._check_pool(pool_object,provider) end + + it 'should move the VM to completed queue' do + expect(subject).to receive(:move_vm_queue).with(pool,vm,'ready','completed',String).and_call_original + + subject._check_pool(pool_object,provider) + end end context 'Ready VM in the inventory' do @@ -2770,6 +2823,17 @@ EOT # REPOPULATE context 'Repopulate a pool' do + let(:config) { + YAML.load(<<-EOT +--- +:config: + task_limit: 10 +:pools: + - name: #{pool} + size: 0 +EOT + ) + } it 'should not call clone_vm when number of VMs is equal to the pool size' do expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) expect(subject).to receive(:clone_vm).exactly(0).times @@ -2817,18 +2881,24 @@ EOT end context 'when pool is marked as empty' do + let(:vm_response) { + # Mock response from Base Provider for vms_in_pool + [{ 'name' => vm}] + } + before(:each) do - expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) redis.set("vmpooler__empty__#{pool}", 'true') end it 'should not log a message when the pool remains empty' do + expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty").exactly(0).times subject._check_pool(pool_object,provider) end it 'should remove the empty pool mark if it is no longer empty' do + expect(provider).to receive(:vms_in_pool).with(pool).and_return(vm_response) create_ready_vm(pool,vm,token) expect(redis.get("vmpooler__empty__#{pool}")).to be_truthy