Merge pull request #13 from puppetlabs/maint-refactor-dns

(maint) Refactor cloud dns

.github/workflows/release.yml

@@ -20,10 +20,10 @@ jobs:
           draft: false
           prerelease: false
           generateReleaseNotes: true
-      - name: Install Ruby 2.5.8
+      - name: Install Ruby jruby-9.3.6.0
         uses: ruby/setup-ruby@v1
         with:
-          ruby-version: '2.5.8'
+          ruby-version: 'jruby-9.3.6.0'
       - name: Build gem
         run: gem build *.gemspec
       - name: Publish gem

.github/workflows/testing.yml

@@ -18,7 +18,7 @@ jobs:
     strategy:
       matrix:
         ruby-version:
-          - '2.5.8'
+          - 'jruby-9.3.6.0'
     steps:
     - uses: actions/checkout@v2
     - name: Set up Ruby
@@ -34,8 +34,7 @@ jobs:
     strategy:
       matrix:
         ruby-version:
-          - '2.5.8'
+          - 'jruby-9.3.6.0'
-          - 'jruby-9.2.12.0'
     steps:
     - uses: actions/checkout@v2
     - name: Set up Ruby

lib/vmpooler-provider-gce/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module VmpoolerProviderGce
-  VERSION = '0.3.0'
+  VERSION = '0.4.0'
 end

lib/vmpooler/cloud_dns.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'googleauth'
+require 'google/cloud/dns'
+
+module Vmpooler
+  class PoolManager
+    # This class interacts with GCP Cloud DNS to create or delete records.
+    class CloudDns
+      def initialize(project, dns_zone_resource_name)
+        @dns = Google::Cloud::Dns.new(project_id: project)
+        @dns_zone_resource_name = dns_zone_resource_name
+      end
+
+      def dns_create_or_replace(created_instance)
+        dns_zone = @dns.zone(@dns_zone_resource_name) if @dns_zone_resource_name
+        return unless dns_zone && created_instance && created_instance['name'] && created_instance['ip']
+
+        retries = 0
+        name = created_instance['name']
+        begin
+          change = dns_zone.add(name, 'A', 60, [created_instance['ip']])
+          debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address added") if change
+        rescue Google::Cloud::AlreadyExistsError => _e
+          # DNS setup is done only for new instances, so in the rare case where a DNS record already exists (it is stale) and we replace it.
+          # the error is Google::Cloud::AlreadyExistsError: alreadyExists: The resource 'entity.change.additions[0]' named 'instance-8.test.vmpooler.net. (A)' already exists
+          change = dns_zone.replace(name, 'A', 60, [created_instance['ip']])
+          debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address previously existed and was replaced") if change
+        rescue Google::Cloud::FailedPreconditionError => e
+          # this error was experienced intermittently, will retry to see if it can complete successfully
+          # the error is Google::Cloud::FailedPreconditionError: conditionNotMet: Precondition not met for 'entity.change.deletions[0]'
+          debug_logger("DNS create failed, retrying error: #{e}")
+          sleep 5
+          retry if (retries += 1) < 30
+        end
+      end
+
+      def dns_teardown(created_instance)
+        dns_zone = @dns.zone(@dns_zone_resource_name) if @dns_zone_resource_name
+        return unless dns_zone && created_instance
+
+        retries = 0
+        name = created_instance['name']
+        change = dns_zone.remove(name, 'A')
+        debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address removed") if change
+      rescue Google::Cloud::FailedPreconditionError => e
+        # this error was experienced intermittently, will retry to see if it can complete successfully
+        # the error is Google::Cloud::FailedPreconditionError: conditionNotMet: Precondition not met for 'entity.change.deletions[1]'
+        debug_logger("DNS teardown failed, retrying error: #{e}")
+        sleep 5
+        retry if (retries += 1) < 30
+      end
+
+      # used in local dev environment, set DEBUG_FLAG=true
+      # this way the upstream vmpooler manager does not get polluted with logs
+      def debug_logger(message, send_to_upstream: false)
+        # the default logger is simple and does not enforce debug levels (the first argument)
+        puts message if ENV['DEBUG_FLAG']
+        logger.log('[g]', message) if send_to_upstream
+      end
+    end
+  end
+end

lib/vmpooler/providers/gce.rb

@@ -2,7 +2,7 @@
 
 require 'googleauth'
 require 'google/apis/compute_v1'
