Use current tags for filtering resources in chargeback for VMs #17470

lpichler · 2018-05-23T17:47:04Z

2 issues:

selection of rates based on tags
filtering resources to charge them by tags

but We are using MetricRollup#tag_names as tag_names from resources in these two situations and if user will add tags later then related MetricRollup records have still old tags.

So thanks to that it could not choose rate and aslo found any MetricRollup records because in query is used non-current MetricRollup#tag_names.

This fix using current tags and also old tags(in MetricRollups) in for these two places.

Links

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

jrafanie · 2018-05-23T18:43:34Z

app/models/chargeback/consumption_with_rollups.rb

@@ -22,7 +22,7 @@ def hash_features_affecting_rate

    def tag_names
      @tag_names ||= @rollups.inject([]) do |memo, rollup|
-        memo |= rollup.tag_names.split('|') if rollup.tag_names.present?
+        memo |= rollup.resource.tags.collect(&:name) if rollup.resource.tags.collect(&:name).present?


I think I understand this... @lpichler is this capturing the current tag names associated with the resource instead of using the tag names at the time the rollup was created?

yes exactly, but I updated it yet - to use both tags from metric rollups and also current tags.

this change is needed for selecting a rates according to tags.

jrafanie · 2018-05-23T18:44:06Z

app/models/chargeback_vm.rb

-    else
-      scope.where(:resource => vms(region))
-    end
+    scope.where(:resource => vms(region))


I don't understand this change. Why can we remove this?

it was bad touch, now I extended it to posibility filter as according to current tags and also according to old tags.

this change is needed for filtering out resource on report - related to filter tab in report

jrafanie · 2018-05-24T16:01:33Z

app/models/chargeback/consumption_with_rollups.rb

@@ -22,7 +24,11 @@ def hash_features_affecting_rate

    def tag_names
      @tag_names ||= @rollups.inject([]) do |memo, rollup|
-        memo |= rollup.tag_names.split('|') if rollup.tag_names.present?
+        resource = rollup.resource


Is it possible to have a nil resource? Is that why were were using the tags on the rollups originally?

yes we had a BZ before for this case, if they destroy VM.

probably yes, (it is from origin code) I think that reasons could be more, also performance for example.

jrafanie · 2018-05-24T16:02:30Z

app/models/chargeback/consumption_with_rollups.rb

-        memo |= rollup.tag_names.split('|') if rollup.tag_names.present?
+        resource = rollup.resource
+        tag_names = []
+        tag_names |= resource.tags.collect(&:name).map { |x| x.gsub(TAG_PREFIX, "") } if resource


ah, cool, you handle the nil resource here

jrafanie · 2018-05-24T16:11:19Z

app/models/chargeback_vm.rb

@@ -83,7 +83,8 @@ def self.build_results_for_report_ChargebackVm(options)
  def self.where_clause(records, options, region)
    scope = records.where(:resource_type => "VmOrTemplate")
    if options[:tag] && (@report_user.nil? || !@report_user.self_service?)
-      scope.for_tag_names(options[:tag].split("/")[2..-1])
+      scope_with_current_tags = scope.where(:resource => Vm.find_tagged_with(:any => @options[:tag], :ns => '*'))


It seems odd that we hardcode VmOrTemplate on line 84 and Vm on line 86. This class should have knowledge what classes it's handling at a high level, such as a class method. I'm not sure why it's VmOrTemplate above and Vm below. Should it be VmOrTemplate.find_tagged_with ?

yes it is -- but we are charging only VMs and not VmOrTemplate. So to have VmOrTemplate here is basically useless.

But reason VmOrTemplate on line 84 is that we are not doing metric rollups for only type VM but for VmOrTemplate.

but if you think that we have to keep it more consistent I can change it to VmOrTemplate.find_tagged_with but it is not needed.

@lpichler we can address these hardcoded class names in a followup. It feels like we need a method call to replace these class names:

class ChargebackVm < Chargeback ... def self.chargeable_resource Vm end end

Then, all of these hardcoded places can call chargeable_resource or chargeable_resource.to_s or chargeable_resource.tableize depending if they need Vm, "Vm", or "vms"

Again, we don't have to do this now, it's just something I'm noticing while looking at these change...

jrafanie · 2018-05-24T16:12:59Z

spec/models/chargeback_vm_spec.rb

          [rate_assignment_options_1, rate_assignment_options_2].each do |rate_assignment|
            metric_rollup.tag_names = rate_assignment[:tag].first.tag.send(:name_path)
-            uniq_rates = chargeback_vm.get(consumption)
+            @vm.tag_with(["/managed/#{metric_rollup.tag_names}"], :ns => '*')


Do we have a test where a rollup is tagged with one set of tags and the resource/vm is tagged with an additional tag and we confirm that the additional tag is used for running the chargeback report?

Very good point 👍 because we don't and I found bug during writing this test.

…st_with_prefix

lpichler · 2018-05-24T19:34:49Z

