From 64bca33d456669884cd44bfb9261a4ec811ed5e4 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 31 Mar 2017 13:59:59 -0700 Subject: [PATCH] (POOLER-70) Update destroy_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 - Splits the destroy_vm function into two. One function spawns the thread while the other actually does the work. This makes testing much easier. --- lib/vmpooler/pool_manager.rb | 48 +++++------ spec/unit/pool_manager_spec.rb | 153 +++++++++++++-------------------- 2 files changed, 81 insertions(+), 120 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 5066dd6..ae5c510 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -229,36 +229,32 @@ module Vmpooler # Destroy a VM def destroy_vm(vm, pool, provider) Thread.new do - $redis.srem('vmpooler__completed__' + pool, vm) - $redis.hdel('vmpooler__active__' + pool, vm) - $redis.hset('vmpooler__vm__' + vm, 'destroy', Time.now) - - # Auto-expire metadata key - $redis.expire('vmpooler__vm__' + vm, ($config[:redis]['data_ttl'].to_i * 60 * 60)) - - host = provider.find_vm(vm) - - if host - start = Time.now - - if - (host.runtime) && - (host.runtime.powerState) && - (host.runtime.powerState == 'poweredOn') - - $logger.log('d', "[ ] [#{pool}] '#{vm}' is being shut down") - host.PowerOffVM_Task.wait_for_completion - end - - host.Destroy_Task.wait_for_completion - finish = '%.2f' % (Time.now - start) - - $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds") - $metrics.timing("destroy.#{pool}", finish) + begin + _destroy_vm(vm, pool, provider) + rescue => err + $logger.log('d', "[!] [#{pool}] '#{vm}' failed while destroying the VM with an error: #{err}") + raise end end end + def _destroy_vm(vm, pool, provider) + $redis.srem('vmpooler__completed__' + pool, vm) + $redis.hdel('vmpooler__active__' + pool, vm) + $redis.hset('vmpooler__vm__' + vm, 'destroy', Time.now) + + # Auto-expire metadata key + $redis.expire('vmpooler__vm__' + vm, ($config[:redis]['data_ttl'].to_i * 60 * 60)) + + start = Time.now + + provider.destroy_vm(pool, vm) + + finish = '%.2f' % (Time.now - start) + $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds") + $metrics.timing("destroy.#{pool}", finish) + end + def create_vm_disk(vm, disk_size, provider) Thread.new do _create_vm_disk(vm, disk_size, provider) diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 6c4af1a..13c0b3f 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -617,140 +617,105 @@ EOT end end - describe "#destroy_vm" do - let (:provider) { double('provider') } - - let(:config) { - YAML.load(<<-EOT ---- -:redis: - data_ttl: 168 -EOT - ) - } - + describe '#destroy_vm' do before do expect(subject).not_to be_nil + expect(Thread).to receive(:new).and_yield end + it 'calls _destroy_vm' do + expect(subject).to receive(:_destroy_vm).with(vm,pool,provider) + + subject.destroy_vm(vm,pool,provider) + end + + it 'logs a message if an error is raised' do + allow(logger).to receive(:log) + expect(logger).to receive(:log).with('d',"[!] [#{pool}] '#{vm}' failed while destroying the VM with an error: MockError") + expect(subject).to receive(:_destroy_vm).with(vm,pool,provider).and_raise('MockError') + + expect{subject.destroy_vm(vm,pool,provider)}.to raise_error(/MockError/) + end + end + + describe "#_destroy_vm" do before(:each) do - expect(Thread).to receive(:new).and_yield + expect(subject).not_to be_nil create_completed_vm(vm,pool,true) + + allow(provider).to receive(:destroy_vm).with(pool,vm).and_return(true) + + # Set redis configuration + config[:redis] = {} + config[:redis]['data_ttl'] = 168 end context 'when redis data_ttl is not specified in the configuration' do - let(:config) { - YAML.load(<<-EOT ---- -:redis: - "key": "value" -EOT - ) - } - before(:each) do - expect(provider).to receive(:find_vm).and_return(nil) + config[:redis]['data_ttl'] = nil end it 'should call redis expire with 0' do expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to_not be_nil - subject.destroy_vm(vm,pool,provider) + subject._destroy_vm(vm,pool,provider) expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to be_nil end end context 'when there is no redis section in the configuration' do - let(:config) {} + before(:each) do + config[:redis] = nil + end it 'should raise an error' do - expect{ subject.destroy_vm(vm,pool,provider) }.to raise_error(NoMethodError) + expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(NoMethodError) end end context 'when a VM does not exist' do before(:each) do - expect(provider).to receive(:find_vm).and_return(nil) + # As per base_spec, destroy_vm will return true if the VM does not exist + expect(provider).to receive(:destroy_vm).with(pool,vm).and_return(true) end - it 'should not call any provider methods' do - subject.destroy_vm(vm,pool,provider) + it 'should not raise an error' do + subject._destroy_vm(vm,pool,provider) end end - context 'when a VM exists' do - let (:destroy_task) { double('destroy_task') } - let (:poweroff_task) { double('poweroff_task') } + context 'when the VM is destroyed without error' do + it 'should log a message the VM was destroyed' do + expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/) + allow(logger).to receive(:log) + subject._destroy_vm(vm,pool,provider) + end + + it 'should emit a timing metric' do + expect(metrics).to receive(:timing).with("destroy.#{pool}", String) + + subject._destroy_vm(vm,pool,provider) + end + end + + context 'when the VM destruction raises an eror' do before(:each) do - expect(provider).to receive(:find_vm).and_return(host) - allow(host).to receive(:runtime).and_return(true) + # As per base_spec, destroy_vm will return true if the VM does not exist + expect(provider).to receive(:destroy_vm).with(pool,vm).and_raise('MockError') end - context 'and an error occurs during destroy' do - before(:each) do - allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff') - expect(host).to receive(:Destroy_Task).and_return(destroy_task) - expect(destroy_task).to receive(:wait_for_completion).and_raise(RuntimeError,'DestroyFailure') - expect(metrics).to receive(:timing).exactly(0).times - end + it 'should not log a message the VM was destroyed' do + expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/).exactly(0).times + allow(logger).to receive(:log) - it 'should raise an error in the thread' do - expect { subject.destroy_vm(vm,pool,provider) }.to raise_error(/DestroyFailure/) - end + expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(/MockError/) end - context 'and an error occurs during power off' do - before(:each) do - allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn') - expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task) - expect(poweroff_task).to receive(:wait_for_completion).and_raise(RuntimeError,'PowerOffFailure') - expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down") - expect(metrics).to receive(:timing).exactly(0).times - end + it 'should not emit a timing metric' do + expect(metrics).to receive(:timing).with("destroy.#{pool}", String).exactly(0).times - it 'should raise an error in the thread' do - expect { subject.destroy_vm(vm,pool,provider) }.to raise_error(/PowerOffFailure/) - end - end - - context 'and is powered off' do - before(:each) do - allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff') - expect(host).to receive(:Destroy_Task).and_return(destroy_task) - expect(destroy_task).to receive(:wait_for_completion) - expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/) - end - - it 'should log a message the VM was destroyed' do - expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/) - subject.destroy_vm(vm,pool,provider) - end - end - - context 'and is powered on' do - before(:each) do - allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn') - expect(host).to receive(:Destroy_Task).and_return(destroy_task) - expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task) - expect(poweroff_task).to receive(:wait_for_completion) - expect(destroy_task).to receive(:wait_for_completion) - expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/) - end - - it 'should log a message the VM is being shutdown' do - expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down") - allow(logger).to receive(:log) - - subject.destroy_vm(vm,pool,provider) - end - - it 'should log a message the VM was destroyed' do - expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/) - allow(logger).to receive(:log) - - subject.destroy_vm(vm,pool,provider) - end + expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(/MockError/) end end end