Suspend providers #17452

slemrmartin · 2018-05-21T14:04:18Z

Adds ability to suspend provider due to maintenance

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

depends on: Maintenance zone support for suspending provider manageiq-schema#275
dependent: Allow the suspension of cloud, infra and physical infra providers manageiq-ui-classic#4269
dependent: Use the new universal methods for suspending/resuming a provider manageiq-api#434
depends on: EMS validation with maintenance zone specs fix manageiq-providers-ovirt#311

It moves EMS and child managers to Maintenance zone, also sets EMS.enabled to false, which causes stopping provider-specific workers

Cc @Ladas, @agrare, @jerryk55, @romanblanco

app/models/miq_queue.rb

Ladas · 2018-05-22T06:23:20Z

app/models/miq_queue.rb

@@ -206,6 +209,20 @@ def self.submit_job(options)
 put(options)
 end

+ # TODO: Array param not seen yet


@agrare this can be list of any <models_class, id>, right? I wonder if we need to check enabled for each. There might be a better place for it (where the array resources are loaded from the DB)

@Ladas yeah we do that lookup here so maybe you could retry it later here?

Ladas · 2018-05-22T06:25:04Z

app/models/miq_queue.rb

+ if resource.respond_to?(:ext_management_system)
+ ems = resource.ext_management_system
+ if ems.present? && !ems.enabled
+ _log.info("Provider is disabled: #{ems.name}. MiqQueue skipped.")


we should probably log the options too? And we can probably make it log.debug

There is already a method for logging all the queue options properly, so use that one.

Fryguy · 2018-05-24T12:26:47Z

app/models/ext_management_system.rb

@@ -177,6 +177,15 @@ def hostname_format_valid?

 default_value_for :enabled, true

+ def self.enable(ems_id)
+ ExtManagementSystem.where('id = ? OR parent_ems_id = ?', ems_id, ems_id).update_all(enabled: true)


Prefer where(:id => ems_id).or.where(:parent_ems_id => ems_id)

Also, you can leave out the model name just start the line with where(:id => ...

Fryguy · 2018-05-24T12:32:54Z

@jrafanie Please review.

Personally, I'm reacting negatively to modifying any of the queueing code for when a worker is disabled. It feels like the wrong place. I understand how you are trying to avoid filling up the queue when a worker is disabled, but it feels like a sledgehammer approach because there may be things that do have to get to the queue. However, I'm not sure how to solve it directly, otherwise I'd offer an suggestion. :)

slemrmartin · 2018-05-24T13:33:10Z

@Fryguy This is the reason when I make PR in too incomplete state, because I feel it could have side effects. So I'll try to find better places.
I have some starting point where metrics collection starts,
and beginning of smart state process (which is really nice to debug..generic worker -> queue -> generic worker -> queue -> generic worker -> queue.....etc.)
It would be great if someone more experienced tell me more

Ladas · 2018-05-24T14:30:21Z

@Fryguy so question is, what things do have to get to the queue? :-)

I am thinking e.g. retirement? Is that queued by scheduler? Then it has to be somehow postponed, until the provider is started again. Then e.g. metrics collection should be just cancelled (both collection and processing).

If this will not be solved in a generic place, we will need to get a list of tasks we should not queue/cancel/postpone. And this should be probably documented, because there are side effects. (e.g. retirement can be postponed -> cost will be bigger than expected, metrics collection might have a data gap -> charge-back and reports will not be accurate, etc.)

jrafanie · 2018-05-24T15:31:35Z

app/models/miq_queue.rb

@@ -173,6 +173,9 @@ def self.put(options)
 def self.submit_job(options)
 service = options.delete(:service) || "generic"
 resource = options.delete(:affinity)
+
+ return unless resource_enabled?(resource)


I'm confused, what about places that do MiqQueue.put or other callers of .put? Why is submit_job singled out here? I'd think we'd need it everywhere we put something on the queue.

slemrmartin · 2018-05-30T17:20:28Z

Please review.
I'm not sure if travis is failing due to this PR, my local branch is fine and test doesn't seem to be related to this PR

Fryguy · 2018-06-12T15:49:05Z

app/models/ext_management_system.rb

+ # - these workers are not provider specific, but provider-type specific
+ # @param value [Boolean]
+ def change_enabled_with_children!(value)
+ self.enabled = value


I wonder if this is better to handle as an after hook such that after enabled= is modified, then we push down to the children. That way the caller just calls enabled=, then .save!

app/models/ext_management_system.rb

