Filter relevant fields also according to chargeback class in Chargeback #17414

lpichler · 2018-05-14T09:46:18Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1580543

This bug has been discovered after this fix and unfortunately chargeback report is broken with mentioned PR.

Error msg (during generation report for VMs)

Generating report... Costs2
/Users/liborpichler/manageiq/manageiq/.bundle/ruby/2.4.0/gems/activemodel-5.0.7/lib/active_model/attribute_methods.rb:433:in `method_missing': undefined method `derived_vm_numvcpu_cores' for #<MetricRollup:0x00007fbc0ef7d588> (NoMethodError)
Did you mean?  derived_vm_numvcpus
               derived_vm_numvcpus=
               derived_vm_numvcpus?
               derived_vm_numvcpus_change
               derived_vm_numvcpus_was
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_with_rollups.rb:91:in `collect'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_with_rollups.rb:91:in `values'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_with_rollups.rb:66:in `none?'

Note: derived_vm_numvcpu_cores is related only to chargeback for chargeback for containers.

Why?

Chargeback rates contain chargeback rate details (ChargebackRateDetail) for all types of chargeback (VMs, ContainerProjects, ContainerImages).

In calculation we are cycling thru all rate details independently on chargeback type and then irrelevant fields should be skipped for certain chargeback type. It was basically accidentally skipped before this recent fix and now after the fix, relevant field is ( for examplecpu core allocated ) not skipped for certain chargeback type(example: for Chargeback for VM - even field is related only for chargeback for containers) because of eventual participation on calculation of total_costcolumn done in fix.

Fix
Now column represented by chargeback detail rate has to be present as attribute in related chargeback class.

cc @gtanzillo

@miq-bot assign @jrafanie

@miq-bot add_label gaprindashvili/yes

lpichler · 2018-05-14T09:46:32Z

@miq-bot add_label chargeback

miq-bot · 2018-05-14T09:56:23Z

Checked commit lpichler@213289c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🍰

jrafanie · 2018-05-14T19:36:59Z

app/models/chargeback.rb

@@ -152,7 +152,7 @@ def calculate_costs(consumption, rates)
    self.class.try(:refresh_dynamic_metric_columns)

    rates.each do |rate|
-      rate.rate_details_relevant_to(relevant_fields).each do |r|
+      rate.rate_details_relevant_to(relevant_fields, self.class.attribute_names).each do |r|


It feels like this should be a method... I believe you're doing a set intersection of relevant_fields and self.class.attribute_names) in rate_details_relevant_to below, right?

def calculate_costs(consumption, rates) self.fixed_compute_metric = consumption.chargeback_fields_present if consumption.chargeback_fields_present self.class.try(:refresh_dynamic_metric_columns) rates.each do |rate| rate.rate_details_relevant_to(relevant_fields & self.class.attribute_names).each do |r|

Either that or move the set intersection into the method below.

Unfortunately , it is not same.

Originally,
method rate_details_relevant_to is going thru all rate chargeback details and it is basically asking each rate detail chargeback - Does your cost calculation affect a selected report fields(report_cols) ?

And it will meet also rate detail chargeback with derived_vm_numvcpu_cores.
But if you have only total_cost on report - this total value are affecting all columns - derived_vm_numvcpu_cores included.

But we need to refuse derived_vm_numvcpu_cores for chargeback vm.

Hopefully, you understand better.

And sure we need to do some refactoring here: for example similar condition is also in ChargebackRateDetail#charge:97 method, I will do it in folluw up PR

jrafanie · 2018-05-14T19:37:17Z

app/models/chargeback_rate.rb

-  def rate_details_relevant_to(report_cols)
-    # we can memoize, as we get the same report_cols thrrough the life of the object
-    @relevant ||= chargeback_rate_details.select { |r| r.affects_report_fields(report_cols) }
+  def rate_details_relevant_to(report_cols, allowed_cols)


We probably don't want to change this method signature if we don't have to.

yes we don't want but it is necessary here. But we can improve with some unification because similar condition is also in ChargebackRateDetail#charge:97 method,as I mentioned above.

jrafanie · 2018-05-14T19:37:37Z

app/models/chargeback_rate_detail.rb

@@ -12,7 +12,7 @@ class ChargebackRateDetail < ApplicationRecord

  delegate :rate_type, :to => :chargeback_rate, :allow_nil => true

-  delegate :metric_key, :cost_keys, :rate_key, :to => :chargeable_field
+  delegate :metric_column_key, :metric_key, :cost_keys, :rate_key, :to => :chargeable_field


Wait, we still want to delegate metric_key, don't we?

No, we need it in ChargebackRateDetail#charge method

jrafanie · 2018-05-14T19:37:56Z

app/models/chargeable_field.rb

+  # fixed_compute_metric is used in report and calculations
+  # TODO: remove and unify with metric_key
+  def metric_column_key
+    fixed? ? metric_key.gsub(/\_1|\_2/, '') : metric_key


I'm not sure what this is doing

It is hack,

we have chargeback_rate_details and each detail is representing metric column na cost column.

For example:

�CPU Allocated Cost has metric column: cpu_allocated_metric and cost column cpu_allocated_cost

so when we are checking if cpu_allocated_cost is allowed to report, we are asking here https://github.com/ManageIQ/manageiq/pull/17414/files/213289cd034ea8054bbb9a30c80d229cd776c049#diff-7c998acd3d11c70ed830e216754f4b6bR42
if string cpu_allocated_cost (from selected fields) is related to metric_key which is based from ChargeableField#group and Chargeback#source.

But we have to add exception for fixed rate detail because there are two columns
fixed_compute_1_cost and fixed_compute_1_cost but related metric field is only one: fixed_compute_metric and then method ChargeableField#metric_key is not enough (it would produce fixed_compute_1_metric ) but we need to base check in fixed_compute_metric (some for storage)

so input of this method is fixed_compute_1_metric and it should remove _1 (etc.)

jrafanie · 2018-05-21T18:29:59Z

@lpichler I'll merge this for now to fix the issue. Can you do your suggested improvements in a followup PR? Thanks!

lpichler · 2018-05-21T19:00:56Z

sure @jrafanie, thanks!

Follow UP: ManageIQ#17414

simaishi · 2018-06-26T16:18:46Z

Backported to Gaprindashvili via #17639

Follow UP: ManageIQ#17414

Filter relevant fields also according to chargeback class in Chargeback

213289c

miq-bot assigned jrafanie May 14, 2018

miq-bot added gaprindashvili/yes core/chargeback labels May 14, 2018

jrafanie reviewed May 14, 2018

View reviewed changes

jrafanie added this to the Sprint 86 Ending May 21, 2018 milestone May 21, 2018

jrafanie added the bug label May 21, 2018

jrafanie merged commit 6f61221 into ManageIQ:master May 21, 2018

lpichler deleted the fix_chargeback_column_filtering branch May 21, 2018 19:00

lpichler mentioned this pull request May 22, 2018

Removing guard conditions in Chargeback #17463

Merged

lpichler added a commit to lpichler/manageiq that referenced this pull request May 31, 2018

Fix:Include also sub_metric key in chargeback rate

99abf48

Follow UP: ManageIQ#17414

lpichler mentioned this pull request Jun 26, 2018

[GAPRINDASHVILI] Filter relevant fields also according to chargeback class in Chargeback #17639

Merged

simaishi added blocker gaprindashvili/backported and removed gaprindashvili/yes labels Jun 26, 2018

lpichler added a commit to lpichler/manageiq that referenced this pull request Sep 25, 2018

Fix:Include also sub_metric key in chargeback rate

cb54c7d

Follow UP: ManageIQ#17414

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter relevant fields also according to chargeback class in Chargeback #17414

Filter relevant fields also according to chargeback class in Chargeback #17414

lpichler commented May 14, 2018 •

edited by jrafanie

lpichler commented May 14, 2018

miq-bot commented May 14, 2018

jrafanie May 14, 2018

lpichler May 21, 2018

jrafanie May 14, 2018

lpichler May 21, 2018

jrafanie May 14, 2018

lpichler May 21, 2018

jrafanie May 14, 2018

lpichler May 21, 2018

jrafanie commented May 21, 2018

lpichler commented May 21, 2018

simaishi commented Jun 26, 2018

Filter relevant fields also according to chargeback class in Chargeback #17414

Filter relevant fields also according to chargeback class in Chargeback #17414

Conversation

lpichler commented May 14, 2018 • edited by jrafanie

lpichler commented May 14, 2018

miq-bot commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented May 21, 2018

lpichler commented May 21, 2018

simaishi commented Jun 26, 2018

lpichler commented May 14, 2018 •

edited by jrafanie