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

Migrations for suspending provider #222

Closed
wants to merge 10 commits into from
5 changes: 5 additions & 0 deletions db/migrate/20180618083035_add_visible_to_zone.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddVisibleToZone < ActiveRecord::Migration[5.0]
def change
add_column :zones, :visible, :boolean
end
end
16 changes: 16 additions & 0 deletions db/migrate/20180618084054_init_zones_visibility.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class InitZonesVisibility < ActiveRecord::Migration[5.0]
class Zone < ActiveRecord::Base
end

def up
say_with_time("Updating all zones to visible") do
Zone.update_all(:visible => true)
end
end

def down
say_with_time("Resetting zone visibility") do
slemrmartin marked this conversation as resolved.
Show resolved Hide resolved
Zone.update_all(:visible => nil)
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddBackupZoneIdToExtManagementSystem < ActiveRecord::Migration[5.0]
def change
add_reference :ext_management_systems, :backup_zone, :type => :bigint, :index => true
end
end
97 changes: 97 additions & 0 deletions db/migrate/20180626125655_rename_user_maintenance_zone_record.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
class RenameUserMaintenanceZoneRecord < ActiveRecord::Migration[5.0]
class UniqueWithinRegionValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
return if value.nil?
match_case = options.key?(:match_case) ? options[:match_case] : true
record_base_class = record.class.base_class
field_matches =
if match_case
record_base_class.where(attribute => value)
else
record_base_class.where(record_base_class.arel_attribute(attribute).lower.eq(value.downcase))
end
unless field_matches.in_region(record.region_id).where.not(:id => record.id).empty?
record.errors.add(attribute, "is not unique within region #{record.region_id}")
end
end
end

class Zone < ActiveRecord::Base
include ActiveRecord::IdRegions

validates_presence_of :name
validates :name, "rename_user_maintenance_zone_record/unique_within_region" => true

MAINTENANCE_ZONE_NAME = "__maintenance__".freeze
end

class Job < ActiveRecord::Base
self.inheritance_column = :_type_disabled

belongs_to :miq_task, :dependent => :delete
after_update_commit :update_linked_task

def update_linked_task
miq_task&.update_attributes!(:zone => zone)
end
end

class MiqTask < ActiveRecord::Base
has_one :job, :dependent => :destroy
has_one :miq_queue
end

class MiqQueue < ActiveRecord::Base
belongs_to :miq_task
end

def up
say_with_time("Renaming user-defined Maintenance Zone") do
zone = Zone.in_my_region.find_by(:name => Zone::MAINTENANCE_ZONE_NAME)
if zone.present?
# STEP 1 - rename zone
Copy link
Member

Choose a reason for hiding this comment

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

I still think this is the wrong approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdunne you were worried about string associations

  1. possibility that user has __maintenance__ as name is almost zero
  2. if so, zone and associations are renamed to name with possibility of duplication zero^2.

could you be more specific? When this fails (with significant probability).
Is it not acceptable for @Fryguy, @agrare, @carbonin too?

Copy link
Member

Choose a reason for hiding this comment

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

  1. possibility that user has maintenance as name is almost zero

I agree, but while unlikely, not impossible

  1. if so, zone and associations are renamed to name with possibility of duplication zero^2.

Right now you're only updating Job and MiqQueue. The following models have a zone column:

=> ["MiqQueue", "Job", "MiqTask", "ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job", "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::PlaybookRunner", "ManageIQ::Providers::EmsRefreshWorkflow", "ManageIQ::Providers::AnsibleRunnerWorkflow", "VmScan", "ManageIQ::Providers::NativeOperationWorkflow", "Service::LinkingWorkflow"]

I also believe it's in Settings and who knows how many other places it's buried in a serialized column. I just don't think it's practical to try renaming a zone

zone.name = "#{zone.name}_#{Zone.my_region_number}"
if zone.save
# STEP 2 - rename string associations
rename_string_associations(Zone::MAINTENANCE_ZONE_NAME, zone.name)
else
raise "Zone '#{Zone::MAINTENANCE_ZONE_NAME}' cannot be renamed to #{zone.name}. Reason: #{zone.errors.messages.inspect}"
end
end
end
end

def down
say_with_time("Renaming user-defined Maintenance Zone") do
Zone.in_my_region.where(:name => Zone::MAINTENANCE_ZONE_NAME,
:visible => false).destroy_all

user_zone_name = "#{Zone::MAINTENANCE_ZONE_NAME}_#{Zone.my_region_number}"
orig = Zone.in_my_region.find_by(:name => user_zone_name)
if orig.present?
# STEP 1 - rename user's zone back
orig.name = Zone::MAINTENANCE_ZONE_NAME
if orig.save
# STEP 2 - rename string associations
rename_string_associations(user_zone_name, Zone::MAINTENANCE_ZONE_NAME)
else
raise "Zone '#{user_zone_name}' cannot be renamed to #{Zone::MAINTENANCE_ZONE_NAME}. Reason: #{orig.errors.messages.inspect}"
end
end
end
end

private

