From 107cd22226bac088b282905a0deee0bd7d1657d6 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Tue, 28 Aug 2018 15:23:29 -0400
Subject: [PATCH 1/9] Save the current list of migrations ran in the region
 record on create

This will allow the global region to track which migrations have
been run in each remote region.

https://bugzilla.redhat.com/show_bug.cgi?id=1601015
---
 app/models/miq_region.rb       | 6 +++++-
 spec/models/miq_region_spec.rb | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/app/models/miq_region.rb b/app/models/miq_region.rb
index 29d97802e7a..aac2d6d8d4b 100644
--- a/app/models/miq_region.rb
+++ b/app/models/miq_region.rb
@@ -113,8 +113,12 @@ def self.seed
             _("Region [%{region_id}] does not match the database's region [%{db_id}]") % {:region_id => my_region_id,
                                                                                           :db_id     => db_region_id}
     end
+    create_params = {
+      :description    => "Region #{my_region_id}",
+      :migrations_ran => ActiveRecord::SchemaMigration.normalized_versions
+    }
 
-    create_with(:description => "Region #{my_region_id}").find_or_create_by!(:region => my_region_id) do
+    create_with(create_params).find_or_create_by!(:region => my_region_id) do
       _log.info("Creating Region [#{my_region_id}]")
     end
   end
diff --git a/spec/models/miq_region_spec.rb b/spec/models/miq_region_spec.rb
index f1b8d9266cc..324620be671 100644
--- a/spec/models/miq_region_spec.rb
+++ b/spec/models/miq_region_spec.rb
@@ -67,6 +67,10 @@
       allow(MiqRegion).to receive_messages(:my_region_number => @region_number + 1)
       expect { MiqRegion.seed }.to raise_error(Exception)
     end
+
+    it "sets the migrations_ran column" do
+      expect(MiqRegion.first.migrations_ran).to match_array(ActiveRecord::SchemaMigration.normalized_versions)
+    end
   end
 
   describe ".replication_type" do

From 2642c290db4fa40562e185581a697634cd9522d2 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Tue, 28 Aug 2018 15:25:01 -0400
Subject: [PATCH 2/9] Patch ActiveRecord::Migration#migrate to ensure global
 region schema consistency

This patch does two things.

1. It maintains the migration list in MiqRegion#migrations_ran as each
   migration is completed
2. It halts the global region migration process if the changes from
   the same migration in all the remote regions have not been replicated

This combination will allow us to ensure that the global region will
never attempt to replicate data which would be inconsistent with the
current schema.

This patch uses SQL directly to do the update because the migration
which adds the migrations_ran column was not able to utilize the attribute
for the newly added column.

We were not able to refresh the attributes cache of the MiqRegion
model from within the #migrate method for some reason, but were
able to detect if the column was present and update it while bypassing
the AR caches.

This seemed to be the more sane route given the issues we've had with
model cache clearing in the past.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1601015
---
 lib/extensions/ar_migration.rb           | 45 ++++++++++++
 spec/lib/extensions/ar_migration_spec.rb | 89 ++++++++++++++++++++++++
 2 files changed, 134 insertions(+)
 create mode 100644 lib/extensions/ar_migration.rb
 create mode 100644 spec/lib/extensions/ar_migration_spec.rb

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
new file mode 100644
index 00000000000..5320c39601a
--- /dev/null
+++ b/lib/extensions/ar_migration.rb
@@ -0,0 +1,45 @@
+module ArPglogicalMigration
+  module ArPglogicalMigrationHelper
+    def self.migrations_column_present?
+      ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
+    end
+
+    def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
+      return unless ArPglogicalMigrationHelper.migrations_column_present?
+      region = MiqRegion.find_by(:region => subscription.provider_region)
+      until region.migrations_ran&.include?(version)
+        subscription.disable
+        subscription.enable
+        sleep(wait_time)
+        region.reload
+      end
+    end
+
+    def self.update_local_migrations_ran(version, direction)
+      return unless ArPglogicalMigrationHelper.migrations_column_present?
+      return unless (region = MiqRegion.my_region)
+
+      new_migrations = ActiveRecord::SchemaMigration.normalized_versions
+      new_migrations << version if direction == :up
+      migrations_value = ActiveRecord::Base.connection.quote(PG::TextEncoder::Array.new.encode(new_migrations))
+
+      ActiveRecord::Base.connection.exec_query(<<~SQL)
+        UPDATE miq_regions
+        SET migrations_ran = #{migrations_value}
+        WHERE id = #{region.id}
+      SQL
+    end
+  end
+
+  def migrate(direction)
+    PglogicalSubscription.all.each do |s|
+      ArPglogicalMigrationHelper.wait_for_remote_region_migration(s, version.to_s)
+    end
+
+    ret = super
+    ArPglogicalMigrationHelper.update_local_migrations_ran(version.to_s, direction)
+    ret
+  end
+end
+
+ActiveRecord::Migration.prepend(ArPglogicalMigration)
diff --git a/spec/lib/extensions/ar_migration_spec.rb b/spec/lib/extensions/ar_migration_spec.rb
new file mode 100644
index 00000000000..b3f2de0e947
--- /dev/null
+++ b/spec/lib/extensions/ar_migration_spec.rb
@@ -0,0 +1,89 @@
+describe ArPglogicalMigration::ArPglogicalMigrationHelper do
+  let!(:my_region) do
+    MiqRegion.seed
+    MiqRegion.my_region
+  end
+
+  context "without the migrations ran column" do
+    before do
+      column_list = %w(id region created_at updated_at description guid).map { |n| double(:name => n) }
+      allow(ActiveRecord::Base.connection).to receive(:columns).with("miq_regions").and_return(column_list)
+    end
+
+    describe ".migrations_column_present?" do
+      it "is falsey" do
+        expect(described_class.migrations_column_present?).to be_falsey
+      end
+    end
+
+    describe ".wait_for_remote_region_migrations" do
+      it "does nothing" do
+        expect(MiqRegion).not_to receive(:find)
+        described_class.wait_for_remote_region_migration(double("subscription"), "12345")
+      end
+    end
+
+    describe ".update_local_migrations_ran" do
+      it "does nothing" do
+        expect(MiqRegion).not_to receive(:my_region)
+        described_class.update_local_migrations_ran("12345", :up)
+      end
+    end
+  end
+
+  describe ".migrations_column_present?" do
+    it "is truthy" do
+      # we never want to remove this column so we can just test directly
+      expect(described_class.migrations_column_present?).to be_truthy
+    end
+  end
+
+  context "with a dummy version" do
+    let(:version) { "1234567890" }
+
+    # sanity check - if this is somehow a version we have, these tests will make no sense
+    before { expect(my_region.migrations_ran).not_to include(version) }
+
+    describe ".wait_for_remote_region_migration" do
+      let(:subscription) { double("Subscription", :enable => nil, :disable => nil, :provider_region => my_region.region) }
+
+      it "sleeps until the migration is added" do
+        my_region.update_attributes!(:migrations_ran => nil)
+        t = Thread.new do
+          Thread.current.abort_on_exception = true
+          # need to stub these because the thread uses a separate connection object which won't be in the same transaction
+          allow(MiqRegion).to receive(:find_by).with(:region => my_region.region).and_return(my_region)
+          allow(my_region).to receive(:reload)
+          described_class.wait_for_remote_region_migration(subscription, version, 0)
+        end
+
+        # Try to pass execution to the created thread
+        # NOTE: This is could definitely be a source of weird spec timing issues because
+        #       we're relying on the thread scheduler to pass to the next thread
+        #       when we sleep, but if this isn't here we likely won't execute the conditional
+        #       block in .wait_for_remote_region_migrations
+        sleep 1
+
+        expect(t.alive?).to be true
+        my_region.update_attributes!(:migrations_ran => ActiveRecord::SchemaMigration.normalized_versions << version)
+
+        # Wait a max of 5 seconds so we don't disrupt the whole test suite if something terrible happens
+        t = t.join(5)
+        expect(t.status).to be false
+      end
+    end
+
+    describe ".update_local_migrations_ran" do
+      it "adds the given version when the direction is :up" do
+        described_class.update_local_migrations_ran(version, :up)
+        expect(my_region.reload.migrations_ran).to match_array(ActiveRecord::SchemaMigration.normalized_versions << version)
+      end
+
+      it "doesn't blow up when there is no region" do
+        MiqRegion.destroy_all
+        MiqRegion.my_region_clear_cache
+        described_class.update_local_migrations_ran(version, :up)
+      end
+    end
+  end
+end

