manageiq icon indicating copy to clipboard operation
manageiq copied to clipboard

[WIP] Don't include all types in Vm children

Open kbrock opened this issue 7 months ago • 13 comments

Trying out various methods of not including every model in the Vm.all or MiqTemplate.all queries.

Dependes upon:

  • [ ] https://github.com/ManageIQ/manageiq/pull/23608

Before

vmdb(dev)> puts Vm.all.to_sql
SELECT "vms".* FROM "vms"
WHERE "vms"."type" IN ('Vm', 'VmServer',
  'ManageIQ::Providers::PhysicalInfraManager::Vm', 'ManageIQ::Providers::InfraManager::Vm',
  'ManageIQ::Providers::CloudManager::Vm', 'ManageIQ::Providers::CiscoIntersight::PhysicalInfraManager::Vm',
  'ManageIQ::Providers::Vmware::InfraManager::Vm', 'ManageIQ::Providers::Ovirt::InfraManager::Vm',
  'ManageIQ::Providers::Nutanix::InfraManager::Vm', 'ManageIQ::Providers::Kubevirt::InfraManager::Vm',
  'ManageIQ::Providers::IbmPowerHmc::InfraManager::Vm', 'ManageIQ::Providers::Redhat::InfraManager::Vm',
  'ManageIQ::Providers::Openshift::InfraManager::Vm', 'ManageIQ::Providers::IbmPowerHmc::InfraManager::Vios',
  'ManageIQ::Providers::IbmPowerHmc::InfraManager::Lpar', 'ManageIQ::Providers::Vmware::CloudManager::Vm',
  'ManageIQ::Providers::OracleCloud::CloudManager::Vm', 'ManageIQ::Providers::Openstack::CloudManager::Vm',
  'ManageIQ::Providers::IbmCloud::VPC::CloudManager::Vm', 'ManageIQ::Providers::IbmCloud::PowerVirtualServers::CloudManager::Vm',
  'ManageIQ::Providers::Google::CloudManager::Vm', 'ManageIQ::Providers::AzureStack::CloudManager::Vm',
  'ManageIQ::Providers::Azure::CloudManager::Vm', 'ManageIQ::Providers::Amazon::CloudManager::Vm',
  'ManageIQ::Providers::IbmPowerVc::CloudManager::Vm', 'ManageIQ::Providers::IbmCic::CloudManager::Vm')
  AND "vms"."template" = FALSE
=> nil
vmdb(dev)> puts ManageIQ::Providers::Vmware::InfraManager::Vm.all.to_sql
SELECT "vms".* FROM "vms"
WHERE "vms"."type" = 'ManageIQ::Providers::Vmware::InfraManager::Vm'
  AND "vms"."template" = FALSE

After

vmdb(dev)> puts Vm.all.to_sql
SELECT "vms".* FROM "vms"
WHERE "vms"."template" = FALSE
=> nil
vmdb(dev)> puts ManageIQ::Providers::Vmware::InfraManager::Vm.all.to_sql
SELECT "vms".* FROM "vms"
WHERE "vms"."type" = 'ManageIQ::Providers::Vmware::InfraManager::Vm'
  AND "vms"."template" = FALSE

Side Effect

Saving a Vm does not set Vm#type so it comes back as a generic VmOrTemplate.

kbrock avatar Sep 29 '25 16:09 kbrock

[DELETED COMMENT] - I removed this noise and merged into the code (You can see the text in comment history if you want to see the noise, but viewing the next compare is more useful)

kbrock avatar Sep 29 '25 16:09 kbrock

update:

  • added default scope to the experiment
  • changed to using finder_needs_type_condition vs inheritance_column hack.

kbrock avatar Sep 29 '25 17:09 kbrock

update:

  • removed template fixes (it is trivial and not really necessary. Worrying about the type column first)

kbrock avatar Sep 29 '25 17:09 kbrock

FYI: I tried

class Vm
  self.abstract_class = true
end

type = Vm.first.type

type == "Vm"                                             # expected value
type == "ManageIQ::Providers::Vmware::InfraManager::Vm"` # actual   value

This may be due to the factory issue, but guessing not

kbrock avatar Oct 02 '25 22:10 kbrock

update:

  • removed the changes to tests around generic factories.

kbrock avatar Oct 03 '25 13:10 kbrock

WIP: It is having trouble instantiating a class of type Vm.

Think this needs to depend upon us changing the factories to not try and instantiate the theoretically abstract classes (technically, Vm is not abstract)

kbrock avatar Oct 03 '25 20:10 kbrock

update:

  • incorporated Jason's suggestion to cut this down significantly and remove the inheritance hack

update:

  • extracted https://github.com/ManageIQ/manageiq/pull/23608

update:

  • rebase on top of #23608 since it was needed

Now that this code has simplified and we know why the tests were failing, this has changed from a pipe dream to me and is now something I would like.

WIP: only a wip to wait for commit 1 / factories to stop instantiating abstract classes.

kbrock avatar Oct 03 '25 22:10 kbrock

update:

  • fixed miq_template (mimic vm logic using < instead of !=

update:

  • fixed vm (rubocop)

kbrock avatar Oct 07 '25 16:10 kbrock

I'm curious why this depends on https://github.com/ManageIQ/manageiq/pull/23608. We're not actually making Vm and MiqTemplate abstract here.

Fryguy avatar Oct 07 '25 16:10 Fryguy

@Fryguy yea. I thought it was a tangent, pulled it out and the tests started failing.

This change freaks out when you create a vm of type="Vm".

Think it was failing HERE But I can remove the dependency and retry again

self notes

rspec ./spec/models/manageiq/providers/network_manager_spec.rb:8 
rspec ./spec/models/miq_report/generator_spec.rb:71 
rspec ./spec/models/mixins/ci_feature_mixin_spec.rb:4
rspec ./spec/models/mixins/relationship_mixin_spec.rb:437
rspec ./spec/models/mixins/relationship_mixin_spec.rb:446
rspec ./spec/models/mixins/relationship_mixin_spec.rb:408
rspec ./spec/models/mixins/relationship_mixin_spec.rb:419
rspec ./spec/models/vim_performance_state_spec.rb:139

kbrock avatar Oct 07 '25 16:10 kbrock

@Fryguy ok, for a Vm, we stated that it doesn't need STI, so it does not set the type.

kbrock avatar Oct 07 '25 17:10 kbrock

I see, so queries to the base VmOrTemplate fail?

Fryguy avatar Oct 07 '25 18:10 Fryguy

Well, a query on VmOrTemplate will not tack on anything type or template related, so it will work. But if your test creates a Vm or other, then it will not populate type - so those will fail tests. In production, I'm assuming we do not create Vm or VmInfra or ..providers::VmOrTemplate

kbrock avatar Oct 10 '25 17:10 kbrock