spec the purge method and align it with the new base method signature

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 should_be_ignored(vm, allowlist)
-                  next if whitelist&.include?("") && vm.labels.nil?
                   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 should_be_ignored(disk, allowlist)
-                  next if whitelist&.include?("") && disk.labels.nil?
                   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 should_be_ignored(sn, allowlist)
-                  next if whitelist&.include?("") && sn.labels.nil?
                   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)

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,

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"
+      puts "create snapshot w/ 2 disks"
-      result = subject.create_snapshot(poolname, vmname, "sams")
       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_resources' do
-  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 ] }
     let(:empty_list) { [] }
 
     before(:each) do
       allow(subject).to receive(:connect_to_gce).and_return(connection)
     end
 
-    context 'with an empty folder' do
+    context 'with empty allowlist' 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
       before(:each) do
-        expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(folder_children)
+        allow(subject).to receive(:wait_for_zone_operation)
-        allow(subject).to receive(:folder_configured?).and_return(false)
       end
-
+      it 'should attempt to delete unconfigured instances when they dont have a label' do
-      skip 'should destroy the folder and children' do
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo")])
-        expect(subject).to receive(:destroy_folder_and_children).with(child_folder).and_return(nil)
+        disk_list = MockDiskList.new(items: nil)
-
+        snapshot_list = MockSnapshotList.new(items: nil)
-        subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist)
+        # 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
+    context 'with allowlist containing a pool name' do
-      expect(subject).to receive(:get_folder_children).and_throw('mockerror')
+      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
   

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
+#     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 folders or pool labels
+#     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)
 #