Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change aggregation mixin methods into virtual attributes #20149

Merged
merged 1 commit into from
May 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions app/models/container_project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ class ContainerProject < ApplicationRecord

include EventMixin
include Metric::CiMixin
include AggregationMixin::Methods

PERF_ROLLUP_CHILDREN = :all_container_groups

Expand All @@ -71,20 +70,16 @@ def perf_rollup_parents(interval_name = nil)
[]
end

# required by aggregate_hardware
alias all_computer_system_ids computer_system_ids
alias all_computer_systems computer_systems

def aggregate_memory(targets = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible to swap with virtual total?
(if not, then that is ok)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

methinks not

aggregate_hardware(:computer_systems, :memory_mb, targets)
Hardware.where(:computer_system => computer_systems).sum(:memory_mb)
end

def aggregate_cpu_speed(targets = nil)
aggregate_hardware(:computer_systems, :cpu_speed, targets)
Hardware.where(:computer_system => computer_systems).sum(:cpu_speed)
end

def aggregate_cpu_total_cores(targets = nil)
aggregate_hardware(:computer_systems, :cpu_total_cores, targets)
Hardware.where(:computer_system => computer_systems).sum(:cpu_total_cores)
end

def disconnect_inv
Expand Down
8 changes: 4 additions & 4 deletions app/models/ems_cluster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ class EmsCluster < ApplicationRecord
has_many :policy_events, -> { order("timestamp") }
has_many :miq_events, :as => :target, :dependent => :destroy
has_many :miq_alert_statuses, :as => :resource, :dependent => :destroy
has_many :host_hardwares, :class_name => 'Hardware', :through => :hosts, :source => :hardware
has_many :vm_hardwares, :class_name => 'Hardware', :through => :vms_and_templates, :source => :hardware
has_many :storages, -> { distinct }, :through => :hosts
has_many :lans, -> { distinct }, :through => :hosts

has_many :switches, -> { distinct }, :through => :hosts

Expand Down Expand Up @@ -51,7 +55,6 @@ class EmsCluster < ApplicationRecord
self.default_relationship_type = "ems_metadata"

include AggregationMixin

include Metric::CiMixin
include MiqPolicyMixin
include AsyncDeleteMixin
Expand Down Expand Up @@ -107,9 +110,6 @@ def total_vcpus
# Relationship methods
#

alias_method :storages, :all_storages
alias_method :datastores, :all_storages # Used by web-services to return datastores as the property name

# Direct Vm relationship methods
def direct_vm_rels
# Look for only the Vms at the second depth (default RP + 1)
Expand Down
2 changes: 2 additions & 0 deletions app/models/ext_management_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ def validate_zone_not_maintenance_when_ems_enabled?
include RelationshipMixin
self.default_relationship_type = "ems_metadata"

has_many :host_hardwares, :class_name => 'Hardware', :through => :hosts, :source => :hardware
d-m-u marked this conversation as resolved.
Show resolved Hide resolved
has_many :vm_hardwares, :class_name => 'Hardware', :through => :vms_and_templates, :source => :hardware
include AggregationMixin

include AuthenticationMixin
Expand Down
9 changes: 6 additions & 3 deletions app/models/host.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,18 @@ class Host < ApplicationRecord
:inverse_of => :host
has_many :host_aggregate_hosts, :dependent => :destroy
has_many :host_aggregates, :through => :host_aggregate_hosts
has_many :host_hardwares, :class_name => 'Hardware', :source => :hardware, :dependent => :nullify
has_many :vm_hardwares, :class_name => 'Hardware', :through => :vms_and_templates, :source => :hardware
has_one :conversion_host, :as => :resource, :dependent => :destroy, :inverse_of => :resource

# Physical server reference
belongs_to :physical_server, :inverse_of => :host

serialize :settings, Hash

deprecate_attribute :address, :hostname
alias_attribute :state, :power_state
alias_attribute :to_s, :name
deprecate_attribute :address, :hostname
alias_attribute :state, :power_state
alias_attribute :to_s, :name

include ProviderObjectMixin
include EventMixin
Expand Down Expand Up @@ -170,6 +172,7 @@ class Host < ApplicationRecord
include AsyncDeleteMixin
include ComplianceMixin
include AvailabilityMixin
include AggregationMixin

before_create :make_smart
after_save :process_events
Expand Down
13 changes: 1 addition & 12 deletions app/models/manageiq/providers/container_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ContainerManager < BaseManager
has_many :container_quota_items, :through => :container_quotas
has_many :container_limit_items, :through => :container_limits
has_many :container_template_parameters, :through => :container_templates
has_many :computer_system_hardwares, :class_name => 'Hardware', :through => :computer_systems, :source => :hardware

# Archived and active entities to destroy when the container manager is deleted
has_many :all_containers, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "Container"
Expand Down Expand Up @@ -67,18 +68,6 @@ def supports_metrics?
true
end

# required by aggregate_hardware
alias :all_computer_systems :computer_systems
alias :all_computer_system_ids :computer_system_ids
d-m-u marked this conversation as resolved.
Show resolved Hide resolved

def aggregate_cpu_total_cores(targets = nil)
d-m-u marked this conversation as resolved.
Show resolved Hide resolved
aggregate_hardware(:computer_systems, :cpu_total_cores, targets)
end

def aggregate_memory(targets = nil)
aggregate_hardware(:computer_systems, :memory_mb, targets)
end

class << model_name
define_method(:route_key) { "ems_containers" }
define_method(:singular_route_key) { "ems_container" }
Expand Down
2 changes: 0 additions & 2 deletions app/models/miq_enterprise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ class MiqEnterprise < ApplicationRecord
acts_as_miq_taggable

include SupportsFeatureMixin
include AggregationMixin
kbrock marked this conversation as resolved.
Show resolved Hide resolved

include MiqPolicyMixin
include Metric::CiMixin

Expand Down
1 change: 0 additions & 1 deletion app/models/miq_region.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ class MiqRegion < ApplicationRecord
acts_as_miq_taggable
include UuidMixin
include NamingSequenceMixin
include AggregationMixin
include ConfigurationManagementMixin

include MiqPolicyMixin
Expand Down
4 changes: 2 additions & 2 deletions app/models/miq_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -491,15 +491,15 @@ def self.managed_resources
{
:vms => Vm.active.count,
:hosts => Host.active.count,
:aggregate_physical_cpus => MiqRegion.my_region.aggregate_physical_cpus(Host.active),
:aggregate_physical_cpus => Host.active.in_my_region.sum(:aggregate_physical_cpus),
d-m-u marked this conversation as resolved.
Show resolved Hide resolved
}
end

def self.unmanaged_resources
{
:vms => Vm.not_active.count,
:hosts => Host.archived.count,
:aggregate_physical_cpus => MiqRegion.my_region.aggregate_physical_cpus(Host.archived),
:aggregate_physical_cpus => Host.archived.in_my_region.sum(:aggregate_physical_cpus),
}
end

Expand Down
25 changes: 7 additions & 18 deletions app/models/mixins/aggregation_mixin.rb
Original file line number Diff line number Diff line change
@@ -1,23 +1,12 @@
module AggregationMixin
extend ActiveSupport::Concern
included do
virtual_column :aggregate_cpu_speed, :type => :integer, :uses => :hosts
virtual_column :aggregate_cpu_total_cores, :type => :integer, :uses => :hosts
virtual_column :aggregate_physical_cpus, :type => :integer, :uses => :hosts
virtual_column :aggregate_memory, :type => :integer, :uses => :hosts
virtual_column :aggregate_vm_cpus, :type => :integer, :uses => :vms_and_templates
virtual_column :aggregate_vm_memory, :type => :integer, :uses => :vms_and_templates
virtual_column :aggregate_disk_capacity, :type => :integer, :uses => :hosts

alias_method :all_hosts, :hosts
alias_method :all_host_ids, :host_ids
alias_method :all_vms_and_templates, :vms_and_templates
alias_method :all_vm_or_template_ids, :vm_or_template_ids
alias_method :all_vms, :vms
alias_method :all_vm_ids, :vm_ids
alias_method :all_miq_templates, :miq_templates
alias_method :all_miq_template_ids, :miq_template_ids
d-m-u marked this conversation as resolved.
Show resolved Hide resolved

include AggregationMixin::Methods
virtual_aggregate :aggregate_cpu_speed, :host_hardwares, :sum, :aggregate_cpu_speed
NickLaMuro marked this conversation as resolved.
Show resolved Hide resolved
virtual_aggregate :aggregate_cpu_total_cores, :host_hardwares, :sum, :cpu_total_cores
virtual_aggregate :aggregate_disk_capacity, :host_hardwares, :sum, :disk_capacity
virtual_aggregate :aggregate_memory, :host_hardwares, :sum, :memory_mb
virtual_aggregate :aggregate_physical_cpus, :host_hardwares, :sum, :cpu_sockets
virtual_aggregate :aggregate_vm_cpus, :vm_hardwares, :sum, :cpu_sockets
virtual_aggregate :aggregate_vm_memory, :vm_hardwares, :sum, :memory_mb
end
end
55 changes: 0 additions & 55 deletions app/models/mixins/aggregation_mixin/methods.rb

This file was deleted.

2 changes: 0 additions & 2 deletions app/models/mixins/relationships_aggregation_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ module RelationshipsAggregationMixin
virtual_column :aggregate_vm_cpus, :type => :integer, :uses => :all_relationships
virtual_column :aggregate_vm_memory, :type => :integer, :uses => :all_relationships
virtual_column :aggregate_disk_capacity, :type => :integer, :uses => :all_relationships

include AggregationMixin::Methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like by deleting this file, two files now have lost the functionality of this module:

app/models/resource_pool.rb:15:11:  include RelationshipsAggregationMixin
app/models/ems_folder.rb:13:11:  include RelationshipsAggregationMixin

And don't seem to be addressed by this PR. Is that a problem or by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of them have the relationships necessary to make any of the aggregate methods work, so no loss there. The Default implementations which can be overridden with something more optimized I'm not sure about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by a Keenan 👍 I assume he was going with, we're okay here anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this unresolved in case it comes up in the future as a problem, so it is more easy for it to be noticed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following error shows on current master when clicking on a resource pool, trying to render its textual summary:

FATAL -- : Error caught: [ActionView::Template::Error] missing attribute: aggregate_cpu_speed
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activerecord-5.2.4.4/lib/active_record/attribute_methods/read.rb:77:in `block in _read_attribute'
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activemodel-5.2.4.4/lib/active_model/attribute_set.rb:48:in `block in fetch_value'
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activemodel-5.2.4.4/lib/active_model/attribute.rb:222:in `value'
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activemodel-5.2.4.4/lib/active_model/attribute_set.rb:48:in `fetch_value'
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activerecord-5.2.4.4/lib/active_record/attribute_methods/read.rb:77:in `_read_attribute'
.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/activerecord-5.2.4.4/lib/active_record/attribute_methods/read.rb:40:in `__temp__167676275676164756f5360757f53707565646'
manageiq-ui-classic/app/helpers/resource_pool_helper/textual_summary.rb:49:in `textual_aggregate_cpu_speed'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

# Default implementations which can be overridden with something more optimized
Expand Down
2 changes: 2 additions & 0 deletions app/models/zone.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ class Zone < ApplicationRecord
has_many :container_groups, :through => :container_managers
has_many :container_replicators, :through => :container_managers
has_many :containers, :through => :container_managers
has_many :host_hardwares, :class_name => 'Hardware', :through => :hosts, :source => :hardware
has_many :vm_hardwares, :class_name => 'Hardware', :through => :vms_and_templates, :source => :hardware
virtual_has_many :active_miq_servers, :class_name => "MiqServer"

before_destroy :remove_servers_if_podified
Expand Down
85 changes: 49 additions & 36 deletions spec/models/ems_cluster_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
RSpec.describe EmsCluster do
include_examples "AggregationMixin"
context("VMware") do
before do
@cluster = FactoryBot.create(:ems_cluster)
Expand Down Expand Up @@ -37,16 +38,10 @@
it('#direct_miq_template_ids') { expect(@cluster.direct_miq_template_ids).to match_array [@template1.id, @template2.id] }
it('#total_direct_miq_templates') { expect(@cluster.total_direct_miq_templates).to eq(2) }

it('#all_vms_and_templates') { expect(@cluster.all_vms_and_templates).to match_array [@vm1, @vm2, @template1, @template2] }
it('#all_vm_or_template_ids') { expect(@cluster.all_vm_or_template_ids).to match_array [@vm1.id, @vm2.id, @template1.id, @template2.id] }
it('#total_vms_and_templates') { expect(@cluster.total_vms_and_templates).to eq(4) }

it('#all_vms') { expect(@cluster.all_vms).to match_array [@vm1, @vm2] }
it('#all_vm_ids') { expect(@cluster.all_vm_ids).to match_array [@vm1.id, @vm2.id] }
it('#total_vms') { expect(@cluster.total_vms).to eq(2) }

it('#all_miq_templates') { expect(@cluster.all_miq_templates).to match_array [@template1, @template2] }
it('#all_miq_template_ids') { expect(@cluster.all_miq_template_ids).to match_array [@template1.id, @template2.id] }
it('#total_miq_templates') { expect(@cluster.total_miq_templates).to eq(2) }

it('ResourcePool#v_direct_vms') { expect(@rp1.v_direct_vms).to eq(1) }
Expand All @@ -55,7 +50,6 @@
it('ResourcePool#v_direct_miq_templates') { expect(@rp1.v_direct_vms).to eq(1) }
it('ResourcePool#v_total_miq_templates') { expect(@rp1.v_total_vms).to eq(2) }
it('#hosts') { expect(@cluster.hosts).to match_array [@host1, @host2] }
it('#all_hosts') { expect(@cluster.all_hosts).to match_array [@host1, @host2] }
it('#total_hosts') { expect(@cluster.total_hosts).to eq(2) }
end

Expand Down Expand Up @@ -98,44 +92,63 @@
it('#direct_miq_template_ids') { expect(@cluster.direct_miq_template_ids).to match_array [@template1.id, @template2.id] }
it('#total_direct_miq_templates') { expect(@cluster.total_direct_miq_templates).to eq(2) }

it('#all_vms_and_templates') { expect(@cluster.all_vms_and_templates).to match_array [@vm1, @vm2, @template1, @template2] }
it('#all_vm_or_template_ids') { expect(@cluster.all_vm_or_template_ids).to match_array [@vm1.id, @vm2.id, @template1.id, @template2.id] }
it('#total_vms_and_templates') { expect(@cluster.total_vms_and_templates).to eq(4) }

it('#all_vms') { expect(@cluster.all_vms).to match_array [@vm1, @vm2] }
it('#all_vm_ids') { expect(@cluster.all_vm_ids).to match_array [@vm1.id, @vm2.id] }
it('#total_vms') { expect(@cluster.total_vms).to eq(2) }

it('#all_miq_templates') { expect(@cluster.all_miq_templates).to match_array [@template1, @template2] }
it('#all_miq_template_ids') { expect(@cluster.all_miq_template_ids).to match_array [@template1.id, @template2.id] }
it('#total_miq_templates') { expect(@cluster.total_miq_templates).to eq(2) }
it('#hosts') { expect(@cluster.hosts).to match_array [@host1, @host2] }
it('#all_hosts') { expect(@cluster.all_hosts).to match_array [@host1, @host2] }
it('#total_hosts') { expect(@cluster.total_hosts).to eq(2) }
end

it "#save_drift_state" do
# TODO: Beef up with more data
cluster = FactoryBot.create(:ems_cluster)
cluster.save_drift_state

expect(cluster.drift_states.size).to eq(1)
expect(DriftState.count).to eq(1)

expect(cluster.drift_states.first.data).to eq({
:aggregate_cpu_speed => 0,
:aggregate_cpu_total_cores => 0,
:aggregate_memory => 0,
:aggregate_physical_cpus => 0,
:aggregate_vm_cpus => 0,
:aggregate_vm_memory => 0,
:class => "EmsCluster",
:id => cluster.id,
:name => cluster.name,
:vms => [],
:miq_templates => [],
:hosts => [],
})
context("#save_drift_state") do
it "without aggregate data" do
# TODO: Beef up with more data
cluster = FactoryBot.create(:ems_cluster)
cluster.save_drift_state

expect(cluster.drift_states.size).to eq(1)
expect(DriftState.count).to eq(1)

expect(cluster.drift_states.first.data).to eq(
:class => "EmsCluster",
:id => cluster.id,
:name => cluster.name,
:vms => [],
:miq_templates => [],
:hosts => []
)
end

it "with aggregate data" do
cluster = FactoryBot.create(:ems_cluster)
host = FactoryBot.create(:host,
:ems_cluster => cluster,
:ext_management_system => FactoryBot.create(:ext_management_system),
:hardware => FactoryBot.build(:hardware,
:cpu_total_cores => 4,
:cpu_speed => 1000,
:memory_mb => 2_048))

vm = FactoryBot.create(:vm_redhat, :host => host, :ems_cluster => cluster)

cluster.save_drift_state

expect(cluster.drift_states.size).to eq(1)
expect(DriftState.count).to eq(1)
expect(cluster.drift_states.first.data).to eq(
:aggregate_cpu_speed => 4000,
:aggregate_cpu_total_cores => 4,
:aggregate_memory => 2048,
:aggregate_physical_cpus => 1,
:class => "EmsCluster",
:id => cluster.id,
:name => cluster.name,
:vms => [{:class => "ManageIQ::Providers::Redhat::InfraManager::Vm", :id => vm.id}],
:miq_templates => [],
:hosts => [{:class => "Host", :id => host.id}]
)
end
end

context("#perf_capture_enabled_host_ids=") do
Expand Down
1 change: 1 addition & 0 deletions spec/models/ext_management_system_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
RSpec.describe ExtManagementSystem do
include_examples "AggregationMixin"
describe ".with_tenant" do
# tenant_root
# \___ tenant_eye_bee_em (service_template_eye_bee_em)
Expand Down
Loading