Fix `ChargebackVm.extra_resources_without_rollups` hard-coding SCVMM VMs #22288

agrare · 2023-01-04T18:23:20Z

See also:

Use supports in more instances #21989

The .extra_resources_without_rollups method is checking for @options.include_metrics and adding in SCVMM type VMs because they do not support metrics. This a. hard-codes a plugin class and b. ignores other types which do not support metrics capture.

I suspect at the time SCVMM was the only VM provider which didn't support metrics, but that assumption is no longer true.

Depends on:

app/models/chargeback_vm.rb

agrare · 2023-01-04T18:25:53Z

app/models/vm_or_template.rb

+
+  # when looking at supported features, only look at the classes permitted to be used
+  singleton_class.send(:alias_method, :supported_subclasses, :concrete_subclasses)
+


Thinking this should probably go in a mixin or ApplicationRecord since it is common for all classes which could use ActsAsStiLeafClass

+1 for mixin, but I'm curious what it would look like. Definitely for a separate PR.

agrare · 2023-01-04T18:38:16Z

app/models/metric/ci_mixin.rb

-    supports :capture do
-      metrics_capture_klass = "#{self.class.module_parent.name}::MetricsCapture".safe_constantize
-      unless metrics_capture_klass&.method_defined?(:perf_collect_metrics)
-        _('This provider does not support metrics collection')
-      end


NOTE since this CiMixin is included in base classes like ::VmOrTemplate and ::Host so every subclass would report that it supported capture, even if it didn't. You had to check each instance in order to tell if the class supported metrics.

app/models/metric/ci_mixin.rb

agrare · 2023-01-04T19:49:54Z

app/models/metric/ci_mixin.rb

-        _('This provider does not support metrics collection')
-      end
-    end
+    supports_not :capture


Going to switch this to supports_not at the base classes like we do for other features, and supports :capture only in classes that support it.

These are the models which include Metric::CiMixin

app/models/miq_region.rb app/models/service.rb app/models/zone.rb app/models/storage.rb app/models/ext_management_system.rb app/models/host.rb app/models/vm_or_template.rb app/models/availability_zone.rb app/models/container.rb app/models/container_group.rb app/models/container_image.rb app/models/container_node.rb app/models/container_project.rb app/models/container_replicator.rb app/models/container_service.rb app/models/ems_cluster.rb app/models/host_aggregate.rb app/models/miq_enterprise.rb

And this is the full list of classes we need to add supports :capture to (some of these actually not like ManageIQ::Providers::Vmware::InfraManager::OvfService)