def rename_string_associations(old_zone_name, new_zone_name)
[Job, MiqQueue].each do |klass|
klass.where(:zone => old_zone_name).each do |record|
record.zone = new_zone_name
unless record.save
# This is not critical, migrations can continue
say("WARN: #{klass.name}'s (id: #{record.id}) zone not renamed. Reason: #{record.errors.messages.inspect}")
end
end
end
end
end
26 changes: 26 additions & 0 deletions spec/migrations/20180618084054_init_zones_visibility_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require_migration
describe InitZonesVisibility do
let(:zone_stub) { migration_stub(:Zone) }

migration_context :up do
it "makes zones visible" do
zone = zone_stub.create!(:name => 'zone1')

migrate
zone.reload

expect(zone.visible).to be_truthy
end
end

migration_context :down do
it 'resets zone visibility' do
zone = zone_stub.create!(:name => 'zone_visible', :visible => true)

migrate
zone.reload

expect(zone.visible).to be_nil
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
require_migration

describe RenameUserMaintenanceZoneRecord do
let(:zone_stub) { migration_stub(:Zone) }
let(:job_stub) { migration_stub(:Job) }
let(:miq_task_stub) { migration_stub(:MiqTask) }
let(:miq_queue_stub) { migration_stub(:MiqQueue) }

let(:remote_region_start) do
anonymous_class_with_id_regions.rails_sequence_start +
anonymous_class_with_id_regions.rails_sequence_factor
end

migration_context :up do
it "renames original maintenance zone" do
orig = zone_stub.create!(:name => zone_stub::MAINTENANCE_ZONE_NAME)

migrate
orig.reload

expect(orig.name).to eq("#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")
end

it "does not rename zone from remote regions" do
id = remote_region_start

user_zone = zone_stub.create!(:id => id,
:name => zone_stub::MAINTENANCE_ZONE_NAME)

migrate
user_zone.reload

expect(user_zone.name).to eq(zone_stub::MAINTENANCE_ZONE_NAME)
end

it "renames string associations belonging to former maintenance zone" do
orig = zone_stub.create!(:name => zone_stub::MAINTENANCE_ZONE_NAME)
assoc = {}
assoc[:miq_task] = miq_task_stub.create!(:zone => orig.name)
assoc[:job] = job_stub.create!(:zone => orig.name, :miq_task => assoc[:miq_task])
assoc[:miq_queue] = miq_queue_stub.create!(:zone => orig.name, :miq_task => assoc[:miq_task])

migrate
orig.reload
assoc.each_value do |record|
record.reload
expect(record.zone).to eq(orig.name)
end
end

it "does not rename unrelated jobs, tasks and queues" do
orig = zone_stub.create!(:name => zone_stub::MAINTENANCE_ZONE_NAME)
other_zone_name = "Some other zone"
other = zone_stub.create!(:name => other_zone_name)

assoc = {}
assoc[:miq_task] = miq_task_stub.create!(:zone => other.name)
assoc[:job] = job_stub.create!(:zone => other.name, :miq_task => assoc[:miq_task])
assoc[:miq_queue] = miq_queue_stub.create!(:zone => other.name, :miq_task => assoc[:miq_task])

migrate
orig.reload
other.reload
expect(other.name).to eq(other_zone_name)
assoc.each_value do |record|
record.reload
expect(record.zone).to eq(other_zone_name)
end
end

it "raises error when original zone renamed to existing one" do
zone_stub.create!(:name => zone_stub::MAINTENANCE_ZONE_NAME)
zone_stub.create!(:name => "#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")
expect { migrate }.to raise_error(/is not unique within region/)
end
end

migration_context :down do
it "removes MaintenanceZone" do
zone_stub.create!(:name => zone_stub::MAINTENANCE_ZONE_NAME,
:description => 'Maintenance Zone',
:visible => false)

migrate

expect(zone_stub.where(:name => zone_stub::MAINTENANCE_ZONE_NAME).where(:visible => false).count).to eq(0)
end

it "renames original maintenance zone back" do
orig = zone_stub.create!(:name => "#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")

migrate
orig.reload

expect(orig.name).to eq(zone_stub::MAINTENANCE_ZONE_NAME)
end

it "does not rename original maintenance zone from remote region" do
id = remote_region_start

user_zone = zone_stub.create!(:id => id,
:name => "#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")

migrate
user_zone.reload

expect(user_zone.name).to eq("#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")
end

it "renames string associations belonging to original maintenance zone back" do
orig = zone_stub.create!(:name => "#{zone_stub::MAINTENANCE_ZONE_NAME}_#{zone_stub.my_region_number}")
assoc = {}
assoc[:miq_task] = miq_task_stub.create!(:zone => orig.name)
assoc[:job] = job_stub.create!(:zone => orig.name, :miq_task => assoc[:miq_task])
assoc[:miq_queue] = miq_queue_stub.create!(:zone => orig.name, :miq_task => assoc[:miq_task])

migrate
orig.reload
assoc.each_value do |record|
record.reload
expect(record.zone).to eq(orig.name)
end
end
end
end