From 81d71b8a13d4e10d56dd8c01d7118cc4f463d26a Mon Sep 17 00:00:00 2001 From: Samuel Beaulieu Date: Tue, 7 Dec 2021 14:54:16 -0600 Subject: [PATCH] Move vSphere specific methods out of VMPooler VMPooler has the vSphere provider taken out, moving some vSphere related methods to the provider: 1) pool_folders 2) get_base_folders And the related spec tests. At the same time renaming some configuration and code items to remove harmful terminology. Note this version of the vsphere provider needs to run on vmpooler that also contain the renaming changes (version >2.1) --- Gemfile.lock | 6 +- lib/vmpooler/providers/vsphere.rb | 31 +++++++-- spec/unit/pool_manager_spec.rb | 102 ++++++---------------------- spec/unit/providers/vsphere_spec.rb | 84 ++++++++++++++++++----- vmpooler-provider-vsphere.gemspec | 2 +- 5 files changed, 118 insertions(+), 107 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5bde0f7..23c1771 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -136,7 +136,7 @@ GEM rubocop-ast (>= 1.0.1) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 2.0) - rubocop-ast (1.14.0) + rubocop-ast (1.15.0) parser (>= 3.0.1.1) ruby-progressbar (1.11.0) ruby2_keywords (0.0.5) @@ -160,7 +160,7 @@ GEM thrift (0.15.0) tilt (2.0.10) unicode-display_width (1.8.0) - vmpooler (2.0.0) + vmpooler (2.1.0) concurrent-ruby (~> 1.1) connection_pool (~> 2.2) net-ldap (~> 0.16) @@ -197,7 +197,7 @@ DEPENDENCIES rubocop (~> 1.1.0) simplecov (>= 0.11.2) thor (~> 1.0, >= 1.0.1) - vmpooler (~> 2.0) + vmpooler (~> 2.1) vmpooler-provider-vsphere! yarjuf (>= 2.0) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index b860743..c572d48 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -51,8 +51,8 @@ module Vmpooler 'vsphere' end - def folder_configured?(folder_title, base_folder, configured_folders, whitelist) - return true if whitelist&.include?(folder_title) + def folder_configured?(folder_title, base_folder, configured_folders, allowlist) + return true if allowlist&.include?(folder_title) return false unless configured_folders.keys.include?(folder_title) return false unless configured_folders[folder_title] == base_folder @@ -119,7 +119,30 @@ module Vmpooler try >= max_tries ? raise : retry end - def purge_unconfigured_folders(base_folders, configured_folders, whitelist) + # Return a list of pool folders + def pool_folders(provider_name) + folders = {} + $config[:pools].each do |pool| + next unless pool['provider'] == provider_name.to_s + + folder_parts = pool['folder'].split('/') + datacenter = get_target_datacenter_from_config(pool['name']) + folders[folder_parts.pop] = "#{datacenter}/vm/#{folder_parts.join('/')}" + end + folders + end + + def get_base_folders(folders) + base = [] + folders.each do |_key, value| + base << value + end + base.uniq + end + + def purge_unconfigured_resources(allowlist) + configured_folders = pool_folders(name) + base_folders = get_base_folders(configured_folders) @connection_pool.with_metrics do |pool_object| connection = ensured_vsphere_connection(pool_object) @@ -129,7 +152,7 @@ module Vmpooler folder_children.each do |folder_hash| folder_hash.each do |folder_title, folder_object| - destroy_folder_and_children(folder_object) unless folder_configured?(folder_title, base_folder, configured_folders, whitelist) + destroy_folder_and_children(folder_object) unless folder_configured?(folder_title, base_folder, configured_folders, allowlist) end end end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 24dae84..4f551eb 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -61,7 +61,7 @@ EOT --- :config: :providers: - :mock: + :vsphere: :pools: - name: '#{pool}' size: 1 @@ -1060,7 +1060,7 @@ EOT end end - describe '#purge_unused_vms_and_folders' do + describe '#purge_unused_vms_and_resources' do let(:config) { YAML.load(<<-EOT --- :config: {} @@ -1074,104 +1074,46 @@ EOT } it 'should return when purging is not enabled' do - expect(subject.purge_unused_vms_and_folders).to be_nil + expect(subject.purge_unused_vms_and_resources).to be_nil end context 'with purging enabled globally' do before(:each) do - config[:config]['purge_unconfigured_folders'] = true + config[:config]['purge_unconfigured_resources'] = true expect(Thread).to receive(:new).and_yield end it 'should run a purge for each provider' do - expect(subject).to receive(:purge_vms_and_folders) + expect(subject).to receive(:purge_vms_and_resources) - subject.purge_unused_vms_and_folders + subject.purge_unused_vms_and_resources end it 'should log when purging fails' do - expect(subject).to receive(:purge_vms_and_folders).and_raise(RuntimeError,'MockError') + expect(subject).to receive(:purge_vms_and_resources).and_raise(RuntimeError,'MockError') expect(logger).to receive(:log).with('s', '[!] failed while purging provider mock VMs and folders with an error: MockError') - subject.purge_unused_vms_and_folders + subject.purge_unused_vms_and_resources end end context 'with purging enabled on the provider' do before(:each) do - config[:providers][:mock]['purge_unconfigured_folders'] = true + config[:providers][:mock]['purge_unconfigured_resources'] = true expect(Thread).to receive(:new).and_yield end it 'should run a purge for the provider' do - expect(subject).to receive(:purge_vms_and_folders) + expect(subject).to receive(:purge_vms_and_resources) - subject.purge_unused_vms_and_folders + subject.purge_unused_vms_and_resources end end end - describe '#pool_folders' do - let(:folder_name) { 'myinstance' } - let(:folder_base) { 'vmpooler' } - let(:folder) { [folder_base,folder_name].join('/') } - let(:datacenter) { 'dc1' } + describe '#purge_vms_and_resources' do let(:provider_name) { 'mock_provider' } - let(:expected_response) { - { - folder_name => "#{datacenter}/vm/#{folder_base}" - } - } - let(:config) { YAML.load(<<-EOT ---- -:providers: - :mock: -:pools: - - name: '#{pool}' - folder: '#{folder}' - size: 1 - datacenter: '#{datacenter}' - provider: '#{provider_name}' - - name: '#{pool}2' - folder: '#{folder}' - size: 1 - datacenter: '#{datacenter}' - provider: '#{provider_name}2' -EOT - ) - } - - context 'when evaluating pool folders' do - before do - expect(subject).not_to be_nil - # Inject mock provider into global variable - Note this is a code smell - $providers = { provider_name => provider } - end - - it 'should return a list of pool folders' do - expect(provider).to receive(:get_target_datacenter_from_config).with(pool).and_return(datacenter) - - expect(subject.pool_folders(provider_name)).to eq(expected_response) - end - - it 'should raise an error when the provider fails to get the datacenter' do - expect(provider).to receive(:get_target_datacenter_from_config).with(pool).and_raise('mockerror') - - expect{ subject.pool_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror') - end - end - end - - describe '#purge_vms_and_folders' do - let(:folder_name) { 'myinstance' } - let(:folder_base) { 'vmpooler' } - let(:datacenter) { 'dc1' } - let(:full_folder_path) { "#{datacenter}/vm/folder_base" } - let(:configured_folders) { { folder_name => full_folder_path } } - let(:base_folders) { [ full_folder_path ] } - let(:folder) { [folder_base,folder_name].join('/') } - let(:provider_name) { 'mock_provider' } - let(:whitelist) { nil } + let(:allowlist) { nil } let(:config) { YAML.load(<<-EOT --- :config: {} @@ -1179,9 +1121,7 @@ EOT :mock_provider: {} :pools: - name: '#{pool}' - folder: '#{folder}' size: 1 - datacenter: '#{datacenter}' provider: '#{provider_name}' EOT ) @@ -1194,20 +1134,18 @@ EOT $providers = { provider_name => provider } end - it 'should run purge_unconfigured_folders' do - expect(subject).to receive(:pool_folders).and_return(configured_folders) - expect(provider).to receive(:purge_unconfigured_folders).with(base_folders, configured_folders, whitelist) - expect(provider).to receive(:provider_config).and_return({}) + it 'should run purge_unconfigured_resources' do + expect(provider).to receive(:purge_unconfigured_resources).with(allowlist) + allow(provider).to receive(:provider_config).and_return({}) - subject.purge_vms_and_folders(provider_name) + subject.purge_vms_and_resources(provider_name) end it 'should raise any errors' do - expect(subject).to receive(:pool_folders).and_return(configured_folders) - expect(provider).to receive(:purge_unconfigured_folders).with(base_folders, configured_folders, whitelist).and_raise('mockerror') - expect(provider).to receive(:provider_config).and_return({}) + expect(provider).to receive(:purge_unconfigured_resources).with(allowlist).and_raise('mockerror') + allow(provider).to receive(:provider_config).and_return({}) - expect{ subject.purge_vms_and_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror') + expect{ subject.purge_vms_and_resources(provider_name) }.to raise_error(RuntimeError, 'mockerror') end end end diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index b05457c..f2a1975 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -104,36 +104,36 @@ EOT let(:other_folder) { 'folder2' } let(:base_folder) { 'dc1/vm/base' } let(:configured_folders) { { folder_title => base_folder } } - let(:whitelist) { nil } + let(:allowlist) { nil } it 'should return true when configured_folders includes the folder_title' do - expect(subject.folder_configured?(folder_title, base_folder, configured_folders, whitelist)).to be true + expect(subject.folder_configured?(folder_title, base_folder, configured_folders, allowlist)).to be true end it 'should return false when title is not in configured_folders' do - expect(subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist)).to be false + expect(subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist)).to be false end context 'with another base folder' do let(:base_folder) { 'dc2/vm/base' } let(:configured_folders) { { folder_title => 'dc1/vm/base' } } it 'should return false' do - expect(subject.folder_configured?(folder_title, base_folder, configured_folders, whitelist)).to be false + expect(subject.folder_configured?(folder_title, base_folder, configured_folders, allowlist)).to be false end end - context 'with a whitelist set' do - let(:whitelist) { [ other_folder ] } + context 'with a allowlist set' do + let(:allowlist) { [ other_folder ] } it 'should return true' do - expect(subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist)).to be true + expect(subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist)).to be true end end - context 'with string whitelist value' do - let(:whitelist) { 'whitelist' } + context 'with string allowlist value' do + let(:allowlist) { 'allowlist' } it 'should raise an error' do - expect(whitelist).to receive(:include?).and_raise('mockerror') + expect(allowlist).to receive(:include?).and_raise('mockerror') - expect{ subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist) }.to raise_error(RuntimeError, 'mockerror') + expect{ subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist) }.to raise_error(RuntimeError, 'mockerror') end end end @@ -297,12 +297,61 @@ EOT end end - describe '#purge_unconfigured_folders' do + describe '#pool_folders' do + let(:pool) { 'pool1' } + let(:folder_name) { 'myinstance' } + let(:folder_base) { 'vmpooler' } + let(:folder) { [folder_base,folder_name].join('/') } + let(:datacenter) { 'dc1' } + let(:provider_name) { 'mock_provider' } + let(:expected_response) { + { + folder_name => "#{datacenter}/vm/#{folder_base}" + } + } + context 'when evaluating pool folders' do + before do + expect(subject).not_to be_nil + #replace top-level global config + $config = YAML.load(<<-EOT +--- +:providers: + :mock: +:pools: + - name: '#{pool}' + folder: '#{folder}' + size: 1 + datacenter: '#{datacenter}' + provider: '#{provider_name}' + - name: '#{pool}2' + folder: '#{folder}' + size: 1 + datacenter: '#{datacenter}' + provider: '#{provider_name}2' + EOT + ) + end + + it 'should return a list of pool folders' do + expect(subject).to receive(:get_target_datacenter_from_config).with(pool).and_return(datacenter) + + expect(subject.pool_folders(provider_name)).to eq(expected_response) + end + + it 'should raise an error when the provider fails to get the datacenter' do + expect(subject).to receive(:get_target_datacenter_from_config).with(pool).and_raise('mockerror') + + expect{ subject.pool_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror') + end + end + end + + describe '#purge_unconfigured_resources' do let(:folder_title) { 'folder1' } let(:base_folder) { 'dc1/vm/base' } let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => base_folder }) } let(:child_folder) { mock_RbVmomi_VIM_Folder({ :name => folder_title }) } - let(:whitelist) { nil } + let(:allowlist) { nil } let(:base_folders) { [ base_folder ] } let(:configured_folders) { { folder_title => base_folder } } let(:folder_children) { [ folder_title => child_folder ] } @@ -310,6 +359,7 @@ EOT before(:each) do allow(subject).to receive(:connect_to_vsphere).and_return(connection) + allow(subject).to receive(:pool_folders).and_return(configured_folders) end context 'with an empty folder' do @@ -317,7 +367,7 @@ EOT expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(empty_list) expect(subject).to_not receive(:destroy_folder_and_children) - subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) + subject.purge_unconfigured_resources(allowlist) end end @@ -325,7 +375,7 @@ EOT expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(folder_children) allow(subject).to receive(:folder_configured?).and_return(true) - subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) + subject.purge_unconfigured_resources(allowlist) end context 'with a folder that is not configured' do @@ -337,14 +387,14 @@ EOT it 'should destroy the folder and children' do expect(subject).to receive(:destroy_folder_and_children).with(child_folder).and_return(nil) - subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) + subject.purge_unconfigured_resources(allowlist) end end it 'should raise any errors' do expect(subject).to receive(:get_folder_children).and_throw('mockerror') - expect{ subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) }.to raise_error(/mockerror/) + expect{ subject.purge_unconfigured_resources(allowlist) }.to raise_error(/mockerror/) end end diff --git a/vmpooler-provider-vsphere.gemspec b/vmpooler-provider-vsphere.gemspec index a0d332b..6c7febe 100644 --- a/vmpooler-provider-vsphere.gemspec +++ b/vmpooler-provider-vsphere.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |s| s.require_paths = ["lib"] s.add_dependency 'rbvmomi', '>= 2.1', '< 4.0' - s.add_development_dependency 'vmpooler', '~> 2.0' + s.add_development_dependency 'vmpooler', '~> 2.1' # renaming done in version 2.1 # Testing dependencies s.add_development_dependency 'climate_control', '>= 0.2.0'