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

Prioritize rate with tag of VM when selecting from more rates #12534

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Nov 9, 2016

There is a case that more rates can be selected
(for example when storage have more tags)
and in this case, values are accumulated together in report.

so this first way how to deterministically select just one rate
of storage and compute.

So when more rates are selected then
rate with matching tag of VM is used.

This is applied only when rates are assigned to any tagged resources and more rates was selected otherwise behavior is same as before.

@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch 2 times, most recently from a7675f8 to fa94d5f Compare November 9, 2016 17:44
@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from 3a24bc7 to af2046a Compare November 11, 2016 09:28
@lpichler lpichler changed the title [WIP] Select chargeback rates deterministically Select chargeback rates deterministically Nov 11, 2016
@lpichler
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Nov 11, 2016
@lpichler
Copy link
Contributor Author

cc @sergio-ocon

@chargio
Copy link
Contributor

chargio commented Nov 11, 2016

Do we know which user is generating the report?
It seems that the customer was sharing storage between customers, so we should choose only those tags that are shared between the customer and the storage.
If that is not possible, then we should only choose one if there is more than one tag from the same category... and test whether that would give the proper number for all customers (not only the one with the first tag)

@@ -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 = cb.uniq_rates(metric_rollup_record, options.try(:tag))
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need .try(: here.

This comment however is nitpick and can be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

options = Chargeback::ReportOptions.new_from_h({})

has always key :tag or method tag

thanks

@lpichler
Copy link
Contributor Author

Do we know which user is generating the report?

Yes.

It seems that the customer was sharing storage between customers, so we should choose only those tags that are shared between the customer and the storage.

Which tags do you mean ?
Do you mean user's tags ? Which are also on the storage? (accurately tags which are assigned to user's group)
We can get those tags and it is nice idea but it can still leads to ambiguous selections of rates.
(let's assume that user's group has tag A a B and we have assigned tag A to rate R1 and tag B to rate R2, so which should be selected ? )
This is the reason why I am considering tag from report definition(there is just one) in chargeback filters (and using this only in the case when we selected more rates) but it can also leads to ambiguous selections of rates so after it I am simply selecting first rate after sorting.

If that is not possible, then we should only choose one if there is more than one tag from the same category... and test whether that would give the proper number for all customers (not only the one with the first tag)

I am not sure if I understand this point. How to select just one ? Which category do you mean ?

thanks

@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from af2046a to a3fbc6d Compare November 15, 2016 10:46
@chargio
Copy link
Contributor

chargio commented Nov 15, 2016

So the customer was tagging the all the elements for visibility, so everything for customer A was tagged as customer A....
And then it happen to share some elements between others (i.e. the base image was shared, and and adiditional hard drive was used for data, in a similar way that Open Stack would do). Every customer VM would have 1 shared HD and 1 specific HD.
The problem is that the shared HD was visible by every single customer, so it was tagged several times (once per customer), and the calculation would find the tags and associate rates to it, and add them. The customer would end up with a HD tariff that was the sum of every single cost for all customers.

When using one single tag, the report was correct, but visibility not.

@isimluk
Copy link
Member

isimluk commented Nov 16, 2016

Thanks @sergio-ocon for the detailed description of the problem!

@isimluk
Copy link
Member

isimluk commented Nov 16, 2016

In the long run, we may want to allow users to define rate-selection-mechanism in the reports editor.

Something like:

Select rate by:

  • Select all assigned rates
  • Select assigned rates visible to user running the report
  • Select the lower (less expensive) rate (hard to define, but valid requirement)
  • Select the higher (more expensive) rate
  • Pick any
  • Use priorities

That is a property of report. -- The assigned rates themselves are function of the resource in time. The selection of the rate of all assigned rates, however, is a function of the report.

@lpichler
Copy link
Contributor Author

lpichler commented Nov 16, 2016

and also

  • select rate to prioritize only when multiple selection has happened.

@lpichler
Copy link
Contributor Author

@sergio-ocon thanks

but when we will somehow prioritize rates according to user's tags then we have to solve also case for admin user which has visibility to everything.

@sergio-ocon @gtanzillo

would be one of solutions acceptable for our case ? (selection in report definition)

@chargio
Copy link
Contributor

chargio commented Nov 17, 2016