From e37c8de5fb0acaf0e15283b56dee7b98cfe34a3b Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Wed, 19 Sep 2018 16:01:51 -0400
Subject: [PATCH 3/9] Use a separate connection to bounce the subscription

This fixes a "deadlock" in the migration process.

In the migration transaction we take a lock on the
pglogical.local_node relation by trying to disable the subscription.
The process that is responsible for restarting the subscription also
requires a lock on that relation.

The migration will not commit its transaction (and release its locks)
until the data is replicated up from the remote region, but the data
cannot be replicated until the subscription apply process is started.

To solve this issue we use a separate dummy model's connection to
do the disable and enable of the subscription.

https://bugzilla.redhat.com/show_bug.cgi?id=1601015
---
 lib/extensions/ar_migration.rb           | 13 +++++++++++--
 spec/lib/extensions/ar_migration_spec.rb |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index 5320c39601a..59300fc246b 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -4,12 +4,21 @@ def self.migrations_column_present?
       ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
     end
 
+    class HelperARClass < ActiveRecord::Base; end
+
+    def self.restart_subscription(s)
+      c = HelperARClass.establish_connection.connection
+      c.pglogical.subscription_disable(s.id)
+      c.pglogical.subscription_enable(s.id)
+    ensure
+      HelperARClass.remove_connection
+    end
+
     def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
       return unless ArPglogicalMigrationHelper.migrations_column_present?
       region = MiqRegion.find_by(:region => subscription.provider_region)
       until region.migrations_ran&.include?(version)
-        subscription.disable
-        subscription.enable
+        restart_subscription(subscription)
         sleep(wait_time)
         region.reload
       end
diff --git a/spec/lib/extensions/ar_migration_spec.rb b/spec/lib/extensions/ar_migration_spec.rb
index b3f2de0e947..b23aec45c46 100644
--- a/spec/lib/extensions/ar_migration_spec.rb
+++ b/spec/lib/extensions/ar_migration_spec.rb
@@ -48,6 +48,7 @@
       let(:subscription) { double("Subscription", :enable => nil, :disable => nil, :provider_region => my_region.region) }
 
       it "sleeps until the migration is added" do
+        allow(described_class).to receive(:restart_subscription)
         my_region.update_attributes!(:migrations_ran => nil)
         t = Thread.new do
           Thread.current.abort_on_exception = true

From 29656d223a278b41207c99c7784b1cfe1b941beb Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Thu, 20 Sep 2018 15:47:07 -0400
Subject: [PATCH 4/9] Remove useless module names before methods

---
 lib/extensions/ar_migration.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index 59300fc246b..172db8e57a6 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -15,7 +15,7 @@ def self.restart_subscription(s)
     end
 
     def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
-      return unless ArPglogicalMigrationHelper.migrations_column_present?
+      return unless migrations_column_present?
       region = MiqRegion.find_by(:region => subscription.provider_region)
       until region.migrations_ran&.include?(version)
         restart_subscription(subscription)
@@ -25,7 +25,7 @@ def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
     end
 
     def self.update_local_migrations_ran(version, direction)
-      return unless ArPglogicalMigrationHelper.migrations_column_present?
+      return unless migrations_column_present?
       return unless (region = MiqRegion.my_region)
 
       new_migrations = ActiveRecord::SchemaMigration.normalized_versions

From c344144ab2bd25ab3885832796ac3021569ca4d3 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Thu, 20 Sep 2018 16:03:15 -0400
Subject: [PATCH 5/9] Add logging and output when we wait for a remote region
 migration

