From fc7a62806360dcfc47880eeb5bff3cfbf9e583bd Mon Sep 17 00:00:00 2001 From: Samuel Beaulieu Date: Wed, 8 Dec 2021 10:33:25 -0600 Subject: [PATCH] spec the purge method and align it with the new base method signature --- lib/vmpooler/providers/gce.rb | 41 ++++--- spec/computeservice_helper.rb | 6 ++ spec/unit/providers/gce_spec.rb | 185 ++++++++++++++++++++++++-------- vmpooler.yaml.example | 6 +- 4 files changed, 176 insertions(+), 62 deletions(-) diff --git a/lib/vmpooler/providers/gce.rb b/lib/vmpooler/providers/gce.rb index 8741ac0..bac3b12 100644 --- a/lib/vmpooler/providers/gce.rb +++ b/lib/vmpooler/providers/gce.rb @@ -435,49 +435,51 @@ module Vmpooler # Scans zones that are configured for list of resources (VM, disks, snapshots) that do not have the label.pool set # to one of the configured pools. If it is also not in the allowlist, the resource is destroyed - def purge_unconfigured_folders(base_folders, configured_folders, whitelist) + def purge_unconfigured_resources(allowlist) @connection_pool.with_metrics do |pool_object| connection = ensured_gce_connection(pool_object) pools_array = provided_pools filter = {} + # we have to group things by zone, because the API search feature is done against a zone and not global + # so we will do the searches in each configured zone pools_array.each do |pool| filter[zone(pool)] = [] if filter[zone(pool)].nil? - filter[zone(pool)] << "(labels.pool != #{pool} OR -labels.pool:*)" + filter[zone(pool)] << "(labels.pool != #{pool})" end filter.keys.each do |zone| + # this filter should return any item that have a labels.pool that is not in the config OR + # do not have a pool label at all + filter_string = filter[zone].join(" AND ") + " OR -labels.pool:*" #VMs - instance_list = connection.list_instances(project, zone, filter: filter[zone].join(" AND ")) + instance_list = connection.list_instances(project, zone, filter: filter_string) result_list = [] unless instance_list.items.nil? instance_list.items.each do |vm| - next if !vm.labels.nil? && whitelist&.include?(vm.labels['pool']) - next if whitelist&.include?("") && vm.labels.nil? + next if should_be_ignored(vm, allowlist) result = connection.delete_instance(project, zone, vm.name) result_list << result end end #now check they are done result_list.each do |result| - wait_for_operation(project, pool_name, result, connection) + wait_for_zone_operation(project, zone, result, connection) end #Disks - disks_list = connection.list_disks(project, zone, filter: filter[zone].join(" AND ")) + disks_list = connection.list_disks(project, zone, filter: filter_string) unless disks_list.items.nil? disks_list.items.each do |disk| - next if !disk.labels.nil? && whitelist&.include?(disk.labels['pool']) - next if whitelist&.include?("") && disk.labels.nil? + next if should_be_ignored(disk, allowlist) result = connection.delete_disk(project, zone, disk.name) end end #Snapshots - snapshot_list = connection.list_snapshots(project, filter: filter[zone].join(" AND ")) + snapshot_list = connection.list_snapshots(project, filter: filter_string) unless snapshot_list.items.nil? snapshot_list.items.each do |sn| - next if !sn.labels.nil? && whitelist&.include?(sn.labels['pool']) - next if whitelist&.include?("") && sn.labels.nil? + next if should_be_ignored(sn, allowlist) result = connection.delete_snapshot(project, sn.name) end end @@ -485,12 +487,17 @@ module Vmpooler end end + def should_be_ignored(item, allowlist) + (!item.labels.nil? && allowlist&.include?(item.labels['pool'])) || + (allowlist&.include?("") && !item.labels&.keys&.include?('pool')) + end + # END BASE METHODS # Compute resource wait for operation to be DONE (synchronous operation) - def wait_for_operation(project, pool_name, result, connection, retries=5) + def wait_for_zone_operation(project, zone, result, connection, retries=5) while result.status != 'DONE' - result = connection.wait_zone_operation(project, zone(pool_name), result.name) + result = connection.wait_zone_operation(project, zone, result.name) end if result.error # unsure what kind of error can be stored here error_message = "" @@ -498,7 +505,7 @@ module Vmpooler result.error.each do |error| error_message = "#{error_message} #{error.code}:#{error.message}" end - raise "Pool #{pool_name} operation: #{result.description} failed with error: #{error_message}" + raise "Operation: #{result.description} failed with error: #{error_message}" end result rescue Google::Apis::TransmissionError => e @@ -515,6 +522,10 @@ module Vmpooler puts "waited on #{result.name} but was not found, so skipping" end + def wait_for_operation(project, pool_name, result, connection, retries=5) + wait_for_zone_operation(project, zone(pool_name), result, connection, retries) + end + # Return a hash of VM data # Provides vmname, hostname, template, poolname, boottime, status, zone, machine_type information def generate_vm_hash(vm_object, pool_name) diff --git a/spec/computeservice_helper.rb b/spec/computeservice_helper.rb index 09c5c37..a21363c 100644 --- a/spec/computeservice_helper.rb +++ b/spec/computeservice_helper.rb @@ -48,6 +48,12 @@ MockDisk = Struct.new( keyword_init: true ) +MockSnapshotList = Struct.new( + #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/DiskList.html + :id, :items, :kind, :next_page_token, :self_link, :warning, + keyword_init: true +) + MockSnapshot = Struct.new( #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Snapshot.html :auto_created, :chain_name, :creation_timestamp, :description, :disk_size_gb, :download_bytes, :id, :kind, diff --git a/spec/unit/providers/gce_spec.rb b/spec/unit/providers/gce_spec.rb index 27ebe96..4bad5ef 100644 --- a/spec/unit/providers/gce_spec.rb +++ b/spec/unit/providers/gce_spec.rb @@ -59,15 +59,18 @@ EOT describe '#manual tests live' do skip 'runs in gce' do +=begin puts "creating" result = subject.create_vm(poolname, vmname) + puts "create snapshot w/ one disk" + result = subject.create_snapshot(poolname, vmname, "sams") puts "create disk" result = subject.create_disk(poolname, vmname, 10) - puts "create snapshot" - result = subject.create_snapshot(poolname, vmname, "sams") + puts "create snapshot w/ 2 disks" result = subject.create_snapshot(poolname, vmname, "sams2") +=end puts "revert snapshot" - result = subject.revert_snapshot(poolname, vmname, "sams2") + result = subject.revert_snapshot(poolname, vmname, "sams") #result = subject.destroy_vm(poolname, vmname) end @@ -80,7 +83,7 @@ EOT skip 'debug' do - puts subject.purge_unconfigured_folders(nil, nil, ['foo', '', 'blah']) + puts subject.purge_unconfigured_resources(['foo', '', 'blah']) end end @@ -242,7 +245,7 @@ EOT end it 'should raise an error' do - expect{ subject.create_vm(poolname, vmname) }.to raise_error(Google::Apis::ClientError, /The resource .+ was not found/) + expect{ subject.create_vm(poolname, vmname) }.to raise_error(Google::Apis::ClientError) end end @@ -280,9 +283,13 @@ EOT context 'Given a missing VM name' do before(:each) do allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found")) + disk_list = MockDiskList.new(items: nil) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(subject).to receive(:find_all_snapshots).and_return(nil) end it 'should return true' do + expect(connection.should_receive(:delete_instance).never) expect(subject.destroy_vm(poolname, 'missing_vm')).to be true end end @@ -541,13 +548,12 @@ EOT attached_disk = MockAttachedDisk.new(device_name: vmname, source: "foo/bar/baz/#{vmname}") instance = MockInstance.new(name: vmname, disks: [attached_disk]) allow(connection).to receive(:get_instance).and_return(instance) - snapshots = [MockSnapshot.new(name: snapshot_name, self_link: "foo/bar/baz/snapshot/#{snapshot_name}")] + snapshots = [MockSnapshot.new(name: snapshot_name, self_link: "foo/bar/baz/snapshot/#{snapshot_name}", labels: {"diskname" => vmname})] allow(subject).to receive(:find_snapshot).and_return(snapshots) allow(connection).to receive(:stop_instance) allow(subject).to receive(:wait_for_operation) allow(connection).to receive(:detach_disk) allow(connection).to receive(:delete_disk) - allow(connection).to receive(:get_snapshot).and_return(snapshots[0]) new_disk = MockDisk.new(name: vmname, self_link: "foo/bar/baz/disk/#{vmname}") allow(connection).to receive(:insert_disk) allow(connection).to receive(:get_disk).and_return(new_disk) @@ -561,55 +567,146 @@ EOT end end - #TODO: below are todo - describe '#purge_unconfigured_folders' 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(:base_folders) { [ base_folder ] } - let(:configured_folders) { { folder_title => base_folder } } - let(:folder_children) { [ folder_title => child_folder ] } + describe '#purge_unconfigured_resources' do let(:empty_list) { [] } before(:each) do allow(subject).to receive(:connect_to_gce).and_return(connection) end - context 'with an empty folder' do - skip 'should not attempt to destroy any folders' do - 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) - end - end - - skip 'should retrieve the folder children' do - 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) - end - - context 'with a folder that is not configured' do + context 'with empty allowlist' do before(:each) do - expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(folder_children) - allow(subject).to receive(:folder_configured?).and_return(false) + allow(subject).to receive(:wait_for_zone_operation) end - - skip '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) + it 'should attempt to delete unconfigured instances when they dont have a label' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo")]) + disk_list = MockDiskList.new(items: nil) + snapshot_list = MockSnapshotList.new(items: nil) + # the instance_list is filtered in the real code, and should only return non-configured VMs based on labels + # that do not match a real pool name + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).to receive(:delete_instance) + subject.purge_unconfigured_resources(nil) + end + it 'should attempt to delete unconfigured instances when they have a label that is not a configured pool' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "foobar"})]) + disk_list = MockDiskList.new(items: nil) + snapshot_list = MockSnapshotList.new(items: nil) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).to receive(:delete_instance) + subject.purge_unconfigured_resources(nil) + end + it 'should attempt to delete unconfigured disks and snapshots when they do not have a label' do + instance_list = MockInstanceList.new(items: nil) + disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo")]) + snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")]) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).to receive(:delete_disk) + expect(connection).to receive(:delete_snapshot) + subject.purge_unconfigured_resources(nil) end end - skip 'should raise any errors' do - expect(subject).to receive(:get_folder_children).and_throw('mockerror') + context 'with allowlist containing a pool name' do + before(:each) do + allow(subject).to receive(:wait_for_zone_operation) + $allowlist = ["allowed"] + end + it 'should attempt to delete unconfigured instances when they dont have the allowlist label' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "not_this"})]) + disk_list = MockDiskList.new(items: nil) + snapshot_list = MockSnapshotList.new(items: nil) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).to receive(:delete_instance) + subject.purge_unconfigured_resources($allowlist) + end + it 'should ignore unconfigured instances when they have a label that is allowed' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "allowed"})]) + disk_list = MockDiskList.new(items: nil) + snapshot_list = MockSnapshotList.new(items: nil) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).not_to receive(:delete_instance) + subject.purge_unconfigured_resources($allowlist) + end + it 'should ignore unconfigured disks and snapshots when they have a label that is allowed' do + instance_list = MockInstanceList.new(items: nil) + disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"pool" => "allowed"})]) + snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"pool" => "allowed"})]) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).not_to receive(:delete_disk) + expect(connection).not_to receive(:delete_snapshot) + subject.purge_unconfigured_resources($allowlist) + end + it 'should ignore unconfigured item when they have the empty label that is allowed, which means we allow the pool label to not be set' do + $allowlist = ["allowed", ""] + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important"})]) + disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing"})]) + snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")]) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).not_to receive(:delete_instance) + expect(connection).not_to receive(:delete_disk) + expect(connection).not_to receive(:delete_snapshot) + subject.purge_unconfigured_resources($allowlist) + end + end - expect{ subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) }.to raise_error(/mockerror/) + context 'with allowlist containing a pool name' do + before(:each) do + allow(subject).to receive(:wait_for_zone_operation) + $allowlist = ["allowed", ""] + end + it 'should attempt to delete unconfigured instances when they dont have the allowlist label' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "not_this"})]) + disk_list = MockDiskList.new(items: nil) + snapshot_list = MockSnapshotList.new(items: nil) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).to receive(:delete_instance) + subject.purge_unconfigured_resources($allowlist) + end + it 'should ignore unconfigured disks and snapshots when they have a label that is allowed' do + instance_list = MockInstanceList.new(items: nil) + disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"pool" => "allowed"})]) + snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"pool" => "allowed"})]) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).not_to receive(:delete_disk) + expect(connection).not_to receive(:delete_snapshot) + subject.purge_unconfigured_resources($allowlist) + end + it 'should ignore unconfigured item when they have the empty label that is allowed, which means we allow the pool label to not be set' do + instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important"})]) + disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing"})]) + snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")]) + allow(connection).to receive(:list_instances).and_return(instance_list) + allow(connection).to receive(:list_disks).and_return(disk_list) + allow(connection).to receive(:list_snapshots).and_return(snapshot_list) + expect(connection).not_to receive(:delete_instance) + expect(connection).not_to receive(:delete_disk) + expect(connection).not_to receive(:delete_snapshot) + subject.purge_unconfigured_resources($allowlist) + end + end + + it 'should raise any errors' do + expect(subject).to receive(:provided_pools).and_throw('mockerror') + expect{ subject.purge_unconfigured_resources(nil) }.to raise_error(/mockerror/) end end diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index 0f7cbb5..8883601 100644 --- a/vmpooler.yaml.example +++ b/vmpooler.yaml.example @@ -14,8 +14,8 @@ # # - purge_unconfigured_resources # Enable purging of VMs detected -# For GCE: by default any VM in the project but without a "pool" label, or a "pool" label with the value for an unconfigured pool -# An optional allowlist can be provided to ignore purging certain folders or pool labels +# By default will purge VMs in the project without a "pool" label, or a "pool" label with the value for an unconfigured pool +# An optional allowlist can be provided to ignore purging certain VMs based on pool labels # # Setting this on the provider will enable purging for the provider # Expects a boolean value @@ -25,7 +25,7 @@ # For GCE: Specify pool labels that should be ignored when purging VMs, for pools that are not configured. For example if the label is # set to 'pool=donotdelete' and there is no pool with that name configured, adding "donotdelete" to the allowlist would not purge the VM. # adding "" (empty string) to the allowlist has a special meaning whereas VMs that do not have the "pool" label are also not purged. -# This option is only evaluated when 'purge_unconfigured_folders' is enabled +# This option is only evaluated when 'purge_unconfigured_resources' is enabled # Expects an array of strings specifying the allowlisted labels by name # (optional; default: nil) #