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

Maintenance zone support for suspending provider #275

Merged
merged 2 commits into from
Sep 24, 2018

Conversation

slemrmartin
Copy link
Contributor

@slemrmartin slemrmartin commented Sep 20, 2018

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1455145
Issue: ManageIQ/manageiq#17489

Suspending provider consists of moving provider (ExtManagementSystem) to maintenance zone.

Maintenance zone will be created in Zone.seed and identified by association to region.
Name of zone will be generated as maintenance_#{uuid}. This zone will have visible flag set to false, which will be handled by PRs in UI and API.


This is an alternative to #222, where zone is supposed to be identified by unique name.

@slemrmartin
Copy link
Contributor Author

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 I prefer this approach. I would probably put Zone.update_all(:visible => true) in the same migration that adds the column.

@Ladas
Copy link
Contributor

Ladas commented Sep 24, 2018

@bdunne I believe this is the last day we can merge it? So can you? (btw. I believe it was already commented that we should not mix data and schema migrations together)

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

👍

@agrare
Copy link
Member

agrare commented Sep 24, 2018

@slemrmartin what does the corresponding core PR look like if the maintenance zone name isn't constant?
e.g. here https://github.com/ManageIQ/manageiq/pull/17452/files#diff-d8c450b508868941c2b0c5884e718324R129 where all we know is the maintenance zone name.
Are you going to have to do some query for Zone.where(:visible => :false) and cache that name?

@bdunne
Copy link
Member

bdunne commented Sep 24, 2018

We should probably implement a Zone.maintenance_zone method that caches on the class

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

Choose a reason for hiding this comment

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

I know this was here from the original PR, but I'm not a fan of the name of this column.

backup makes it sound like it will get used if there is a problem rather than as a part of normal operation (i.e. resume).

I haven't been able to come up with anything I'm really happy with, but here are some of my thoughts:

  • runtime_zone
  • zone_before_pause

Any other ideas? @agrare @Fryguy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

original_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 also considered original_zone or last_zone, but really this is something specific to pausing an ems so I would rather make that obvious from the attribute name.

Spoke about it for a bit with @jrafanie, our "Master Namer of Things" and we came up with zone_when_paused. How about that?

Copy link
Member

Choose a reason for hiding this comment

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

Naming 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.

Don't want to bikeshed too much about the name, but zone_when_paused would be the maintenance zone, I'd think zone_before_pause would be better than zone_when_paused

@carbonin
Copy link
Member

Generally, I'm with @bdunne. This looks like a much better approach than a rename. I'll 👍 if we can come up with a better name for the backup_zone column.

@slemrmartin
Copy link
Contributor Author

@slemrmartin what does the corresponding core PR look like if the maintenance zone name isn't constant?
e.g. here https://github.com/ManageIQ/manageiq/pull/17452/files#diff-d8c450b508868941c2b0c5884e718324R129 where all we know is the maintenance zone name.
Are you going to have to do some query for Zone.where(:visible => :false) and cache that name?

@agrare there can be used Zone.maintenance_zone&.name (which will be changed to find zone by region.maintenance_zone).
Flag "visible" will be used by UI and API and can be more used for hide it from user's eyes.

@slemrmartin
Copy link
Contributor Author

slemrmartin commented Sep 24, 2018

We should probably implement a Zone.maintenance_zone method that caches on the class

It's is implemented in dependent PR, only has to be changed according to this PR

@agrare
Copy link
Member

agrare commented Sep 24, 2018

@slemrmartin @bdunne 👍

@miq-bot
Copy link
Member

miq-bot commented Sep 24, 2018

Checked commits slemrmartin/manageiq-schema@a73880e~...eab842d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

db/migrate/20180618084054_init_zones_visibility.rb

@agrare
Copy link
Member

agrare commented Sep 24, 2018

LGTM, @carbonin / @bdunne thoughts?

@carbonin carbonin self-assigned this Sep 24, 2018
@carbonin carbonin merged commit 32838a4 into ManageIQ:master Sep 24, 2018
@carbonin carbonin added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 24, 2018
@slemrmartin slemrmartin deleted the suspend-provider-region-zone branch September 24, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants