From e783e937840006a737c580def51d851448a5997d Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 17 Feb 2017 15:46:35 -0800 Subject: [PATCH] (POOLER-73) Modify spec tests for _migrate_vm Modify spec tests for _migrate_vm --- spec/unit/pool_manager_migration_spec.rb | 87 ------------- spec/unit/pool_manager_spec.rb | 156 +++++++++++++++++++++++ 2 files changed, 156 insertions(+), 87 deletions(-) delete mode 100644 spec/unit/pool_manager_migration_spec.rb diff --git a/spec/unit/pool_manager_migration_spec.rb b/spec/unit/pool_manager_migration_spec.rb deleted file mode 100644 index 9fd491b..0000000 --- a/spec/unit/pool_manager_migration_spec.rb +++ /dev/null @@ -1,87 +0,0 @@ -require 'spec_helper' -require 'mock_redis' -require 'time' - -describe 'Pool Manager' do - let(:logger) { double('logger') } - let(:redis) { MockRedis.new } - let(:metrics) { Vmpooler::DummyStatsd.new } - let(:config) { - { - config: { - 'site_name' => 'test pooler', - 'migration_limit' => 2, - vsphere: { - 'server' => 'vsphere.puppet.com', - 'username' => 'vmpooler@vsphere.local', - 'password' => '', - 'insecure' => true - }, - pools: [ {'name' => 'pool1', 'size' => 5, 'folder' => 'pool1_folder'} ], - statsd: { 'prefix' => 'stats_prefix'}, - pool_names: [ 'pool1' ] - } - } - } - let(:pool) { config[:config][:pools][0]['name'] } - let(:vm) { - { - 'name' => 'vm1', - 'host' => 'host1', - 'template' => pool, - } - } - - describe '#_migrate_vm' do - let(:vsphere) { double(pool) } - let(:pooler) { Vmpooler::PoolManager.new(config, logger, redis, metrics) } - context 'evaluates VM for migration and logs host' do - before do - create_migrating_vm vm['name'], pool, redis - allow(vsphere).to receive(:find_vm).and_return(vm) - allow(pooler).to receive(:get_vm_host_info).and_return([{'name' => 'host1'}, 'host1']) - end - - it 'logs VM host when migration is disabled' do - config[:config]['migration_limit'] = nil - - expect(redis.sismember("vmpooler__migrating__#{pool}", vm['name'])).to be true - expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm['name']}' is running on #{vm['host']}") - - pooler._migrate_vm(vm['name'], pool, vsphere) - - expect(redis.sismember("vmpooler__migrating__#{pool}", vm['name'])).to be false - end - - it 'verifies that migration_limit greater than or equal to migrations in progress and logs host' do - add_vm_to_migration_set vm['name'], redis - add_vm_to_migration_set 'vm2', redis - - expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm['name']}' is running on #{vm['host']}. No migration will be evaluated since the migration_limit has been reached") - - pooler._migrate_vm(vm['name'], pool, vsphere) - end - - it 'verifies that migration_limit is less than migrations in progress and logs old host, new host and migration time' do - allow(vsphere).to receive(:find_least_used_compatible_host).and_return([{'name' => 'host2'}, 'host2']) - allow(vsphere).to receive(:migrate_vm_host) - - expect(redis.hget("vmpooler__vm__#{vm['name']}", 'migration_time')) - expect(redis.hget("vmpooler__vm__#{vm['name']}", 'checkout_to_migration')) - expect(logger).to receive(:log).with('s', "[>] [#{pool}] '#{vm['name']}' migrated from #{vm['host']} to host2 in 0.00 seconds") - - pooler._migrate_vm(vm['name'], pool, vsphere) - end - - it 'fails when no suitable host can be found' do - error = 'ArgumentError: No target host found' - allow(vsphere).to receive(:find_least_used_compatible_host) - allow(vsphere).to receive(:migrate_vm_host).and_raise(error) - - expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm['name']}' migration failed with an error: #{error}") - - pooler._migrate_vm(vm['name'], pool, vsphere) - end - end - end -end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index ba147ab..825d082 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -1418,6 +1418,162 @@ EOT end end + describe "#_migrate_vm" do + let(:vsphere) { double('vsphere') } + let(:vm_parent_hostname) { 'parent1' } + let(:config) { + YAML.load(<<-EOT +--- +:config: + migration_limit: 5 +:pools: + - name: #{pool} +EOT + ) + } + + before do + expect(subject).not_to be_nil + end + + context 'when an error occurs' do + it 'should log an error message and attempt to remove from vmpooler_migration queue' do + expect(vsphere).to receive(:find_vm).with(vm).and_raise(RuntimeError,'MockError') + expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: MockError") + expect(subject).to receive(:remove_vmpooler_migration_vm) + subject._migrate_vm(vm, pool, vsphere) + end + end + + context 'when VM does not exist' do + it 'should log an error message when VM does not exist' do + expect(vsphere).to receive(:find_vm).with(vm).and_return(nil) + # This test is quite fragile. Should refactor the code to make this scenario easier to detect + expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: undefined method `summary' for nil:NilClass") + subject._migrate_vm(vm, pool, vsphere) + end + end + + context 'when VM exists but migration is disabled' do + before(:each) do + expect(vsphere).to receive(:find_vm).with(vm).and_return(host) + allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname]) + create_migrating_vm(vm, pool) + end + + [-1,-32768,false,0].each do |testvalue| + it "should not migrate a VM if the migration limit is #{testvalue}" do + config[:config]['migration_limit'] = testvalue + expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm}' is running on #{vm_parent_hostname}") + subject._migrate_vm(vm, pool, vsphere) + end + + it "should remove the VM from vmpooler__migrating queue in redis if the migration limit is #{testvalue}" do + redis.sadd("vmpooler__migrating__#{pool}", vm) + config[:config]['migration_limit'] = testvalue + + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy + subject._migrate_vm(vm, pool, vsphere) + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey + end + end + end + + context 'when VM exists but migration limit is reached' do + before(:each) do + expect(vsphere).to receive(:find_vm).with(vm).and_return(host) + allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname]) + + create_migrating_vm(vm, pool) + redis.sadd('vmpooler__migration', 'fakevm1') + redis.sadd('vmpooler__migration', 'fakevm2') + redis.sadd('vmpooler__migration', 'fakevm3') + redis.sadd('vmpooler__migration', 'fakevm4') + redis.sadd('vmpooler__migration', 'fakevm5') + end + + it "should not migrate a VM if the migration limit is reached" do + expect(logger).to receive(:log).with('s',"[ ] [#{pool}] '#{vm}' is running on #{vm_parent_hostname}. No migration will be evaluated since the migration_limit has been reached") + subject._migrate_vm(vm, pool, vsphere) + end + + it "should remove the VM from vmpooler__migrating queue in redis if the migration limit is reached" do + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy + subject._migrate_vm(vm, pool, vsphere) + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey + end + end + + context 'when VM exists but migration limit is not yet reached' do + before(:each) do + expect(vsphere).to receive(:find_vm).with(vm).and_return(host) + allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname]) + + create_migrating_vm(vm, pool) + redis.sadd('vmpooler__migration', 'fakevm1') + redis.sadd('vmpooler__migration', 'fakevm2') + end + + context 'and host to migrate to is the same as the current host' do + before(:each) do + expect(vsphere).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname]) + end + + it "should not migrate the VM" do + expect(logger).to receive(:log).with('s', "[ ] [#{pool}] No migration required for '#{vm}' running on #{vm_parent_hostname}") + subject._migrate_vm(vm, pool, vsphere) + end + + it "should remove the VM from vmpooler__migrating queue in redis" do + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy + subject._migrate_vm(vm, pool, vsphere) + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey + end + + it "should not change the vmpooler_migration queue count" do + before_count = redis.scard('vmpooler__migration') + subject._migrate_vm(vm, pool, vsphere) + expect(redis.scard('vmpooler__migration')).to eq(before_count) + end + + it "should call remove_vmpooler_migration_vm" do + expect(subject).to receive(:remove_vmpooler_migration_vm) + subject._migrate_vm(vm, pool, vsphere) + end + end + + context 'and host to migrate to different to the current host' do + let(:vm_new_hostname) { 'new_hostname' } + before(:each) do + expect(vsphere).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_new_hostname}, vm_new_hostname]) + expect(subject).to receive(:migrate_vm_and_record_timing).with(host, vm, pool, Object, vm_parent_hostname, vm_new_hostname, vsphere).and_return('1.00') + end + + it "should migrate the VM" do + expect(logger).to receive(:log).with('s', "[>] [#{pool}] '#{vm}' migrated from #{vm_parent_hostname} to #{vm_new_hostname} in 1.00 seconds") + subject._migrate_vm(vm, pool, vsphere) + end + + it "should remove the VM from vmpooler__migrating queue in redis" do + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy + subject._migrate_vm(vm, pool, vsphere) + expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey + end + + it "should not change the vmpooler_migration queue count" do + before_count = redis.scard('vmpooler__migration') + subject._migrate_vm(vm, pool, vsphere) + expect(redis.scard('vmpooler__migration')).to eq(before_count) + end + + it "should call remove_vmpooler_migration_vm" do + expect(subject).to receive(:remove_vmpooler_migration_vm) + subject._migrate_vm(vm, pool, vsphere) + end + end + end + end + describe "#get_vm_host_info" do before do expect(subject).not_to be_nil