Migrations for suspending provider #222

slemrmartin · 2018-06-18T14:34:09Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Issue: ManageIQ/manageiq#17489

(dependent) Suspend providers manageiq#17452

Adds Zone visibility and EMS's backup zone relation

slemrmartin · 2018-06-27T14:11:13Z

Cc @Fryguy

Fryguy · 2018-06-28T15:17:23Z

db/migrate/20180618083035_add_visible_to_zone.rb

@@ -0,0 +1,5 @@
+class AddVisibleToZone < ActiveRecord::Migration[5.0]
+  def change
+    add_column :zones, :visible, :boolean, :default => true


Prefer default_value_for at the model level over a database-level default.

That being said, you will need a data migration to update all existing zones to visible => true.

@Fryguy exactly, so why is preferred more difficult solution? database-level default, at least for boolean, was standard for every project I seen before.

I thought about it, what about compromise solution? keep :default => true as better alternative to data migration and add default_value_for to keep your code standards

...ok, I disagree with your approach, but we need to finish this RFE, so as you wish

Fryguy · 2018-06-28T15:18:20Z

db/migrate/20180618084757_add_backup_zone_id_to_ext_management_system.rb

@@ -0,0 +1,5 @@
+class AddBackupZoneIdToExtManagementSystem < ActiveRecord::Migration[5.0]
+  def change
+    add_reference :ext_management_systems, :backup_zone, :type => :bigint, :index => true, :after => :zone_id


Remove the :after bit...it's not necessary.

it makes db structure more clear, if you group logical related columns together

Fryguy · 2018-06-28T15:19:14Z

db/migrate/20180626125654_make_maintenance_zone_record.rb

+      Zone.where(:name => 'maintenance').where(:visible => false).destroy_all
+    end
+  end
+end


This entire migration is unnecessary, because this can be handled in Zone.seed.

@Fryguy - are you sure that Zone.seed is called after migrations in all existing installations?

Fryguy · 2018-07-31T13:59:21Z

db/migrate/20180626125654_make_maintenance_zone_record.rb

+class MakeMaintenanceZoneRecord < ActiveRecord::Migration[5.0]
+  class Zone < ActiveRecord::Base
+    validates_presence_of   :name
+    validates_uniqueness_of :name


These must be removed. In a global region, since zones are transferred from other regions, there are multiple "default" zones, for example. As such, this migration will fail to migrate a global database.

Fryguy · 2018-07-31T14:00:33Z

db/migrate/20180626125654_make_maintenance_zone_record.rb

+
+  def up
+    say_with_time("Creating Maintenance Zone") do
+      zone = Zone.where(:name => Zone::MAINTENANCE_ZONE_NAME).first


It just occurred to me that this is not region aware. Please change this to Zone.in_my_region.where(...), to allow migrating a global database. You can also create a spec showing this, but will need to give each zone an id within a region. There should be other migrations that have examples showing how to do that.

slemrmartin · 2018-08-01T13:15:39Z

@Fryguy I'm not sure, how exactly migrations work on global database so please look if in_my_region occurences are enough

Fryguy · 2018-08-01T14:19:38Z

spec/migrations/20180626125654_make_maintenance_zone_record_spec.rb

+
+  migration_context :up do
+    before(:each) do
+      zone_stub.delete_all


Not sure why you need this...the database should be empty.

it is not cleaned between specs in migration_context when I use zone_stub.create!. I don't know if it is some bad local settings, but specs are failing without this line.

Fryguy · 2018-08-01T14:22:30Z

db/migrate/20180626125654_make_maintenance_zone_record.rb

+        zone.save
+      end
+
+      Zone.create_with(:description => "Maintenance Zone", :visible => false).find_or_create_by!(:name => Zone::MAINTENANCE_ZONE_NAME)


We typically don't create seed data in migrations, instead doing that in the .seed method. Typically a migrated empty database should still be empty in the end.

What I recommend here is to keep the bit where we rename any existing __maintenance__ zone, but get rid of the part that actually creates the maintenance zone, instead moving that to .seed in the core.

slemrmartin · 2018-08-01T15:22:33Z

spec/migrations/20180626125655_rename_user_maintenance_zone_record_spec.rb

+
+  migration_context :up do
+    before(:each) do
+      zone_stub.delete_all


@Fryguy my respose was hidden after commit...
I think it should be cleared, but it's not, create! in spec below is creating more and more records when running repeatedly and spec doesn't work then.

hm, this should not happen, something must be off, because it should all just a transaction that gets rolled back

Delete like this will not work if the specs will run in parallel. So we need to find a way to to properly fix this

@Ladas probably some bad before :all in the past in my local db so it work without it normally.

agrare · 2018-08-24T15:40:42Z

@carbonin 👍 works for me

slemrmartin · 2018-08-28T07:48:07Z

