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

Add Cumulative Chargeback rates #17795

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Aug 2, 2018

Add cumulative calculation to chargeback for tagged resources
It is possible to assign chargeback rates to more tags from
various tag categories (ManageIQ/manageiq-ui-classic#4310).

It will cause costs for each rate will be added up.

Example:

VM has tag department/it and enviroment/test

Rate 1 is assigned to tag department/it
Rate 2 is assigned to tag enviroment/test

Then if checkbox from (ManageIQ/manageiq-ui-classic#4112)
will turn on it will add costs from Rate 1 and Rate 2
together.

cost = Rate 1 + Rate 1

But metric values are not affected.

This basically enabling optionally behaviour before #12534

Pictures

VM has tag
Auto Approve - Max CPU/1
Enviroment/Develompent

  1. Report when rate Default is assigned only to Auto Approve - Max CPU/1

Rate for Auto Approve - Max CPU/1

screen shot 2018-08-03 at 14 49 43

Report Result

screen shot 2018-08-03 at 14 47 36

  1. Report when rate Enviroment Rate is assigned only to Enviroment/Development

Rate for Enviroment/Development

screen shot 2018-08-03 at 14 49 53

Report Result

screen shot 2018-08-03 at 14 47 32

  1. Report when rate Default is assigned to Auto Approve - Max CPU/1 and
    rate Enviroment Rate is assigned only to Enviroment/Development

screen shot 2018-08-03 at 14 47 25

Links

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

@miq-bot miq-bot added the wip label Aug 2, 2018
@lpichler lpichler force-pushed the add_cumulative_chargeback_calculation branch 5 times, most recently from bcc50b9 to b403052 Compare August 3, 2018 11:44
@lpichler
Copy link
Contributor Author

lpichler commented Aug 3, 2018

@miq-bot assign @gtanzillo

@miq-bot add_label gaprindashvili/yes, blocker

@lpichler lpichler changed the title [WIP] Add Cumulative Chargeback rates Add Cumulative Chargeback rates Aug 3, 2018
@miq-bot miq-bot removed the wip label Aug 3, 2018
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just one minor suggestion...

) do
def self.new_from_h(hash)
new(*hash.values_at(*members))
end

def skip_field_accumulation?(field, value)
return false unless cumulative_rate_calculation?
Copy link
Member

Choose a reason for hiding this comment

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

The unless here is a little confusing. Would you mind flipping these to return false if ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I also added comment to bring clarity in this method.
thanks!

@lpichler lpichler force-pushed the add_cumulative_chargeback_calculation branch 2 times, most recently from 016b7d2 to 18e09f2 Compare August 3, 2018 13:41
It is possible to assign chargeback rates to more tags from
various tag categories (ManageIQ/manageiq-ui-classic#4310).

It will cause costs for each rate will be added up.

Example:

VM has tag department/it and enviroment/test

Rate 1 is assigned to tag department/it
Rate 2 is assigned to tag enviroment/test

Then if checkbox from (ManageIQ/manageiq-ui-classic#4310)
will turn on it will add costs from Rate 1 and Rate 2
together.

cost = Rate 1 + Rate 1

But metric value will not be doubled.
@lpichler lpichler force-pushed the add_cumulative_chargeback_calculation branch from 18e09f2 to ccee53b Compare August 3, 2018 14:12
@miq-bot
Copy link
Member

miq-bot commented Aug 3, 2018

Checked commit lpichler@ccee53b 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. 👍

@gtanzillo gtanzillo added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 6, 2018
@gtanzillo gtanzillo merged commit ae59274 into ManageIQ:master Aug 6, 2018
@lpichler lpichler deleted the add_cumulative_chargeback_calculation branch August 6, 2018 13:34
simaishi pushed a commit that referenced this pull request Aug 7, 2018
@simaishi
Copy link
Contributor

simaishi commented Aug 7, 2018

Gaprindashvili backport details:

$ git log -1
commit a63ea70d70c780f9bcecdaf9ad4f6be54d698c76
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Aug 6 09:32:58 2018 -0400

    Merge pull request #17795 from lpichler/add_cumulative_chargeback_calculation
    
    Add Cumulative Chargeback rates
    (cherry picked from commit ae59274ed37a551d71e851809cf78bb3904a38d7)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1613367

@simaishi
Copy link
Contributor

simaishi commented Aug 7, 2018

@lpichler Please take a look at Travis failure in G-branch:
https://travis-ci.org/ManageIQ/manageiq/jobs/413101800

  1) ChargebackVm Old Chargeback with metric rollups #get_rates selecting based on tagged cloud volumes doesn't choose rate thanks to missing tag on cloud_volume
     Failure/Error: @options.cumulative_rate_calculation? ? rates.sort_by(&:description) : unique_rates_by_tagged_resources(rates, metric_rollup_record_tags)
     NoMethodError:
       undefined method `cumulative_rate_calculation?' for nil:NilClass
     Shared Example Group: "ChargebackVm" called from ./spec/models/chargeback_vm_spec.rb:1212
     # ./app/models/chargeback/rates_cache.rb:37:in `rates'
     # ./app/models/chargeback/rates_cache.rb:11:in `get'
     # ./spec/models/chargeback_vm_spec.rb:825:in `block (6 levels) in <top (required)>'

@lpichler
Copy link
Contributor Author

lpichler commented Aug 8, 2018

@simaishi there is fix of CI #17816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants