From 07cd0ffc33d58e98e08704a32f9f9a98e2e8e9e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= Date: Tue, 8 Nov 2016 09:20:49 +0100 Subject: [PATCH 1/2] Refactor: Move parents_determining_rate to MetricRollup record This will help us to dispart Chargeback calculations and rates cache. --- app/models/chargeback.rb | 6 +- app/models/chargeback_container_image.rb | 5 -- app/models/chargeback_container_project.rb | 5 -- app/models/chargeback_vm.rb | 6 -- app/models/metric/chargeback_helper.rb | 11 +++ spec/models/chargeback_vm_spec.rb | 73 ------------------- .../metric_rollup/chargeback_helper_spec.rb | 62 ++++++++++++++++ 7 files changed, 74 insertions(+), 94 deletions(-) create mode 100644 spec/models/metric_rollup/chargeback_helper_spec.rb diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb index d3c7b931990..15bf790a52e 100644 --- a/app/models/chargeback.rb +++ b/app/models/chargeback.rb @@ -131,7 +131,7 @@ def get_rates(perf) prefix = Chargeback.report_cb_model(self.class.name).underscore ChargebackRate.get_assigned_for_target(perf.resource, :tag_list => perf.tag_list_reconstruct.map! { |t| prefix + t }, - :parents => get_rate_parents(perf)) + :parents => perf.parents_determining_rate) end end @@ -189,10 +189,6 @@ def self.report_tag_field "tag_name" end - def self.get_rate_parents - raise "Chargeback: get_rate_parents must be implemented in child class." - end - def self.set_chargeback_report_options(rpt, group_by, header_for_tag, tz) rpt.cols = %w(start_date display_range) diff --git a/app/models/chargeback_container_image.rb b/app/models/chargeback_container_image.rb index 1dea57cb1dd..2bc90877edb 100644 --- a/app/models/chargeback_container_image.rb +++ b/app/models/chargeback_container_image.rb @@ -88,9 +88,4 @@ def self.report_col_options "total_cost" => {:grouping => [:total]} } end - - def get_rate_parents(_perf) - # get rates from image tags only - [] - end end # class ChargebackContainerImage diff --git a/app/models/chargeback_container_project.rb b/app/models/chargeback_container_project.rb index 725adad058c..81b69b7c908 100644 --- a/app/models/chargeback_container_project.rb +++ b/app/models/chargeback_container_project.rb @@ -80,9 +80,4 @@ def self.report_col_options "total_cost" => {:grouping => [:total]} } end - - def get_rate_parents(perf) - # Get rate from assigned containers providers only - [perf.parent_ems].compact - end end # class Chargeback diff --git a/app/models/chargeback_vm.rb b/app/models/chargeback_vm.rb index b1546fbafa5..d4220aff165 100644 --- a/app/models/chargeback_vm.rb +++ b/app/models/chargeback_vm.rb @@ -149,10 +149,4 @@ def self.report_col_options "total_cost" => {:grouping => [:total]} } end - - def get_rate_parents(perf) - @enterprise ||= MiqEnterprise.my_enterprise - parents = perf.resource_parents + [@enterprise] - parents.compact - end end # class Chargeback diff --git a/app/models/metric/chargeback_helper.rb b/app/models/metric/chargeback_helper.rb index 1f6aebbd68e..fb25e9a2055 100644 --- a/app/models/metric/chargeback_helper.rb +++ b/app/models/metric/chargeback_helper.rb @@ -24,4 +24,15 @@ def resource_parents resource.try(:tenant) ].compact end + + def parents_determining_rate + case resource_type + when VmOrTemplate.name + (resource_parents + [MiqEnterprise.my_enterprise]).compact + when ContainerProject.name + [parent_ems].compact + when Container.name + [] + end + end end diff --git a/spec/models/chargeback_vm_spec.rb b/spec/models/chargeback_vm_spec.rb index bd0d5c22bce..0ec0ce17627 100644 --- a/spec/models/chargeback_vm_spec.rb +++ b/spec/models/chargeback_vm_spec.rb @@ -50,79 +50,6 @@ Timecop.return end - describe "#get_rate_parents" do - let(:vm) do - FactoryGirl.create(:vm_vmware, :name => "test_vm", :evm_owner => @admin, :ems_ref => "ems_ref", - :ems_cluster => @ems_cluster, :storage => @storage, :host => @host1, - :ext_management_system => @ems - ) - end - - let(:metric_rollup_without_parents) do - FactoryGirl.create(:metric_rollup_vm_hr, - :cpu_usagemhz_rate_average => @cpu_usagemhz_rate, - :derived_vm_numvcpus => @cpu_count, - :derived_memory_available => @memory_available, - :derived_memory_used => @memory_used, - :disk_usage_rate_average => @disk_usage_rate, - :net_usage_rate_average => @net_usage_rate, - :derived_vm_used_disk_storage => @vm_used_disk_storage.gigabytes, - :derived_vm_allocated_disk_storage => @vm_allocated_disk_storage.gigabytes, - :tag_names => "environment/prod", - :resource => vm - ) - end - - let(:metric_rollup_with_parents) do - FactoryGirl.create(:metric_rollup_vm_hr, - :cpu_usagemhz_rate_average => @cpu_usagemhz_rate, - :derived_vm_numvcpus => @cpu_count, - :derived_memory_available => @memory_available, - :derived_memory_used => @memory_used, - :disk_usage_rate_average => @disk_usage_rate, - :net_usage_rate_average => @net_usage_rate, - :derived_vm_used_disk_storage => @vm_used_disk_storage.gigabytes, - :derived_vm_allocated_disk_storage => @vm_allocated_disk_storage.gigabytes, - :tag_names => "environment/prod", - :resource => vm, - :parent_host_id => @host1.id, - :parent_ems_cluster_id => @ems_cluster.id, - :parent_ems_id => @ems.id, - :parent_storage_id => @storage.id, - ) - end - - it "uses resource's host, cluster, storage and ems" do - parents = described_class.new.send(:get_rate_parents, metric_rollup_without_parents) - - expected_array = [ - vm.host, - vm.ems_cluster, - vm.storage, - vm.ext_management_system, - vm.tenant, - MiqEnterprise.my_enterprise - ].compact - - expect(parents).to match_array(expected_array) - end - - it "uses host, cluster, storage ems from MetricRollup record" do - parents = described_class.new.send(:get_rate_parents, metric_rollup_with_parents) - - expected_array = [ - metric_rollup_with_parents.parent_host, - metric_rollup_with_parents.parent_ems_cluster, - metric_rollup_with_parents.parent_storage, - metric_rollup_with_parents.parent_ems, - vm.tenant, - MiqEnterprise.my_enterprise - ].compact - - expect(parents).to match_array(expected_array) - end - end - let(:report_static_fields) { %w(vm_name) } it "uses static fields" do diff --git a/spec/models/metric_rollup/chargeback_helper_spec.rb b/spec/models/metric_rollup/chargeback_helper_spec.rb new file mode 100644 index 00000000000..6da6ad18af6 --- /dev/null +++ b/spec/models/metric_rollup/chargeback_helper_spec.rb @@ -0,0 +1,62 @@ +describe MetricRollup do + describe '#parents_determining_rate' do + before do + MiqRegion.seed + MiqEnterprise.seed + end + + context 'VmOrTemplate' do + let(:ems) { FactoryGirl.build(:ems_vmware) } + let(:ems_cluster) { FactoryGirl.build(:ems_cluster, :ext_management_system => ems) } + let(:storage) { FactoryGirl.build(:storage_target_vmware) } + let(:host) { FactoryGirl.build(:host) } + let(:vm) do + FactoryGirl.create(:vm_vmware, :name => 'test_vm', :ems_ref => 'ems_ref', + :ems_cluster => ems_cluster, :storage => storage, :host => host, + :ext_management_system => ems + ) + end + + subject { metric_rollup.parents_determining_rate } + + context 'metric_rollup record with parents not nil' do + let(:metric_rollup) do + FactoryGirl.build(:metric_rollup_vm_hr, + :resource => vm, + :parent_host => host, + :parent_ems_cluster => ems_cluster, + :parent_ems => ems, + :parent_storage => storage, + ) + end + + let(:parents_from_rollup) do + [ + metric_rollup.parent_host, + metric_rollup.parent_ems_cluster, + metric_rollup.parent_storage, + metric_rollup.parent_ems, + MiqEnterprise.my_enterprise + ] + end + + it { is_expected.to match_array(parents_from_rollup) } + end + + context 'metric_rollup record with parents nil' do + let(:metric_rollup) { FactoryGirl.build(:metric_rollup_vm_hr, :resource => vm) } + let(:parents_from_vm) do + [ + vm.host, + vm.ems_cluster, + vm.storage, + vm.ext_management_system, + MiqEnterprise.my_enterprise + ] + end + + it { is_expected.to match_array(parents_from_vm) } + end + end + end +end From 72e3dfc1163cffa12a31bf56c69315cbd52d6c75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= Date: Tue, 8 Nov 2016 10:45:09 +0100 Subject: [PATCH 2/2] Refactor: Extract rates cache to separate class This will enable us to create more advanced rate selection processes. This will also enable us to cache all the rates in advance --- app/models/chargeback.rb | 15 ++------------- app/models/chargeback/rates_cache.rb | 24 ++++++++++++++++++++++++ spec/models/chargeback_vm_spec.rb | 2 +- 3 files changed, 27 insertions(+), 14 deletions(-) create mode 100644 app/models/chargeback/rates_cache.rb diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb index 15bf790a52e..229f6c36f62 100644 --- a/app/models/chargeback.rb +++ b/app/models/chargeback.rb @@ -21,7 +21,7 @@ def self.build_results_for_report_chargeback(options) _log.info("Calculating chargeback costs...") @options = options = ReportOptions.new_from_h(options) - cb = new + rates = RatesCache.new base_rollup = MetricRollup.includes( :resource => [:hardware, :tenant, :tags, :vim_performance_states, :custom_attributes], @@ -61,7 +61,7 @@ def self.build_results_for_report_chargeback(options) # we need to select ChargebackRates for groups of MetricRollups records # and rates are selected by first MetricRollup record metric_rollup_record = metric_rollup_records.first - rates_to_apply = cb.get_rates(metric_rollup_record) + rates_to_apply = rates.get(metric_rollup_record) # key contains resource_id and timestamp (query_start_time...query_end_time) # extra_fields there some extra field like resource name and @@ -124,17 +124,6 @@ def self.get_tag_keys_and_fields(perf, ts_key) [key, extra_fields] end - def get_rates(perf) - @rates ||= {} - @rates[perf.hash_features_affecting_rate] ||= - begin - prefix = Chargeback.report_cb_model(self.class.name).underscore - ChargebackRate.get_assigned_for_target(perf.resource, - :tag_list => perf.tag_list_reconstruct.map! { |t| prefix + t }, - :parents => perf.parents_determining_rate) - end - end - def calculate_costs(metric_rollup_records, rates, hours_in_interval) chargeback_fields_present = metric_rollup_records.count(&:chargeback_fields_present?) self.fixed_compute_metric = chargeback_fields_present if chargeback_fields_present diff --git a/app/models/chargeback/rates_cache.rb b/app/models/chargeback/rates_cache.rb new file mode 100644 index 00000000000..7424a8e314f --- /dev/null +++ b/app/models/chargeback/rates_cache.rb @@ -0,0 +1,24 @@ +class Chargeback + class RatesCache + def get(perf) + @rates ||= {} + @rates[perf.hash_features_affecting_rate] ||= + begin + prefix = tag_prefix(perf) + ChargebackRate.get_assigned_for_target(perf.resource, + :tag_list => perf.tag_list_reconstruct.map! { |t| prefix + t }, + :parents => perf.parents_determining_rate) + end + end + + private + + def tag_prefix(perf) + case perf.resource_type + when Container.name then 'container_image' + when VmOrTemplate.name then 'vm' + when ContainerProject.name then 'container_project' + end + end + end +end diff --git a/spec/models/chargeback_vm_spec.rb b/spec/models/chargeback_vm_spec.rb index 0ec0ce17627..f63bf3be789 100644 --- a/spec/models/chargeback_vm_spec.rb +++ b/spec/models/chargeback_vm_spec.rb @@ -801,7 +801,7 @@ def used_average_for(metric, hours_in_interval) before do ChargebackRate.set_assignments(:compute, [rate_assignment_options]) - @rate = chargeback_vm.get_rates(metric_rollup).first + @rate = Chargeback::RatesCache.new.get(metric_rollup).first @assigned_rate = ChargebackRate.get_assignments("Compute").first end