@carbonin I'm not sure what you mean by it - what does it mean "just fail the migration"?
Migration only renames possible existing zone, it doesn't create our __maintenance__ zone (because this code is in Zone.seed) - so this conflict can happen in runtime.

And creating of zone in migration doesn't want @Fryguy, so it seems to me that we are in loop :)

agrare · 2018-08-28T13:29:42Z

@slemrmartin Just leave the rename as is but add an exception handler that prints a message like "Failed to rename maintenance, check for zone conflicts" and re-raise the exception. It will fail the upgrade and the customer will have to remediate it.

db/migrate/20180618084054_init_zones_visibility.rb

bdunne · 2018-08-28T13:49:40Z

db/migrate/20180626125655_rename_user_maintenance_zone_record.rb

+  class Zone < ActiveRecord::Base
+    include ActiveRecord::IdRegions
+
+    validates_presence_of   :name


Is this necessary?

If it is, it need to be multi-region aware.

bdunne · 2018-08-28T13:52:19Z

db/migrate/20180626125655_rename_user_maintenance_zone_record.rb

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


The wheres can be merged

bdunne · 2018-08-28T13:58:14Z

"just fail the migration"

Doesn't seem acceptable to me. If it was only developers I could be okay with it, but not for customers. I know it's an edge case, but they're not going to know what went wrong or how to fix it and it's going to lead to an unnecessary support call on a weekend (since that's when they do upgrades). Maybe we could talk about something like #222 (comment) instead?

carbonin · 2018-08-28T14:00:00Z

@bdunne they should know exactly what to do to fix it if we include the remediation steps in the failure message. It should be straight forward enough.

bdunne · 2018-08-28T14:18:07Z

Okay, so @carbonin and I were just discussing this some more...

I think renaming an existing zone in a migration is just out of the question. Too many things don't use real relations and point to zones by name (mistakes were made), that's why there is a uniqueness validation on Zone#name.

That said, it really doesn't matter what the name of our maintenance zone is as long as we can find it when we want to use it. I think we should generate a random zone name and create that as our maintenance zone (and if it conflicts with an existing one, generate a new one until it doesn't conflict). Then we can put a reference to that zone on MiqRegion so that we can find it in the future.

Is there any case where we would want multiple maintenance zones?

slemrmartin · 2018-08-28T15:14:02Z

@bdunne good point
maintenance zone is identified by unique name (just like default zone, I wanted to be consistent) and created outside migration in Zone.seed

Answer is: No, we don't want multiple maintenance zones - that's another reason why it isn't identified by some boolean flag.

I consider your solution as more robust, but it requires new migration to MiqRegion - I hope for choosing any solution, it was customer request (urgent, I suppose) 3 months ago, I think ;))

Fryguy · 2018-09-04T14:21:14Z

Reading back, I'm on the fence about renaming maintenance zone. On one hand, having a hard-coded name is nice because that's what we have for the default zone. However the difference is that the default zone is seeded way before a customer gets a chance to create their own zone. Additionally, as @bdunne mentioned, the data migration would also have to rename every column where we store zone as a name, such as in MiqQueue, and in the worst case some deep yaml options column (I really hope this isn't a thing, but you never know).

That said, it really doesn't matter what the name of our maintenance zone is

It does matter if we have to show it somewhere. We can probably easily hide it from the Zone configuration page, but my concern is there's going to some random locations in the UI we haven't considered and the user is going to see __mainenance__525face0-927b-0136-588e-3c15c2c7fbb8 .

Is there any case where we would want multiple maintenance zones?

I don't think we want multiple zones for any particular purpose, but there will be multiple maintenance zones upon replication. For the default zone we deal with this just fine, but the default zone isn't "special"; it's just a normal zone we seed. For maintenance, if we need to "hide" it, then you get into having to hide multiple zones in the UI.

carbonin · 2018-09-04T15:03:32Z

@Fryguy For what it's worth, we don't show the region accordion for anything other than the current region. So we don't really display the zone anywhere in the UI.

But while thinking about this, I'm realizing that the provider (and its relation to its zone) would be replicated. That makes me think that the global will need the information about which zone is is the maintenance zone for each of the remote regions to display the provider correctly (as suspended).

slemrmartin · 2018-09-04T15:25:50Z

I would prefer to keep identification by name __maintenance__. We searched for all occurences in UI and API and hide this zone (and others) identified by visible? boolean.
But there could be always some random place, of course.

If we need to rename existing __maintenance__, I can search schema.rb for all "zone" columns which are string and add it to this migration. This can solve remaining problems, right?

Also backup zone column for ems https://bugzilla.redhat.com/show_bug.cgi?id=1455145

https://bugzilla.redhat.com/show_bug.cgi?id=1455145

If previously created by user

Due to global region

For global database https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Present also in Zone.seed

Like Job,MiqTask,MiqQueue + raises error when renaming existing zone to another existing.

miq-bot · 2018-09-10T13:34:33Z

Checked commits slemrmartin/manageiq-schema@8767c08~...87bbb6a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 2 offenses detected

db/migrate/20180618084054_init_zones_visibility.rb

❗ - Line 13, Col 12 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.
❗ - Line 7, Col 12 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

bdunne · 2018-09-19T15:39:31Z

db/migrate/20180626125655_rename_user_maintenance_zone_record.rb

+    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


I still think this is the wrong approach

@bdunne you were worried about string associations

possibility that user has __maintenance__ as name is almost zero

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?

possibility that user has maintenance as name is almost zero

I agree, but while unlikely, not impossible

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

slemrmartin · 2018-09-20T09:08:22Z

I prepared alternative PR with region association:
#275

agrare · 2018-09-24T17:52:36Z

Closing in favor of #275 now that it is merged

miq-bot added the wip label Jun 18, 2018

This was referenced Jun 18, 2018

Suspend providers ManageIQ/manageiq#17452

Merged

Suspend provider ManageIQ/manageiq#17489

Closed

slemrmartin changed the title ~~[WIP] Migrations for suspending provider~~ Migrations for suspending provider Jun 20, 2018

miq-bot removed the wip label Jun 20, 2018

slemrmartin changed the title ~~Migrations for suspending provider~~ [WIP] Migrations for suspending provider Jun 20, 2018

miq-bot added the wip label Jun 20, 2018

slemrmartin force-pushed the suspend-provider branch 2 times, most recently from 69d2243 to 2085d39 Compare June 27, 2018 13:51

slemrmartin changed the title ~~[WIP] Migrations for suspending provider~~ Migrations for suspending provider Jun 27, 2018

miq-bot removed the wip label Jun 27, 2018

slemrmartin force-pushed the suspend-provider branch from 2085d39 to 5099394 Compare June 28, 2018 07:12

Fryguy reviewed Jun 28, 2018

View reviewed changes

slemrmartin force-pushed the suspend-provider branch 2 times, most recently from f85d7e2 to c8b53f3 Compare July 10, 2018 12:12

Fryguy reviewed Jul 31, 2018

View reviewed changes

slemrmartin force-pushed the suspend-provider branch from 1c19691 to 6c36a15 Compare August 1, 2018 13:13

Fryguy reviewed Aug 1, 2018

View reviewed changes

slemrmartin commented Aug 1, 2018

View reviewed changes

slemrmartin force-pushed the suspend-provider branch from e2f4ce1 to e4b3ab6 Compare August 7, 2018 08:32

slemrmartin closed this Aug 7, 2018

slemrmartin reopened this Aug 7, 2018

slemrmartin closed this Aug 7, 2018

slemrmartin reopened this Aug 7, 2018

bdunne requested changes Aug 28, 2018

View reviewed changes

slemrmartin added 9 commits September 10, 2018 10:06

Zone changes for suspending provider

8767c08

Also backup zone column for ems https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Changed EMS's backup zone to bigint

c3dda92

Fixes Zone namespace

d5a8fea

Splitting struct/data migrations

5904ac4

https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Changes according to ManageIQ policy

43dedf3

Renaming existing zone

0192756

If previously created by user

Remove uniqueness validation from Zone.

a798122

Due to global region

Region-aware renaming of zones

a73f423

For global database https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Removing Maintenance zone seed from migration

59b1e78

Present also in Zone.seed

slemrmartin force-pushed the suspend-provider branch from e4b3ab6 to 0adca06 Compare September 10, 2018 13:28

Renaming maintenance zone's string associations.

87bbb6a

Like Job,MiqTask,MiqQueue + raises error when renaming existing zone to another existing.

slemrmartin force-pushed the suspend-provider branch from 0adca06 to 87bbb6a Compare September 10, 2018 13:31

bdunne reviewed Sep 19, 2018

View reviewed changes

slemrmartin mentioned this pull request Sep 20, 2018

Maintenance zone support for suspending provider #275

Merged

1 task

agrare closed this Sep 24, 2018

slemrmartin deleted the suspend-provider branch September 24, 2018 18:21

Migrations for suspending provider #222

Migrations for suspending provider #222

Conversation

slemrmartin commented Jun 18, 2018 • edited Loading

slemrmartin commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin commented Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Aug 24, 2018

slemrmartin commented Aug 28, 2018

agrare commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Aug 28, 2018

carbonin commented Aug 28, 2018

bdunne commented Aug 28, 2018

slemrmartin commented Aug 28, 2018

Fryguy commented Sep 4, 2018

carbonin commented Sep 4, 2018

slemrmartin commented Sep 4, 2018

miq-bot commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin commented Sep 20, 2018

agrare commented Sep 24, 2018

slemrmartin commented Jun 18, 2018 •

edited

Loading

slemrmartin commented Aug 1, 2018 •

edited

Loading