From f6791baba0dca233801e59cb61f7d4572fecf7fd Mon Sep 17 00:00:00 2001 From: Samuel Beaulieu Date: Thu, 9 Dec 2021 11:33:10 -0600 Subject: [PATCH] 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 | 15 ++++++++--- spec/unit/providers/gce_spec.rb | 46 ++++++++++++++++++++++++++------- vmpooler.yaml.example | 26 ++++++++++++------- 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/lib/vmpooler/providers/gce.rb b/lib/vmpooler/providers/gce.rb index bac3b12..d86681d 100644 --- a/lib/vmpooler/providers/gce.rb +++ b/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 diff --git a/spec/unit/providers/gce_spec.rb b/spec/unit/providers/gce_spec.rb index 4bad5ef..888593d 100644 --- a/spec/unit/providers/gce_spec.rb +++ b/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/) diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index 8883601..772de78 100644 --- a/vmpooler.yaml.example +++ b/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: #