@jrafanie I added test which is covering both issues (one about tag-seleting rates and second about filtering resources described by you #17470 (comment) ) and I did some refactoring in other commits

miq-bot · 2018-05-24T19:42:33Z

Checked commits lpichler/manageiq@ec5989e~...9130b57 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

jrafanie · 2018-05-24T19:58:32Z

@lpichler LGTM. Are both BZs the same issue? Can we close one as a duplicate of the other? I'd rather not reference duplicated BZs in the PR when it gets merged.

jrafanie · 2018-05-24T19:59:24Z

@lpichler Please add your branch labels? Is this needed on gaprindashvili/fine/euwe?

lpichler · 2018-05-25T09:17:37Z

@miq-bot add_label gaprindashvili/yes

lpichler · 2018-05-25T09:20:56Z

Are both BZs the same issue?
yes,
and I closed one of the BZ and I marked it as a duplicate.

JPrause · 2018-05-29T15:09:38Z

@miq-bot add_label blocker

…o_current_tags Use current tags for filtering resources in chargeback for VMs (cherry picked from commit 1a2a40f) https://bugzilla.redhat.com/show_bug.cgi?id=1583786

simaishi · 2018-05-29T17:06:32Z

Gaprindashvili backport details:

$ git log -1
commit ebd3779c686660a4aa15c991fe5ba228d7a5c65e
Author: Joe Rafaniello <[email protected]>
Date:   Fri May 25 11:22:58 2018 -0400

    Merge pull request #17470 from lpichler/filter_chargeback_according_to_current_tags
    
    Use current tags for filtering resources in chargeback for VMs
    (cherry picked from commit 1a2a40f1d6b111f9f1e85e7e568263c32cd624b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1583786

miq-bot added the wip label May 23, 2018

jrafanie reviewed May 23, 2018

View reviewed changes

Use current tags for filtering resources in chargeback

ec5989e

lpichler force-pushed the filter_chargeback_according_to_current_tags branch from 2c58b28 to ec5989e Compare May 24, 2018 15:33

lpichler changed the title ~~[WIP] Use current tags for filtering resources in chargeback for VMs~~ Use current tags for filtering resources in chargeback for VMs May 24, 2018

miq-bot removed the wip label May 24, 2018

jrafanie reviewed May 24, 2018

View reviewed changes

lpichler changed the title ~~Use current tags for filtering resources in chargeback for VMs~~ [WIP] Use current tags for filtering resources in chargeback for VMs May 24, 2018

miq-bot added the wip label May 24, 2018

Consider current tags of resource in Metric::ChargebackHelper::tag_li…

5d2e130

…st_with_prefix

lpichler changed the title ~~[WIP] Use current tags for filtering resources in chargeback for VMs~~ Use current tags for filtering resources in chargeback for VMs May 24, 2018

miq-bot removed the wip label May 24, 2018

lpichler added 3 commits May 24, 2018 21:37

DRY: add method resource_current_tag_names in Metric::ChargebackHelper

bc49fe1

DRY: add method resource_tag_names in Metric::ChargebackHelper

9271514

DRY: add method all_tag_names in Metric::ChargebackHelper

9130b57

lpichler force-pushed the filter_chargeback_according_to_current_tags branch from 2445da6 to 9130b57 Compare May 24, 2018 19:37

jrafanie added core/chargeback bug labels May 24, 2018

miq-bot added the gaprindashvili/yes label May 25, 2018

jrafanie merged commit 1a2a40f into ManageIQ:master May 25, 2018

jrafanie added this to the Sprint 87 Ending Jun 4, 2018 milestone May 25, 2018

jrafanie self-assigned this May 25, 2018

lpichler deleted the filter_chargeback_according_to_current_tags branch May 28, 2018 13:07

miq-bot added the blocker label May 29, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use current tags for filtering resources in chargeback for VMs #17470

Use current tags for filtering resources in chargeback for VMs #17470

lpichler commented May 23, 2018 •

edited

Loading

jrafanie May 23, 2018

lpichler May 24, 2018

lpichler May 24, 2018

jrafanie May 23, 2018

lpichler May 24, 2018

jrafanie May 24, 2018

lpichler May 24, 2018 •

edited

Loading

jrafanie May 24, 2018

jrafanie May 24, 2018

lpichler May 24, 2018

jrafanie May 24, 2018 •

edited

Loading

jrafanie May 24, 2018

jrafanie May 24, 2018

lpichler May 24, 2018

lpichler commented May 24, 2018 •

edited

Loading

miq-bot commented May 24, 2018

jrafanie commented May 24, 2018

jrafanie commented May 24, 2018

lpichler commented May 25, 2018

lpichler commented May 25, 2018

JPrause commented May 29, 2018

simaishi commented May 29, 2018

Use current tags for filtering resources in chargeback for VMs #17470

Use current tags for filtering resources in chargeback for VMs #17470

Conversation

lpichler commented May 23, 2018 • edited Loading

Links

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

lpichler May 24, 2018 • edited Loading

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 May 24, 2018 • edited Loading

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

lpichler commented May 24, 2018 • edited Loading

miq-bot commented May 24, 2018

jrafanie commented May 24, 2018

jrafanie commented May 24, 2018

lpichler commented May 25, 2018

lpichler commented May 25, 2018

JPrause commented May 29, 2018

simaishi commented May 29, 2018

lpichler commented May 23, 2018 •

edited

Loading

lpichler May 24, 2018 •

edited

Loading

jrafanie May 24, 2018 •

edited

Loading

lpichler commented May 24, 2018 •

edited

Loading