- def enable!
- _log.info("Enabling EMS [#{name}] id [#{id}].")
- update!(:enabled => true)
- end


bdunne · 2018-11-16T22:18:22Z

Have we tested other repos (UI & API repos in particular) with these changes? I wouldn't be surprised if this breaks some tests.

Ladas · 2018-11-19T08:28:39Z

@bdunne @agrare asking for

MiqRegion.seed
Zone.seed

in every spec doesn't make any sense. We should have appliance_setup / server_setup spec helper, that is shared among all specs (in all repos) and calls these things on 1 place. And we shouldn't ask for refactoring like this in a customer issue, there needs to be separate PR for this.

This PR needs to get backported, so it would be best not to change every repo out there. (and AFAIK, we're not backporting cleanups)

slemrmartin · 2018-11-22T09:03:50Z

tested on 14 repos, number of failures decreased to 3, which are solved by ManageIQ/manageiq-providers-ovirt#311 (has to be merged first)

miq-bot · 2018-12-12T12:11:00Z

This pull request is not mergeable. Please rebase and repush.

Enables/Disables ems with children and puts to maintenance zone https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Name as constant

EMS cannot set invisible zone when enabled and change when disabled, MiqServer cannot set invisible zone

It only changes enabled flag.

maintenance zone identified by region and EMS.backup_zone renamed BZ https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Spec and fix for nil zone BZ https://bugzilla.redhat.com/show_bug.cgi?id=1455145

MaintenanceZone uses SecureRandom, Zone.first changed to Zone.default_zone

validation replaces Zone.visible based validations

Avoiding hundreds of errors in dependent repos' specs

miq-bot · 2018-12-13T09:56:16Z

Checked commits slemrmartin/manageiq@352ddb4~...1bd6e0e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
16 files checked, 4 offenses detected

app/models/zone.rb

❗ - Line 11, Col 3 - Rails/HasManyOrHasOneDependent - Specify a :dependent option.
❗ - Line 11, Col 3 - Rails/InverseOf - Specify an :inverse_of option.
❗ - Line 7, Col 3 - Rails/InverseOf - Specify an :inverse_of option.

spec/models/miq_queue_spec.rb

⚠️ - Line 374, Col 7 - Lint/UselessAssignment - Useless assignment to variable - msg.

miq-bot added the wip label May 21, 2018

Ladas reviewed May 22, 2018

View reviewed changes

app/models/miq_queue.rb Outdated Show resolved Hide resolved

Ladas reviewed May 22, 2018

View reviewed changes

Fryguy reviewed May 24, 2018

View reviewed changes

Fryguy assigned jrafanie May 24, 2018

jrafanie reviewed May 24, 2018

View reviewed changes

slemrmartin force-pushed the suspend-workers branch from a79c79a to 0d74660 Compare May 25, 2018 07:29

slemrmartin mentioned this pull request May 29, 2018

Suspend provider #17489

Closed

13 tasks

slemrmartin force-pushed the suspend-workers branch from 410add9 to 67120c5 Compare May 29, 2018 08:37

slemrmartin changed the title ~~[WIP] Suspend provider's workers~~ Suspend provider's workers May 29, 2018

miq-bot removed the wip label May 29, 2018

slemrmartin changed the title ~~Suspend provider's workers~~ [WIP] Suspend provider's workers May 29, 2018

miq-bot added the wip label May 29, 2018

slemrmartin changed the title ~~[WIP] Suspend provider's workers~~ Suspend provider's workers May 30, 2018

miq-bot removed the wip label May 30, 2018

skateman mentioned this pull request Jun 1, 2018

Display providers' suspended status in their quadicon ManageIQ/manageiq-ui-classic#4022

Merged

Fryguy reviewed Jun 12, 2018

View reviewed changes

app/models/ext_management_system.rb

def enable!

_log.info("Enabling EMS [#{name}] id [#{id}].")

update!(:enabled => true)

end

This comment was marked as outdated.

Sign in to view

This was referenced Jun 13, 2018

[WIP] Adding toolbar buttons to suspend a provider ManageIQ/manageiq-ui-classic#4012

Closed

[RFC] Suspending provider ManageIQ/manageiq-design#41

Closed

slemrmartin changed the title ~~Suspend provider's workers~~ [WIP] Suspend provider's workers Jun 18, 2018

slemrmartin changed the title ~~[WIP] Suspend provider's workers~~ [WIP] Suspend providers Jun 18, 2018

miq-bot added the wip label Jun 18, 2018

slemrmartin mentioned this pull request Jun 18, 2018

Migrations for suspending provider ManageIQ/manageiq-schema#222

Closed

1 task

slemrmartin force-pushed the suspend-workers branch from 8a0ce76 to 0470a36 Compare June 18, 2018 15:54

bdunne approved these changes Nov 16, 2018

View reviewed changes

slemrmartin mentioned this pull request Nov 19, 2018

EMS validation with maintenance zone specs fix ManageIQ/manageiq-providers-ovirt#311

Merged

slemrmartin force-pushed the suspend-workers branch from 5f87dbc to 36fe84d Compare November 19, 2018 09:41

miq-bot added the unmergeable label Dec 12, 2018

slemrmartin added 14 commits December 13, 2018 10:14

Pause/Resume EMS

352ddb4

Enables/Disables ems with children and puts to maintenance zone https://bugzilla.redhat.com/show_bug.cgi?id=1455145

Added default_value_for for Zone

d874602

Maintenance zone constant

2365fbd

Name as constant

Disabling MiqQueue for maintenance zone

a970e4f

ExtManagementSystem and MiqServer validations for invisible zone

46cef50

EMS cannot set invisible zone when enabled and change when disabled, MiqServer cannot set invisible zone

Suspending child managers doesn't change zone.

441ee03

It only changes enabled flag.

MiqRegion.maintenance_zone assoc+EMS.zone_when_changed assoc

9675ba5

maintenance zone identified by region and EMS.backup_zone renamed BZ https://bugzilla.redhat.com/show_bug.cgi?id=1455145

MiqQueue.put skip for maintenance_zone

66d638f

Spec and fix for nil zone BZ https://bugzilla.redhat.com/show_bug.cgi?id=1455145

MaintenanceZone random name and fixed Zone.first occurences

32fa469

MaintenanceZone uses SecureRandom, Zone.first changed to Zone.default_zone

MaintenanceZone validation in EMS,MiqServer and create change

4f27a38

validation replaces Zone.visible based validations

Pause/Resume for child managers when pause/resume parent.

ff0f9e9

Zone and EMS code style refactoring

a0d9e00

Strange removing Zone.seed dependency

f448cb8

Maintenance zone better initialization

1bd6e0e

Avoiding hundreds of errors in dependent repos' specs

slemrmartin force-pushed the suspend-workers branch from 36fe84d to 1bd6e0e Compare December 13, 2018 09:51

miq-bot removed the unmergeable label Dec 13, 2018

jrafanie merged commit 5f5b04b into ManageIQ:master Dec 18, 2018

jrafanie added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 18, 2018

slemrmartin deleted the suspend-workers branch January 24, 2019 08:37

Fryguy added hammer/yes? and removed hammer/yes labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suspend providers #17452

Suspend providers #17452

slemrmartin commented May 21, 2018 •

edited

Loading

Ladas May 22, 2018

agrare May 22, 2018

Ladas May 22, 2018

Fryguy May 24, 2018

Fryguy May 24, 2018 •

edited

Loading

Fryguy commented May 24, 2018

slemrmartin commented May 24, 2018

Ladas commented May 24, 2018

jrafanie May 24, 2018

slemrmartin commented May 30, 2018

Fryguy Jun 12, 2018

This comment was marked as outdated.

bdunne commented Nov 16, 2018

Ladas commented Nov 19, 2018

slemrmartin commented Nov 22, 2018 •

edited

Loading

miq-bot commented Dec 12, 2018

miq-bot commented Dec 13, 2018

Suspend providers #17452

Suspend providers #17452

Conversation

slemrmartin commented May 21, 2018 • edited Loading

Ladas May 22, 2018

Choose a reason for hiding this comment

agrare May 22, 2018

Choose a reason for hiding this comment

Ladas May 22, 2018

Choose a reason for hiding this comment

Fryguy May 24, 2018

Choose a reason for hiding this comment

Fryguy May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Fryguy commented May 24, 2018

slemrmartin commented May 24, 2018

Ladas commented May 24, 2018

jrafanie May 24, 2018

Choose a reason for hiding this comment

slemrmartin commented May 30, 2018

Fryguy Jun 12, 2018

Choose a reason for hiding this comment

This comment was marked as outdated.

bdunne commented Nov 16, 2018

Ladas commented Nov 19, 2018

slemrmartin commented Nov 22, 2018 • edited Loading

miq-bot commented Dec 12, 2018

miq-bot commented Dec 13, 2018

slemrmartin commented May 21, 2018 •

edited

Loading

Fryguy May 24, 2018 •

edited

Loading

slemrmartin commented Nov 22, 2018 •

edited

Loading