Adding support for fully qualified allow list eg user=bob

before thhis change the allow list would only support checking the 'pool' label value
we can now specify a different label name by using the format labename=value where
the equal sign '=' is considered the separator

lib/vmpooler/providers/gce.rb

@@ -293,6 +293,7 @@ module Vmpooler
         # 1. shutting down the VM,
         # 2. detaching and deleting the drives,
         # 3. creating new disks with the same name from the snapshot for each disk
+        # 4. attach disks and start instance
         # for one vm, there might be multiple snapshots in time. We select the ones referred to by the
         # snapshot_name, but that may be multiple snapshots, one for each disks
         # The new disk is added labels vm and pool
@@ -425,11 +426,11 @@ module Vmpooler
 
         def vm_ready?(_pool_name, vm_name)
           begin
+            #TODO: we could use a healthcheck resource attached to instance
             open_socket(vm_name, global_config[:config]['domain'])
           rescue StandardError => _e
             return false
           end
-
           true
         end
 
@@ -488,8 +489,16 @@ module Vmpooler
         end
 
         def should_be_ignored(item, allowlist)
-          (!item.labels.nil? && allowlist&.include?(item.labels['pool'])) ||
-            (allowlist&.include?("") && !item.labels&.keys&.include?('pool'))
+          allowlist.map!(&:downcase) # remove uppercase from configured values because its not valid as resource label
+          array_flattened_labels = []
+          unless item.labels.nil?
+            item.labels.each do |k,v|
+              array_flattened_labels << "#{k}=#{v}"
+            end
+          end
+          (!item.labels.nil? && allowlist&.include?(item.labels['pool'])) || # the allow list specifies the value within the pool label
+            (allowlist&.include?("") && !item.labels&.keys&.include?('pool')) || # the allow list specifies "" string and the pool label is not set
+            !(allowlist & array_flattened_labels).empty? # the allow list specify a fully qualified label eg user=Bob and the item has it
         end
 
         # END BASE METHODS

spec/unit/providers/gce_spec.rb

@@ -59,7 +59,6 @@ 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"
@@ -68,7 +67,6 @@ EOT
       result = subject.create_disk(poolname, vmname, 10)
       puts "create snapshot w/ 2 disks"
       result = subject.create_snapshot(poolname, vmname, "sams2")
-=end
       puts "revert snapshot"
       result = subject.revert_snapshot(poolname, vmname, "sams")
       #result = subject.destroy_vm(poolname, vmname)
@@ -82,7 +80,6 @@ EOT
     end
 
     skip 'debug' do
-
       puts subject.purge_unconfigured_resources(['foo', '', 'blah'])
     end
   end
@@ -514,15 +511,17 @@ EOT
     end
 
     context 'when instance does not have attached disks' do
-      it 'should raise an error' do
-        disk = nil
-        instance = MockInstance.new(name: vmname, disks: [disk])
+      it 'should skip detaching/deleting disk' do
+        instance = MockInstance.new(name: vmname, disks: nil)
         allow(connection).to receive(:get_instance).and_return(instance)
-        snapshots = [MockSnapshot.new(name: snapshot_name)]
+        snapshots = []
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
         allow(connection).to receive(:stop_instance)
         allow(subject).to receive(:wait_for_operation)
-        expect{ subject.revert_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/No disk is currently attached to VM #{vmname} in pool #{poolname}, cannot revert snapshot/)
+        allow(connection).to receive(:start_instance)
+        expect(subject).not_to receive(:detach_disk)
+        expect(subject).not_to receive(:delete_disk)
+        subject.revert_snapshot(poolname,vmname,snapshot_name)
       end
     end
 
@@ -664,7 +663,7 @@ EOT
       end
     end
 
-    context 'with allowlist containing a pool name' do
+    context 'with allowlist containing a pool name and the empty string' do
       before(:each) do
         allow(subject).to receive(:wait_for_zone_operation)
         $allowlist = ["allowed", ""]
@@ -704,6 +703,35 @@ EOT
       end
     end
 
+    context 'with allowlist containing a a fully qualified label that is not pool' do
+      before(:each) do
+        allow(subject).to receive(:wait_for_zone_operation)
+        $allowlist = ["user=Bob"]
+      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 item when they match the fully qualified label' do
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important", "user" => "bob"})])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing", "user" => "bob"})])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"user" => "bob"})])
+        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/)

vmpooler.yaml.example

@@ -13,23 +13,27 @@
 #     (optional: will default to it's parent :key: name eg. 'gce')
 #
 #   - purge_unconfigured_resources
-#     Enable purging of VMs detected
-#     By default will purge VMs in the project without a "pool" label, or a "pool" label with the value for an unconfigured pool
+#     Enable purging of VMs, disks and snapshots
+#     By default will purge resources 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
 #     (optional; default: false)
 #
-#   - recources_allowlist
-#     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.
+#   - resources_allowlist
+#     For GCE: Specify labels that should be ignored when purging VMs. For example if a VM's label is
+#     set to 'pool' with value 'donotdelete' and there is no pool with that name configured, it would normally be purged,
+#     unless you add a resources_allowlist "donotdelete" in which case it is ignored and not purged.
+#     Additionally the "" (empty string) has a special meaning whereas VMs that do not have the "pool" label are not purged.
+#     Additionally if you want to ignore VM's with an arbitrary label, include it in the allow list as a string with the separator "="
+#     between the label name and value eg user=bob would ignore VMs that include the label "user" with the value "bob"
+#     If any one of the above condition is met, the resource is ignored and not purged
 #     This option is only evaluated when 'purge_unconfigured_resources' is enabled
-#     Expects an array of strings specifying the allowlisted labels by name
+#     Expects an array of strings specifying the allowlisted labels by name. The strings should be all lower case, since
+#     no uppercase char is allowed in a label
 #     (optional; default: nil)
 #
-# If you want to support more than one provider with different parameters (server, username or passwords) you have to specify the
+# If you want to support more than one provider with different parameters you have to specify the
 # backing service in the provider_class configuration parameter for example 'vsphere' or 'dummy'. Each pool can specify
 # the provider to use.
 #
@@ -43,6 +47,10 @@
     provider_class: 'gce'
     project: 'myproject-foo'
     zone: 'us-central1-f'
+    resources_allowlist:
+      - "user=bob"
+      - ""
+      - "custom-pool"
 
 # :gce:
 #