(POOLER-166) Check for stale dns records

lib/vmpooler/pool_manager.rb

@@ -2,6 +2,7 @@
 
 require 'vmpooler/providers'
 require 'spicy-proton'
+require 'resolv' # ruby standard lib
 
 module Vmpooler
   class PoolManager
@@ -295,22 +296,46 @@ def generate_and_check_hostname(_pool_name)
     end
 
     def find_unique_hostname(pool_name)
+      # generate hostname that is not already in use in vmpooler
+      # also check that no dns record already exists
       hostname_retries = 0
       max_hostname_retries = 3
       while hostname_retries < max_hostname_retries
         hostname, available = generate_and_check_hostname(pool_name)
-        break if available
+        domain = $config[:config]['domain']
+        dns_ip, dns_available = check_dns_available(hostname, domain)
+        break if available && dns_available
 
         hostname_retries += 1
-        $metrics.increment("errors.duplicatehostname.#{pool_name}")
-        $logger.log('s', "[!] [#{pool_name}] Generated hostname #{hostname} was not unique (attempt \##{hostname_retries} of #{max_hostname_retries})")
+
+        if !available
+          $metrics.increment("errors.duplicatehostname.#{pool_name}")
+          $logger.log('s', "[!] [#{pool_name}] Generated hostname #{hostname} was not unique (attempt \##{hostname_retries} of #{max_hostname_retries})")
+        elsif !dns_available
+          $metrics.increment("errors.staledns.#{hostname}")
+          $logger.log('s', "[!] [#{pool_name}] Generated hostname #{hostname} already exists in DNS records (#{dns_ip}), stale DNS")
+        end
       end
 
-      raise "Unable to generate a unique hostname after #{hostname_retries} attempts. The last hostname checked was #{hostname}" unless available
+      raise "Unable to generate a unique hostname after #{hostname_retries} attempts. The last hostname checked was #{hostname}" unless available && dns_available
 
       hostname
     end
 
+    def check_dns_available(vm_name, domain = nil)
+      # Query the DNS for the name we want to create and if it already exists, mark it unavailable
+      # This protects against stale DNS records
+      vm_name = "#{vm_name}.#{domain}" if domain
+      begin
+        dns_ip = Resolv.getaddress(vm_name)
+      rescue Resolv::ResolvError
+        # this is the expected case, swallow the error
+        # eg "no address for blah-daisy"
+        return ['', true]
+      end
+      [dns_ip, false]
+    end
+
     def _clone_vm(pool_name, provider)
       new_vmname = find_unique_hostname(pool_name)
 

spec/unit/pool_manager_spec.rb

@@ -681,6 +681,28 @@
         expect{subject._clone_vm(pool,provider)}.to raise_error(/MockError/)
       end
     end
+
+    context 'with #check_dns_available' do
+      before(:each) do
+        allow(logger).to receive(:log)
+      end
+      it 'should error out if DNS already exists' do
+        vm_name = "foo"
+        resolv = class_double("Resolv").as_stubbed_const(:transfer_nested_constants => true)
+        expect(subject).to receive(:generate_and_check_hostname).exactly(3).times.and_return([vm_name, true]) #skip this, make it available all times
+        expect(resolv).to receive(:getaddress).exactly(3).times.and_return("1.2.3.4")
+        expect(metrics).to receive(:increment).with("errors.staledns.#{vm_name}").exactly(3).times
+        expect{subject._clone_vm(pool,provider)}.to raise_error(/Unable to generate a unique hostname after/)
+      end
+      it 'should be successful if DNS does not exist' do
+        vm_name = "foo"
+        resolv = class_double("Resolv").as_stubbed_const(:transfer_nested_constants => true)
+        expect(subject).to receive(:generate_and_check_hostname).and_return([vm_name, true])
+        expect(resolv).to receive(:getaddress).exactly(1).times.and_raise(Resolv::ResolvError)
+        expect(provider).to receive(:create_vm).with(pool, String)
+        subject._clone_vm(pool,provider)
+      end
+    end
   end
 
   describe '#destroy_vm' do
@@ -2747,7 +2769,7 @@
       let(:loop_delay) { 1 }
       # Note a maxloop of zero can not be tested as it never terminates
       before(:each) do
-  
+
         allow(subject).to receive(:check_disk_queue)
         allow(subject).to receive(:check_snapshot_queue)
         allow(subject).to receive(:check_pool)
@@ -3639,7 +3661,7 @@
         # Modify the pool size to 1 and add a VM in the queue
         redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
         pool_size = 1
-  
+
         subject.repopulate_pool_vms(pool, provider, pool_check_response, pool_size)
       end
     end