ci_mixin_classes.flat_map do |klass| ?> klass.descendants.select { |k| klass_name = "#{k.module_parent}::MetricsCapture"; klass_name.safe_constantize&.name == klass_name } >> end.map(&:name) => ["ManageIQ::Providers::Vmware::InfraManager::OvfService", "ManageIQ::Providers::Vmware::InfraManager::Storage", "ManageIQ::Providers::Ovirt::InfraManager::Storage", "ManageIQ::Providers::Ovirt::InfraManager::IsoDatastore", "ManageIQ::Providers::Redhat::InfraManager::Storage", "ManageIQ::Providers::Redhat::InfraManager::IsoDatastore", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Storage", "ManageIQ::Providers::IbmPowerHmc::InfraManager::MediaRepository", "ManageIQ::Providers::Vmware::InfraManager::Host", "ManageIQ::Providers::Vmware::InfraManager::HostEsx", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Host", "ManageIQ::Providers::Ovirt::InfraManager::Host", "ManageIQ::Providers::Redhat::InfraManager::Host", "ManageIQ::Providers::Openstack::InfraManager::Host", "ManageIQ::Providers::Vmware::InfraManager::Vm", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Vm", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Lpar", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Vios", "ManageIQ::Providers::Ovirt::InfraManager::Vm", "ManageIQ::Providers::Redhat::InfraManager::Vm", "ManageIQ::Providers::Amazon::CloudManager::Vm", "ManageIQ::Providers::Azure::CloudManager::Vm", "ManageIQ::Providers::AzureStack::CloudManager::Vm", "ManageIQ::Providers::Google::CloudManager::Vm", "ManageIQ::Providers::IbmCloud::VPC::CloudManager::Vm", "ManageIQ::Providers::Openstack::CloudManager::Vm", "ManageIQ::Providers::IbmCic::CloudManager::Vm", "ManageIQ::Providers::IbmPowerVc::CloudManager::Vm", "ManageIQ::Providers::OracleCloud::CloudManager::Vm", "ManageIQ::Providers::Vmware::InfraManager::Template", "ManageIQ::Providers::IbmPowerHmc::InfraManager::Template", "ManageIQ::Providers::Ovirt::InfraManager::Template", "ManageIQ::Providers::Redhat::InfraManager::Template", "ManageIQ::Providers::Openstack::InfraManager::Template", "ManageIQ::Providers::Amazon::CloudManager::Template", "ManageIQ::Providers::Azure::CloudManager::Template", "ManageIQ::Providers::Google::CloudManager::Template", "ManageIQ::Providers::IbmCloud::VPC::CloudManager::Template", "ManageIQ::Providers::Openstack::CloudManager::BaseTemplate", "ManageIQ::Providers::Openstack::CloudManager::Template", "ManageIQ::Providers::IbmCic::CloudManager::Template", "ManageIQ::Providers::IbmPowerVc::CloudManager::Template", "ManageIQ::Providers::Openstack::CloudManager::VolumeSnapshotTemplate", "ManageIQ::Providers::Openstack::CloudManager::VolumeTemplate", "ManageIQ::Providers::OracleCloud::CloudManager::Template", "ManageIQ::Providers::Amazon::CloudManager::AvailabilityZone", "ManageIQ::Providers::Azure::CloudManager::AvailabilityZone", "ManageIQ::Providers::AzureStack::CloudManager::AvailabilityZone", "ManageIQ::Providers::Google::CloudManager::AvailabilityZone", "ManageIQ::Providers::IbmCloud::VPC::CloudManager::AvailabilityZone", "ManageIQ::Providers::Openstack::CloudManager::AvailabilityZone", "ManageIQ::Providers::Openstack::CloudManager::AvailabilityZoneNull", "ManageIQ::Providers::IbmCic::CloudManager::AvailabilityZone", "ManageIQ::Providers::IbmCic::CloudManager::AvailabilityZoneNull", "ManageIQ::Providers::IbmPowerVc::CloudManager::AvailabilityZone", "ManageIQ::Providers::IbmPowerVc::CloudManager::AvailabilityZoneNull", "ManageIQ::Providers::OracleCloud::CloudManager::AvailabilityZone", "ManageIQ::Providers::Kubernetes::ContainerManager::Container", "ManageIQ::Providers::IbmCloud::ContainerManager::Container", "ManageIQ::Providers::Kubernetes::ContainerManager::ContainerGroup", "ManageIQ::Providers::IbmCloud::ContainerManager::ContainerGroup", "ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode", "ManageIQ::Providers::IbmCloud::ContainerManager::ContainerNode", "ManageIQ::Providers::Vmware::InfraManager::Cluster", "ManageIQ::Providers::Ovirt::InfraManager::Cluster", "ManageIQ::Providers::Redhat::InfraManager::Cluster", "ManageIQ::Providers::Openstack::InfraManager::Cluster", "ManageIQ::Providers::Openstack::CloudManager::HostAggregate", "ManageIQ::Providers::IbmCic::CloudManager::HostAggregate", "ManageIQ::Providers::IbmPowerVc::CloudManager::HostAggregate"]

Total aside, but capture is kind of a terrible name for metrics collection We should consider a different name, like :metrics_capture or even just :metrics

Yeah I like just supports :metrics, we already have simply supports :events

agrare · 2023-01-05T19:09:36Z

From Pull Request: ManageIQ/manageiq#22288

miq-bot · 2023-01-09T22:20:06Z

Checked commits agrare/manageiq@00e4a12~...392c5cb with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare · 2023-01-09T23:27:11Z

@miq-bot cross-repo-tests manageiq-api, manageiq-ui-classic

From Pull Request: ManageIQ/manageiq#22288

agrare · 2023-01-09T23:45:59Z

Cross-repo is "green" on UI/API (only failures are the usual authentication_spec errors we always see on -api)

agrare · 2023-01-10T20:16:01Z

@Fryguy this should be good to go if you can take a look

agrare requested review from Fryguy and kbrock as code owners January 4, 2023 18:23

agrare added bug wip labels Jan 4, 2023

agrare commented Jan 4, 2023

View reviewed changes

app/models/chargeback_vm.rb Show resolved Hide resolved

agrare commented Jan 4, 2023

View reviewed changes

app/models/metric/ci_mixin.rb Outdated Show resolved Hide resolved

agrare mentioned this pull request Jan 4, 2023

Drop SCVMM provider #22279

Merged

6 tasks

agrare force-pushed the fix_chargeback_vm_extra_resources_without_rollups branch from ec813f3 to 47c056a Compare January 4, 2023 19:32

Fryguy self-assigned this Jan 4, 2023

agrare commented Jan 4, 2023

View reviewed changes

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 5, 2023

Running tests for agrare

7d76d32

From Pull Request: ManageIQ/manageiq#22288

miq-bot mentioned this pull request Jan 5, 2023

[BOT] Cross repo test for ManageIQ/manageiq#22288 ManageIQ/manageiq-cross_repo-tests#730

Closed

agrare force-pushed the fix_chargeback_vm_extra_resources_without_rollups branch from f4512ef to 4c1377e Compare January 5, 2023 21:38

agrare closed this Jan 9, 2023

agrare reopened this Jan 9, 2023

agrare added 4 commits January 9, 2023 17:11

Use VMs not supporting capture instead of SCVMM

00e4a12

Fix supports :capture true for all classes

ed2a114

Add supports :capture to provider generator

ae012b6

Add comment about chargeback_vm scope / AR relation

392c5cb

agrare force-pushed the fix_chargeback_vm_extra_resources_without_rollups branch from 24fc227 to 392c5cb Compare January 9, 2023 22:11

Fryguy approved these changes Jan 9, 2023

View reviewed changes

agrare changed the title ~~[WIP] Fix ChargebackVm.extra_resources_without_rollups hard-coding SCVMM VMs~~ Fix ChargebackVm.extra_resources_without_rollups hard-coding SCVMM VMs Jan 9, 2023

miq-bot removed the wip label Jan 9, 2023

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 9, 2023

Running tests for agrare

5891538

From Pull Request: ManageIQ/manageiq#22288

miq-bot mentioned this pull request Jan 9, 2023

[BOT] Cross repo test for ManageIQ/manageiq#22288 ManageIQ/manageiq-cross_repo-tests#731

Closed

Fryguy merged commit 3fd9571 into ManageIQ:master Jan 10, 2023

agrare deleted the fix_chargeback_vm_extra_resources_without_rollups branch January 10, 2023 21:07

kbrock mentioned this pull request Feb 9, 2024

Use supports in more instances #21989

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ChargebackVm.extra_resources_without_rollups` hard-coding SCVMM VMs #22288

Fix `ChargebackVm.extra_resources_without_rollups` hard-coding SCVMM VMs #22288

agrare commented Jan 4, 2023 •

edited by kbrock

Loading

agrare Jan 4, 2023

Fryguy Jan 4, 2023

agrare Jan 4, 2023

agrare Jan 4, 2023 •

edited

Loading

Fryguy Jan 4, 2023

agrare Jan 4, 2023

agrare commented Jan 5, 2023

miq-bot commented Jan 9, 2023

agrare commented Jan 9, 2023

agrare commented Jan 9, 2023

agrare commented Jan 10, 2023


		# when looking at supported features, only look at the classes permitted to be used
		singleton_class.send(:alias_method, :supported_subclasses, :concrete_subclasses)

Fix ChargebackVm.extra_resources_without_rollups hard-coding SCVMM VMs #22288

Fix ChargebackVm.extra_resources_without_rollups hard-coding SCVMM VMs #22288

Conversation

agrare commented Jan 4, 2023 • edited by kbrock Loading

agrare Jan 4, 2023

Choose a reason for hiding this comment

Fryguy Jan 4, 2023

Choose a reason for hiding this comment

agrare Jan 4, 2023

Choose a reason for hiding this comment

agrare Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Fryguy Jan 4, 2023

Choose a reason for hiding this comment

agrare Jan 4, 2023

Choose a reason for hiding this comment

agrare commented Jan 5, 2023

miq-bot commented Jan 9, 2023

agrare commented Jan 9, 2023

agrare commented Jan 9, 2023

agrare commented Jan 10, 2023

Fix `ChargebackVm.extra_resources_without_rollups` hard-coding SCVMM VMs #22288

Fix `ChargebackVm.extra_resources_without_rollups` hard-coding SCVMM VMs #22288

agrare commented Jan 4, 2023 •

edited by kbrock

Loading

agrare Jan 4, 2023 •

edited

Loading