-require 'google/cloud/dns'
+require 'vmpooler/cloud_dns'
 require 'bigdecimal'
 require 'bigdecimal/util'
 require 'vmpooler/providers/base'
@@ -58,11 +58,6 @@ module Vmpooler
           end
         end
 
-        def dns
-          @dns ||= Google::Cloud::Dns.new(project_id: project)
-          @dns
-        end
-
         # main configuration options
         def project
           provider_config['project']
@@ -566,28 +561,13 @@ module Vmpooler
         # END BASE METHODS
 
         def dns_setup(created_instance)
-          dns_zone = dns.zone(dns_zone_resource_name) if dns_zone_resource_name
+          dns = Vmpooler::PoolManager::CloudDns.new(project, dns_zone_resource_name)
-          return unless dns_zone && created_instance && created_instance['name'] && created_instance['ip']
+          dns.dns_create_or_replace(created_instance)
-
-          name = created_instance['name']
-          begin
-            change = dns_zone.add(name, 'A', 60, [created_instance['ip']])
-            debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address added") if change
-          rescue Google::Cloud::AlreadyExistsError => _e
-            # DNS setup is done only for new instances, so in the rare case where a DNS record already exists (it is stale) and we replace it.
-            # the error is Google::Cloud::AlreadyExistsError: alreadyExists: The resource 'entity.change.additions[0]' named 'instance-8.test.vmpooler.net. (A)' already exists
-            change = dns_zone.replace(name, 'A', 60, [created_instance['ip']])
-            debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address previously existed and was replaced") if change
-          end
         end
 
         def dns_teardown(created_instance)
-          dns_zone = dns.zone(dns_zone_resource_name) if dns_zone_resource_name
+          dns = Vmpooler::PoolManager::CloudDns.new(project, dns_zone_resource_name)
-          return unless dns_zone && created_instance
+          dns.dns_teardown(created_instance)
-
-          name = created_instance['name']
-          change = dns_zone.remove(name, 'A')
-          debug_logger("#{change.id} - #{change.started_at} - #{change.status} DNS address removed") if change
         end
 
         def should_be_ignored(item, allowlist)

spec/unit/providers/gce_spec.rb

@@ -62,7 +62,7 @@ EOT
   describe '#manual tests live' do
     context 'in itsysops' do
       before(:each) { allow(subject).to receive(:dns).and_call_original }
-      let(:vmname) { "instance-27" }
+      let(:vmname) { "instance-31" }
       let(:project) { 'vmpooler-test' }
       let(:config) { YAML.load(<<~EOT
       ---
@@ -75,8 +75,8 @@ EOT
           project: '#{project}'
           zone: '#{zone}'
           network_name: 'projects/itsysopsnetworking/global/networks/shared1'
-          dns_zone_resource_name: 'test-vmpooler-puppet-net'
+          dns_zone_resource_name: 'vmpooler-test-puppet-net'
-          domain: 'test.vmpooler.puppet.net'
+          domain: 'vmpooler-test.puppet.net'
       :pools:
         - name: '#{poolname}'
           alias: [ 'mockpool' ]
@@ -93,8 +93,8 @@ EOT
       skip 'gets a vm' do
         result = subject.create_vm(poolname, vmname)
         #result = subject.destroy_vm(poolname, vmname)
-        subject.get_vm(poolname, vmname)
+        # subject.get_vm(poolname, vmname)
-        #subject.dns_teardown({'name' => vmname})
+        subject.dns_teardown({'name' => vmname})
         # subject.dns_setup({'name' => vmname, 'ip' => '1.2.3.5'})
       end
     end
@@ -267,6 +267,7 @@ EOT
         result = MockResult.new
         result.status = 'DONE'
         allow(connection).to receive(:insert_instance).and_return(result)
+        allow(subject).to receive(:dns_setup).and_return(true)
       end
 
       it 'should return a hash' do
@@ -313,6 +314,7 @@ EOT
         result.status = 'DONE'
         allow(subject).to receive(:wait_for_operation).and_return(result)
         allow(connection).to receive(:delete_instance).and_return(result)
+        allow(subject).to receive(:dns_teardown).and_return(true)
       end
 
       it 'should return true' do
@@ -584,6 +586,7 @@ EOT
 
     before(:each) do
       allow(subject).to receive(:connect_to_gce).and_return(connection)
+      allow(subject).to receive(:dns_teardown).and_return(true)
     end
 
     context 'with empty allowlist' do