Consolidate Openstack refresh workers #16465
Conversation
| @@ -47,7 +47,6 @@ | |||
| "ManageIQ::Providers::Openstack::InfraManager::RefreshWorker" => %i(manageiq_default), | |||
| "ManageIQ::Providers::Openstack::NetworkManager::EventCatcher" => %i(manageiq_default), | |||
| "ManageIQ::Providers::Openstack::NetworkManager::MetricsCollectorWorker" => %i(manageiq_default), | |||
| "ManageIQ::Providers::Openstack::NetworkManager::RefreshWorker" => %i(manageiq_default), | |||
There was a problem hiding this comment.
we should probably do the same for "ManageIQ::Providers::StorageManager::CinderManager::RefreshWorker" ? Or do we support that as a separate manager now?
There was a problem hiding this comment.
hm also for ManageIQ::Providers::StorageManager::SwiftManager::RefreshWorker, since all of those are returned as .child_managers, right?
There was a problem hiding this comment.
They probably should be run in the same worker since I think the only use case we support right now is as part of an Openstack provider afaik, but I know that Cinder and Swift have been kept separate for some reason.
There was a problem hiding this comment.
right, so right now, in the OpenStack PR, you will be doing all the work of .child_managers in that 1 worker. So the other workers need to be removed.
We will need to revisit that, once we have separate Swift (SwiftStack) and Cinder (there was something like CinderStack in progress, like year ago?) managers
There was a problem hiding this comment.
As far as I know there's no way to add Swift or Cinder as stand alone providers, and I'm not aware of any other provider other than the Openstack Cloud Provider that they're used as part of in ManageIQ. @Ladas might want to confirm that though.
There was a problem hiding this comment.
The plan was to run them separately at some point. But looking at:
https://github.com/Ladas/manageiq/blob/742b0d68bf0d0a251fe226f81fde719dfac1086a/app/models/manageiq/providers/storage_manager/cinder_manager.rb#L31
and
https://github.com/Ladas/manageiq/blob/742b0d68bf0d0a251fe226f81fde719dfac1086a/app/models/manageiq/providers/storage_manager/swift_manager.rb#L27
It seems like the parent manager is still a requirement
There was a problem hiding this comment.
If we're going to tie these to Openstack I wonder if we should just move them to the manageiq-providers-openstack repo? @Fryguy ?
There was a problem hiding this comment.
@agrare I am not sure why those are not under OpenStack tbh.
There was a problem hiding this comment.
Okay if we end up supporting these as standalone we can change the combined_managers to exclude storage managers
|
Checked commits mansam/manageiq@beaf2cd~...a986e84 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Kicking the tests |
|
@mansam I'm going to mark this |
|
@agrare I think we want this as gaprindashvili/yes and blocker BZ, the side effect of this is avoiding duplication with targeted refresh (same use case as AWS) @mansam can you create the blocker BZ an run it through Loic? The problem here is that both CloudManager targeted refresh and Network Manager full refresh will create network models (so 2 workers). This can lead to duplication since we do not have the DB unique indexes in place. |
|
@agrare @Ladas Even beyond the duplication issue, it fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486657 |
|
Okay cool can you add that to the main PR description @mansam ? |
…orkers Consolidate Openstack refresh workers (cherry picked from commit 8c9200d) https://bugzilla.redhat.com/show_bug.cgi?id=1524590
|
Gaprindashvili backport details: |
Works in concert with ManageIQ/manageiq-providers-openstack#154 to do all the Openstack refreshing on one worker.
@Ladas
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486657