drop migration from models

[kbrock]

Why are we talking about this?

I am in the process of making providers consistent so we can remove edge cases when zone is saved in the maintenance
state. #20676

What does this code do?

The code looks for cloud volumes and updates their value based upon the ems.
This seems like a data migration that would be better suited in db:migrations

This is an issue because...

How do we have cloud volumes associated with our ems when we
are in the process of creating our ems and providers in the first place?
This seems like an issue
that would only be seen in a development environment as we iterate through code and get these edge cases.

Issue 2?

Also, this child manager is not saved because the way zone is setup there is a validation error. Then the reload drops all the changes to the child record. Not a good situation.

I'm assuming that when this was written it did something, but as the code stands this does nothing, and I don't understand how it can do anything ever based upon point 1.

Issue 3?

The cider_manager references this object (as parent_manager) and needs this object to be saved before it can be saved.
This is all done this object's before create method, so this object (the parent manager) doesn't have an id to go into the child manager. The child manager needs the manager to be saved first. And this needs the child object to be saved to complete before_create and get to the actual create phase. This is cyclical.

Again, why do I care?

I am moving the initialization code into one spot and the saving code into another.

This method bridges both worlds. And since it seems to only be screwing up things with the reload, I'd like to drop it.

[miq-bot]

Checked commit kbrock@af3f688 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[agrare]

@kbrock this looks like a pointless method now if it just calls ensure_cinder_manager right? Nevermind I was thinking we could set name/zone_id/provider_region in ensure_cinder_manager but that wouldn't keep them up to date if they changed

[agrare]

This is a bit of a weird name since it sounds like it is working on multiple cinder managers but only ever works on one.

[kbrock]

this is following some kind of naming convention

[agrare]

Well the convention usually is a top-level ensure_managers calls e.g. ensure_network_manager and ensure_storage_manager

[agrare]

Its a little weird that the CinderManagerMixin provides the ensure_cinder_managers method which calls ensure_cinder_manager but then doesn't define ensure_cinder_manager.

It looks like the caller just has to specify the :type https://github.com/ManageIQ/manageiq-providers-openstack/blob/master/app/models/manageiq/providers/openstack/cloud_manager.rb#L442

Future refactoring, could we just build the cinder_manager in this mixin and allow the including class either define a cinder_manager_klass for the type or build it based off of the included class name?

[agrare]

How do we have cloud volumes associated with our ems when we are in the process of creating our ems and providers in the first place?

If I had to guess this looks like it was supposed to handle the case where a customer has an existing CloudManager with associated cloud_volumes and we wanted to backport the addition of a child StorageManager. The "right way" would have been to do a database migration properly then backport this if we had to, but not leave it in master.

Given this was introduced on fine I don't think we need to add a proper migration for this, worst case cloud_volumes would be deleted from the cloud_manager and added to the storage_manager on the next refresh.

[agrare]

@Fryguy do you have an opinion on doing a data migration for this or not?

[Fryguy]

Generally if the code itself has a migration-at-run-time, I'm of the opinion we create a proper migration and remove the migration-at-run-time code. So, yes?

[agrare]

I believe for that code to work it would have had to be:

1. A customer with an Openstack::CloudManager on euwe after Swift xlink #11622 (comment) was backported
2. Before Add managers in before_create callback instead of before_validation #12878 on fine (when we would no longer create a new cinder manager on an existing cloud manager)

After #12878 the cinder manager would only be created before_create thus there would be no CloudVolumes to "migrate"

[agrare]

Not being able to create a cinder manager on an existing cloud manager without deleting and re-creating feels like its own bug introduced by #12878 though

[kbrock]

Cool. Action items for me:

• Move the swift and cinder migration code to a miq-schema
• define the association with type in openstack for network, cinder, and swift case so we no longer need to specify the class type.
• Move cinder and swift mixins to openstack and subsequently all the ensue methods for both of these

[lpichler]

@kbrock this change causes CI failure in api repo https://travis-ci.com/github/ManageIQ/manageiq-api/jobs/407176144

because manager created by factory here has tenant_id set to nil.

it is working with cinder_manager.save

diff --git a/app/models/mixins/cinder_manager_mixin.rb b/app/models/mixins/cinder_manager_mixin.rb
index 662f624982..eeb283fd85 100644
--- a/app/models/mixins/cinder_manager_mixin.rb
+++ b/app/models/mixins/cinder_manager_mixin.rb
@@ -22,6 +22,7 @@ module CinderManagerMixin
       cinder_manager.name            = "#{name} Cinder Manager"
       cinder_manager.zone_id         = zone_id
       cinder_manager.provider_region = provider_region
+      cinder_manager.save
       true
     end
   end

thanks