---
 lib/extensions/ar_migration.rb           | 14 ++++++++++++++
 spec/lib/extensions/ar_migration_spec.rb |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index 172db8e57a6..af6915c43c8 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -4,6 +4,16 @@ def self.migrations_column_present?
       ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
     end
 
+    def self.log_and_print(message)
+      if @current_message == message
+        print "."
+      else
+        Vmdb.rails_logger.info(message)
+        print message
+      end
+      @current_message = message
+    end
+
     class HelperARClass < ActiveRecord::Base; end
 
     def self.restart_subscription(s)
@@ -17,11 +27,15 @@ def self.restart_subscription(s)
     def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
       return unless migrations_column_present?
       region = MiqRegion.find_by(:region => subscription.provider_region)
+      waited = false
       until region.migrations_ran&.include?(version)
+        waited = true
+        log_and_print("Waiting for remote region #{region.region} to run migration #{version}")
         restart_subscription(subscription)
         sleep(wait_time)
         region.reload
       end
+      puts "\n" if waited
     end
 
     def self.update_local_migrations_ran(version, direction)
diff --git a/spec/lib/extensions/ar_migration_spec.rb b/spec/lib/extensions/ar_migration_spec.rb
index b23aec45c46..20704c362f8 100644
--- a/spec/lib/extensions/ar_migration_spec.rb
+++ b/spec/lib/extensions/ar_migration_spec.rb
@@ -4,6 +4,8 @@
     MiqRegion.my_region
   end
 
+  before { allow(described_class).to receive_messages(:puts => nil, :print => nil) }
+
   context "without the migrations ran column" do
     before do
       column_list = %w(id region created_at updated_at description guid).map { |n| double(:name => n) }

From 752dd518852757d994725add779aa4a3a8dabb62 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Fri, 21 Sep 2018 15:55:38 -0400
Subject: [PATCH 6/9] Refactor the extension to use a class for waiting on the
 migration

This simplifies the logic around logging quite a bit
---
 lib/extensions/ar_migration.rb           |  90 ++++++++++--------
 spec/lib/extensions/ar_migration_spec.rb | 116 +++++++++++++----------
 2 files changed, 120 insertions(+), 86 deletions(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index af6915c43c8..720af38c2d8 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -1,43 +1,9 @@
 module ArPglogicalMigration
-  module ArPglogicalMigrationHelper
+  module PglogicalMigrationHelper
     def self.migrations_column_present?
       ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
     end
 
-    def self.log_and_print(message)
-      if @current_message == message
-        print "."
-      else
-        Vmdb.rails_logger.info(message)
-        print message
-      end
-      @current_message = message
-    end
-
-    class HelperARClass < ActiveRecord::Base; end
-
-    def self.restart_subscription(s)
-      c = HelperARClass.establish_connection.connection
-      c.pglogical.subscription_disable(s.id)
-      c.pglogical.subscription_enable(s.id)
-    ensure
-      HelperARClass.remove_connection
-    end
-
-    def self.wait_for_remote_region_migration(subscription, version, wait_time = 1)
-      return unless migrations_column_present?
-      region = MiqRegion.find_by(:region => subscription.provider_region)
-      waited = false
-      until region.migrations_ran&.include?(version)
-        waited = true
-        log_and_print("Waiting for remote region #{region.region} to run migration #{version}")
-        restart_subscription(subscription)
-        sleep(wait_time)
-        region.reload
-      end
-      puts "\n" if waited
-    end
-
     def self.update_local_migrations_ran(version, direction)
       return unless migrations_column_present?
       return unless (region = MiqRegion.my_region)
@@ -54,13 +20,63 @@ def self.update_local_migrations_ran(version, direction)
     end
   end
 
+  class RemoteRegionMigrationWatcher
+    class HelperARClass < ActiveRecord::Base; end
+
+    attr_reader :region, :subscription, :version
+
+    def initialize(subscription, version)
+      @region       = MiqRegion.find_by(:region => subscription.provider_region)
+      @subscription = subscription
+      @version      = version
+    end
+
+    def wait_for_remote_region_migration(wait_time = 1)
+      return unless wait_for_migration?
+
+      Vmdb.rails_logger.info(wait_message)
+      print(wait_message)
+
+      while wait_for_migration?
+        print(".")
+        restart_subscription
+        sleep(wait_time)
+        region.reload
+      end
+
+      puts("\n")
+    end
+
+    private
+
+    def wait_for_migration?
+      migrations_column_present? ? !region.migrations_ran&.include?(version) : false
+    end
+
+    def migrations_column_present?
+      @migrations_column_present ||= PglogicalMigrationHelper.migrations_column_present?
+    end
+
+    def wait_message
+      @wait_message ||= "Waiting for remote region #{region.region} to run migration #{version}"
+    end
+
+    def restart_subscription
+      c = HelperARClass.establish_connection.connection
+      c.pglogical.subscription_disable(subscription.id)
+      c.pglogical.subscription_enable(subscription.id)
+    ensure
+      HelperARClass.remove_connection
+    end
+  end
+
   def migrate(direction)
     PglogicalSubscription.all.each do |s|
-      ArPglogicalMigrationHelper.wait_for_remote_region_migration(s, version.to_s)
+      RemoteRegionMigrationWatcher.new(s, version.to_s).wait_for_remote_region_migration
     end
 
     ret = super
-    ArPglogicalMigrationHelper.update_local_migrations_ran(version.to_s, direction)
+    PglogicalMigrationHelper.update_local_migrations_ran(version.to_s, direction)
     ret
   end
 end
diff --git a/spec/lib/extensions/ar_migration_spec.rb b/spec/lib/extensions/ar_migration_spec.rb
index 20704c362f8..37371fb767e 100644
--- a/spec/lib/extensions/ar_migration_spec.rb
+++ b/spec/lib/extensions/ar_migration_spec.rb
@@ -1,63 +1,94 @@
-describe ArPglogicalMigration::ArPglogicalMigrationHelper do
+shared_context "without the migrations ran column" do
+  before do
+    column_list = %w(id region created_at updated_at description guid).map { |n| double(:name => n) }
+    allow(ActiveRecord::Base.connection).to receive(:columns).with("miq_regions").and_return(column_list)
+  end
+end
+
+shared_context "with a dummy version" do
+  let(:version) { "1234567890" }
+
+  # sanity check - if this is somehow a version we have, these tests will make no sense
+  before { expect(my_region.migrations_ran).not_to include(version) }
+end
+
+context "with a region seeded" do
   let!(:my_region) do
     MiqRegion.seed
     MiqRegion.my_region
   end
 
-  before { allow(described_class).to receive_messages(:puts => nil, :print => nil) }
+  describe ArPglogicalMigration::PglogicalMigrationHelper do
+    context "without the migrations ran column" do
+      include_context "without the migrations ran column"
 
-  context "without the migrations ran column" do
-    before do
-      column_list = %w(id region created_at updated_at description guid).map { |n| double(:name => n) }
-      allow(ActiveRecord::Base.connection).to receive(:columns).with("miq_regions").and_return(column_list)
-    end
+      describe ".migrations_column_present?" do
+        it "is falsey" do
+          expect(described_class.migrations_column_present?).to be_falsey
+        end
+      end
 
-    describe ".migrations_column_present?" do
-      it "is falsey" do
-        expect(described_class.migrations_column_present?).to be_falsey
+      describe ".update_local_migrations_ran" do
+        it "does nothing" do
+          expect(MiqRegion).not_to receive(:my_region)
+          described_class.update_local_migrations_ran("12345", :up)
+        end
       end
     end
 
-    describe ".wait_for_remote_region_migrations" do
-      it "does nothing" do
-        expect(MiqRegion).not_to receive(:find)
-        described_class.wait_for_remote_region_migration(double("subscription"), "12345")
+    describe ".migrations_column_present?" do
+      it "is truthy" do
+        # we never want to remove this column so we can just test directly
+        expect(described_class.migrations_column_present?).to be_truthy
       end
     end
 
     describe ".update_local_migrations_ran" do
-      it "does nothing" do
-        expect(MiqRegion).not_to receive(:my_region)
-        described_class.update_local_migrations_ran("12345", :up)
+      include_context "with a dummy version"
+
+      it "adds the given version when the direction is :up" do
+        described_class.update_local_migrations_ran(version, :up)
+        expect(my_region.reload.migrations_ran).to match_array(ActiveRecord::SchemaMigration.normalized_versions << version)
       end
-    end
-  end
 
-  describe ".migrations_column_present?" do
-    it "is truthy" do
-      # we never want to remove this column so we can just test directly
-      expect(described_class.migrations_column_present?).to be_truthy
+      it "doesn't blow up when there is no region" do
+        MiqRegion.destroy_all
+        MiqRegion.my_region_clear_cache
+        described_class.update_local_migrations_ran(version, :up)
+      end
     end
   end
 
-  context "with a dummy version" do
-    let(:version) { "1234567890" }
+  describe ArPglogicalMigration::RemoteRegionMigrationWatcher do
+    include_context "with a dummy version"
+
+    let(:subscription) { double("Subscription", :enable => nil, :disable => nil, :provider_region => my_region.region) }
 
-    # sanity check - if this is somehow a version we have, these tests will make no sense
-    before { expect(my_region.migrations_ran).not_to include(version) }
+    subject do
+      described_class.new(subscription, version).tap do |s|
+        allow(s).to receive_messages(:puts => nil, :print => nil)
+      end
+    end
 
-    describe ".wait_for_remote_region_migration" do
-      let(:subscription) { double("Subscription", :enable => nil, :disable => nil, :provider_region => my_region.region) }
+    describe "#wait_for_remote_region_migrations" do
+      context "without the migrations ran column present" do
+        include_context "without the migrations ran column"
+
+        it "does nothing" do
+          expect(Vmdb.rails_logger).not_to receive(:info)
+          subject.wait_for_remote_region_migration
+        end
+      end
 
       it "sleeps until the migration is added" do
-        allow(described_class).to receive(:restart_subscription)
-        my_region.update_attributes!(:migrations_ran => nil)
+        allow(subject).to receive(:restart_subscription)
+        allow(subject.region).to receive(:reload)
+
+        subject.region.update_attributes!(:migrations_ran => nil)
+
         t = Thread.new do
           Thread.current.abort_on_exception = true
-          # need to stub these because the thread uses a separate connection object which won't be in the same transaction
-          allow(MiqRegion).to receive(:find_by).with(:region => my_region.region).and_return(my_region)
-          allow(my_region).to receive(:reload)
-          described_class.wait_for_remote_region_migration(subscription, version, 0)
+          subject.wait_for_remote_region_migration(0)
         end
 
         # Try to pass execution to the created thread
@@ -68,25 +99,12 @@
         sleep 1
 
         expect(t.alive?).to be true
-        my_region.update_attributes!(:migrations_ran => ActiveRecord::SchemaMigration.normalized_versions << version)
+        subject.region.update_attributes!(:migrations_ran => ActiveRecord::SchemaMigration.normalized_versions << version)
 
         # Wait a max of 5 seconds so we don't disrupt the whole test suite if something terrible happens
         t = t.join(5)
         expect(t.status).to be false
       end
     end
-
-    describe ".update_local_migrations_ran" do
-      it "adds the given version when the direction is :up" do
-        described_class.update_local_migrations_ran(version, :up)
-        expect(my_region.reload.migrations_ran).to match_array(ActiveRecord::SchemaMigration.normalized_versions << version)
-      end
-
-      it "doesn't blow up when there is no region" do
-        MiqRegion.destroy_all
-        MiqRegion.my_region_clear_cache
-        described_class.update_local_migrations_ran(version, :up)
-      end
-    end
   end
 end

From a11110d9aecc68683969ba8e8c1de25d1409eb2b Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Tue, 25 Sep 2018 17:41:44 -0400
Subject: [PATCH 7/9] Avoid MiqRegion model in
 ArPglogicalMigration::PglogicalMigrationHelper

---
 lib/extensions/ar_migration.rb           | 16 ++++++++++++++--
 spec/lib/extensions/ar_migration_spec.rb |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index 720af38c2d8..cd0151e09c2 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -4,9 +4,21 @@ def self.migrations_column_present?
       ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
     end
 
+    def self.my_region_number
+      @my_region_number ||= ApplicationRecord.my_region_number
+    end
+
+    def self.my_region_created?
+      ActiveRecord::Base.connection.exec_query(<<~SQL).first["exists"]
+        SELECT EXISTS(
+          SELECT id FROM miq_regions WHERE region = #{ActiveRecord::Base.connection.quote(my_region_number)}
+        )
+      SQL
+    end
+
     def self.update_local_migrations_ran(version, direction)
       return unless migrations_column_present?
-      return unless (region = MiqRegion.my_region)
+      return unless my_region_created?
 
       new_migrations = ActiveRecord::SchemaMigration.normalized_versions
       new_migrations << version if direction == :up
@@ -15,7 +27,7 @@ def self.update_local_migrations_ran(version, direction)
       ActiveRecord::Base.connection.exec_query(<<~SQL)
         UPDATE miq_regions
         SET migrations_ran = #{migrations_value}
-        WHERE id = #{region.id}
+        WHERE region = #{ActiveRecord::Base.connection.quote(my_region_number)}
       SQL
     end
   end
diff --git a/spec/lib/extensions/ar_migration_spec.rb b/spec/lib/extensions/ar_migration_spec.rb
index 37371fb767e..fc823f5af3f 100644
--- a/spec/lib/extensions/ar_migration_spec.rb
+++ b/spec/lib/extensions/ar_migration_spec.rb
@@ -30,7 +30,7 @@
 
       describe ".update_local_migrations_ran" do
         it "does nothing" do
-          expect(MiqRegion).not_to receive(:my_region)
+          expect(ActiveRecord::SchemaMigration).not_to receive(:normalized_versions)
           described_class.update_local_migrations_ran("12345", :up)
         end
       end

From 9b48682727036c0464564bd1de13dbbd24115523 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Tue, 25 Sep 2018 17:47:02 -0400
Subject: [PATCH 8/9] Use an anonymous class for querying the miq_regions table

This de-couples us from the model definition during migrations
---
 lib/extensions/ar_migration.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index cd0151e09c2..1b138bd14a4 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -38,7 +38,8 @@ class HelperARClass < ActiveRecord::Base; end
     attr_reader :region, :subscription, :version
 
     def initialize(subscription, version)
-      @region       = MiqRegion.find_by(:region => subscription.provider_region)
+      region_class  = Class.new(ApplicationRecord) { self.table_name = "miq_regions" }
+      @region       = region_class.find_by(:region => subscription.provider_region)
       @subscription = subscription
       @version      = version
     end

From 30a2e2c35d739af14d7a4bd0cc06e2f94703ec85 Mon Sep 17 00:00:00 2001
From: Nick Carboni <ncarboni@redhat.com>
Date: Wed, 26 Sep 2018 15:02:15 -0400
Subject: [PATCH 9/9] Clarify use of ApplicationRecord vs ActiveRecord::Base

In this module we lean towards ActiveRecord::Base to avoid any
conflict with application logic.
We also cannot use ApplicationRecord at load time because it causes
a load error.
---
 lib/extensions/ar_migration.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/extensions/ar_migration.rb b/lib/extensions/ar_migration.rb
index 1b138bd14a4..e952bad150b 100644
--- a/lib/extensions/ar_migration.rb
+++ b/lib/extensions/ar_migration.rb
@@ -5,6 +5,7 @@ def self.migrations_column_present?
     end
 
     def self.my_region_number
+      # Use ApplicationRecord here because we need to query region information
       @my_region_number ||= ApplicationRecord.my_region_number
     end
 
@@ -38,7 +39,7 @@ class HelperARClass < ActiveRecord::Base; end
     attr_reader :region, :subscription, :version
 
     def initialize(subscription, version)
-      region_class  = Class.new(ApplicationRecord) { self.table_name = "miq_regions" }
+      region_class  = Class.new(ActiveRecord::Base) { self.table_name = "miq_regions" }
       @region       = region_class.find_by(:region => subscription.provider_region)
       @subscription = subscription
       @version      = version