@isimluk I believe that in the long run we should change the way rates are selected.

Right now they are filtered by tag, but they should be selected based on service (dependent on the user) or resource. Tags could then be used as modifiers (a VM with this tag has a different cost that another with a different tag)

That would also allow costs to be accrued at different levels (service, project, tenant, etc), that better reflect the real costs (i.e in the cloud network costs are associated to projects, not VM)

@isimluk
Copy link
Member

isimluk commented Nov 22, 2016

@sergio-ocon, here is an idea:

@isimluk I believe that in the long run (...)

In short run, would it be possible to tag given DataStore(s) by different tag?

  • One tag to determine ownership.
  • Another tag to determine the rate

This solution should work unless

  • there are hundreds of DataStores (too much work)
  • each tenant is charged differently

@chargio
Copy link
Contributor

chargio commented Nov 22, 2016

I would need to discuss it to understand the implications.

Can we run a bluejeans?

@gtanzillo
Copy link
Member

@lpichler @sergio-ocon @isimluk

I'm thinking that for a short term solution, we can try to select the rate that has at least one tag that the target VM or Storage has. I think this would work in the case that @sergio-ocon illustrated above.

Customer A has everything tagged with "Customer A"
Customer B has everything tagged with "Customer B"
The shared storage is tagged with both "Customer A" and "Customer B"
Compute Rate 1 assigned to VMs tagged with "Customer A"
Compute Rate 2 assigned to VMs tagged with "Customer B"
Storage Rate 1 assigned to datastores tagged with "Customer A"
Storage Rate 2 assigned to datastores tagged with "Customer B"

So currently, getting rates for a VM tagged with "customer A" will return:
Compute Rate 1, Storage Rate 1 and Storage Rate 2

If we filter out the storage rates that don't have an "assigned to tag" matching any of the tags on the VM it would eliminate Storage Rate 2.

I think this could work. What do you think? I can setup a meeting to discuss...

@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@lpichler
Copy link
Contributor Author

Nice description @gtanzillo. I was looking on this suggestion and it can work.
If I understand well we will basically prioritize rate which is assigned to some tag with tag of VM.
This information(VM's tag) will be taken from MetricRollup#tag_names.

Are you OK with it @isimluk @sergio-ocon ? Or do we need meeting for it ?

thanks

@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from a3fbc6d to 4c7d2fc Compare December 1, 2016 13:19
@lpichler lpichler changed the title Select chargeback rates deterministically Prioritize rate with tag of VM when selecting from more rates Dec 1, 2016
@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from 4c7d2fc to cf13601 Compare December 1, 2016 14:11
@lpichler
Copy link
Contributor Author

lpichler commented Dec 1, 2016

@gtanzillo @isimluk I implemented suggestion in comment #12534 (comment) - to prioritize tag of VM for selecting rate with this tag.
Description updated.

@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch 5 times, most recently from 65539a1 to a8e94dd Compare December 6, 2016 14:15
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.

Looks great, thanks! 👍

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from a8e94dd to 3cfc381 Compare December 7, 2016 09:35
There is a case that more rates can be selected
(for example when storage have more tags)
and in this case, values are accumulated together in report.

so this first way how to deterministically select just one rate
of storage and compute.

So when more rates are selected then
rate with matching tag of VM is used.
@lpichler lpichler force-pushed the select_chargeback_rate_deterministically branch from 3cfc381 to 06720cd Compare December 7, 2016 12:35
@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2016

Checked commits lpichler/manageiq@e68eba5~...06720cd with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 0 offenses detected
Everything looks good. 🏆

@lpichler
Copy link
Contributor Author

lpichler commented Dec 7, 2016

@gtanzillo rebased as this was merged and I changed name of methods sort of and I added other wrapper method called rates otherwise it is same as we discussed.

@gtanzillo
Copy link
Member

👍 Looks good!

@gtanzillo gtanzillo added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 7, 2016
@gtanzillo gtanzillo merged commit d88a7da into ManageIQ:master Dec 7, 2016
@lpichler lpichler deleted the select_chargeback_rate_deterministically branch December 12, 2016 09:21
@simaishi
Copy link
Contributor

Backported to Euwe via #13556

@simaishi
Copy link
Contributor

Backported to Darga via #13559

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.

None yet

7 participants