Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a patch to ActiveRecord::Migration for tracking replicated migrations #17919

Merged
merged 9 commits into from
Sep 26, 2018
6 changes: 5 additions & 1 deletion app/models/miq_region.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
98 changes: 98 additions & 0 deletions lib/extensions/ar_migration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
module ArPglogicalMigration
module PglogicalMigrationHelper
def self.migrations_column_present?
ActiveRecord::Base.connection.columns("miq_regions").any? { |c| c.name == "migrations_ran" }
end

def self.my_region_number
# Use ApplicationRecord here because we need to query region information
@my_region_number ||= ApplicationRecord.my_region_number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overheard you describing how you can use ApplicationRecord here but must must use AR::Base elsewhere because reasons... the flip flopping of their usage here seems to need a "why" comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the only places I need to use ActiveRecord::Base are when the code is evaluated at load-time rather than runtime. So technically I could use ApplicationRecord.connection in the definitions of .migrations_column_present? and .my_region_created?.

Here I need to use ApplicationRecord because we include the region-aware modules into that class rather than ActiveRecord::Base.

The place that would cause a problem with constant loading would really be HelperARClass. But I also think I could probably get away with using an anonymous class there if I tried really hard. What do you think?

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slick use of squiggly heredoc with raw SQL 👍

end

def self.update_local_migrations_ran(version, direction)
return unless migrations_column_present?
return unless my_region_created?

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 region = #{ActiveRecord::Base.connection.quote(my_region_number)}
SQL
end
end

class RemoteRegionMigrationWatcher
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this might be slightly better than RemoteRegionThing 😆 carbonin#4 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuff and Thing... the best placeholder names that gets you to the point of understanding what it actually does... and hopefully no reviewer will actually merge it until it's renamed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully no reviewer will actually merge it until it's renamed

Hopefully ... but not always

$ git grep "it \"works\""
spec/lib/extensions/ar_taggable_spec.rb:125:    it "works" do
spec/lib/extensions/ar_taggable_spec.rb:143:    it "works" do
spec/lib/rbac_spec.rb:30:      it "works" do
spec/lib/rbac_spec.rb:61:        it "works" do
spec/lib/rbac_spec.rb:75:        it "works" do
spec/models/authentication_spec.rb:38:    it "works" do
spec/models/category_spec.rb:3:    it "works" do
spec/models/miq_approval_spec.rb:16:    it "works" do
spec/models/mixins/authentication_mixin_spec.rb:144:    it "works" do
spec/models/mixins/authentication_mixin_spec.rb:205:          it "works" do
spec/models/vm_or_template_spec.rb:1096:    it "works" do
spec/models/vm_or_template_spec.rb:1106:    it "works" do
spec/models/vm_or_template_spec.rb:1116:    it "works" do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget:

$ git grep "it \"stuff\""
spec/lib/vmdb/loggers/container_logger_spec.rb:2:  it "stuff" do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully ... but not always

I bet every one of those are @bdunne 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet every one of those are @bdunne

I think only it "stuff" is me actually

class HelperARClass < ActiveRecord::Base; end

attr_reader :region, :subscription, :version

def initialize(subscription, version)
region_class = Class.new(ActiveRecord::Base) { self.table_name = "miq_regions" }
@region = region_class.find_by(:region => subscription.provider_region)
@subscription = subscription
bdunne marked this conversation as resolved.
Show resolved Hide resolved
@version = version
end

def wait_for_remote_region_migration(wait_time = 1)
return unless wait_for_migration?

Vmdb.rails_logger.info(wait_message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use this rather than Rails.logger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that Vmdb logger was the better one since it goes through the whole multicast thing, but I can change it if we should prefer Rails.logger. Or I could also use $rails_log (which is what Vmdb.rails_logger is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it goes through the whole multicast thing

Okay, that's probably better. I was just trying to divorce this extension from manageiq as much as possible

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|
RemoteRegionMigrationWatcher.new(s, version.to_s).wait_for_remote_region_migration
end

ret = super
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super.tap?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, kind of would rather this version because we wouldn't be using the return value of super in the tap block.
I think the way I have it is more explicit about returning the previous value where using tap feels more like taking advantage of a side-effect.

PglogicalMigrationHelper.update_local_migrations_ran(version.to_s, direction)
ret
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super.tap do 
  MigrationSyncHelper.update_local_migrations_ran(version.to_s, direction)
end

might read a little better than having a temp var.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdunne actually suggested the same thing and I prefer the temp variable because I see the "return the original object" part of taps behavior as more of a side-effect rather than the primary reason for using it. If my goal is to just return the same value rather than do something with the value returned by super I like this way better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with not using tap since we're not actually using the proposed block variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, what does super return? ret could be named better. I honestly have no idea what it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually know either. I didn't want to use it for anything, but figured it was good to preserve it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh funny, I see .tap's return of the original value as the main feature...in that sense I read it as more of a "oh and by the way", and it happens to just have a reference to the tapped thing, but it's not required to use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, "I have a thing that I need to return, but before I return it, I need to do this other thing"

I agree with not using tap since we're not actually using the proposed block variable.

@jrafanie we use block syntax all the time without referencing anything that is yielded

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I can see that use case. I prefer the original for clear separation of the super from the post-super work. It's preference.

end
end

ActiveRecord::Migration.prepend(ArPglogicalMigration)
110 changes: 110 additions & 0 deletions spec/lib/extensions/ar_migration_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
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

describe ArPglogicalMigration::PglogicalMigrationHelper do
context "without the migrations ran column" do
include_context "without the migrations ran column"

describe ".migrations_column_present?" do
it "is falsey" do
expect(described_class.migrations_column_present?).to be_falsey
end
end

describe ".update_local_migrations_ran" do
it "does nothing" do
expect(ActiveRecord::SchemaMigration).not_to receive(:normalized_versions)
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

describe ".update_local_migrations_ran" do
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

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

describe ArPglogicalMigration::RemoteRegionMigrationWatcher do
include_context "with a dummy version"

let(:subscription) { double("Subscription", :enable => nil, :disable => nil, :provider_region => my_region.region) }

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_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(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
subject.wait_for_remote_region_migration(0)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test scares me a little. It looks like it should be fine but threading is hard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging it as is but it does make me 😨


# 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
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇

t = t.join(5)
expect(t.status).to be false
end
end
end
end
4 changes: 4 additions & 0 deletions spec/models/miq_region_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down