Charge for container limits in ChargebackContainerImage #15616

zeari · 2017-07-20T11:44:08Z

BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1452799
Charge for container cpu and memory limits as the allocated cpu and memory in chargeback reports.
This makes the allocated columns available for "Chargeback for container images" reports.

@lpichler Please review
cc @simon3z

@miq-bot add_label wip, compute/containers

miq-bot · 2017-07-20T11:45:10Z

@zeari Cannot apply the following label because they are not recognized: compute/containers

zeari · 2017-07-20T11:45:44Z

@miq-bot add_label providers/containers

simon3z · 2017-07-21T08:49:48Z

@zeari WRT the first screenshot it seems that we want to reuse the already present "Allocated CPU Count" and "Allocated Memory" rate fields (instead of adding the new "Limit" ones).

lpichler · 2017-07-21T08:57:50Z

app/models/chargeback/consumption_with_rollups.rb

@@ -22,6 +22,10 @@ def avg(metric)
      metric_sum / consumed_hours_in_interval
    end

+    def limit_value(metric)
+      resource.try("limit_" + metric)


will the report be failing if there will be nil ?

No, nil would just be counted as zero

zeari · 2017-08-09T12:59:06Z

@lpichler Please re-review. Instead of adding new columns for container limits, I leverage the method_for_allocated_metrics option to re-use the existing allocated cpu\memory columns.
its the most none-hacky way i found to integrate this feature and its similar to current_value in consumption_without_rollups.rb

@simon3z @Loicavenel Notice this charges for allocated resources only when a metric for a time period is available.
this is in contrary to disregarding collected metrics and just charging a constant 24 * container.limit_cpu\memory for each day.

zeari · 2017-08-17T08:32:42Z

Notice this charges for allocated resources only when a metric for a time period is available.
this is in contrary to disregarding collected metrics and just charging a constant 24 * container.limit_cpu\memory for each day.

So fixed compute works this way and having just allocated costs work the other way is a bad idea.
I think this is the way to go.

@miq-bot remove_label wip

lpichler · 2017-08-21T12:56:01Z

@miq-bot assign @lpichler

lpichler

@zeari you are adding allocated metrics(limits) for memory and CPU.
we added method_for_allocated_metrics for changing a way how allocated metrics are calculated.
So I am ok with using this for those reasons.

Am I right that all allocated metrics for container comes from current state?

the only what I am suggesting is to change terminology
some thing like 'current_value',... method for allocated could be called current_value.

Also we have this option in UI in filter tab ManageIQ/manageiq-ui-classic#1738. So I think we need to remove it for the container.

thanks!

zeari · 2017-08-28T13:51:43Z

app/models/chargeable_field.rb

    return consumption.send(options.method_for_allocated_metrics, metric) if allocated?
    return consumption.avg(metric) if used?
+    return 0 if consumption.none?(metric)


@lpichler im not sure if i should remove this line entirely. consumption.none?(metric) could be true for a container for one of the allocated fields while the actual field is calculated differently through current_value

zeari · 2017-08-28T14:02:06Z

app/models/container.rb

@@ -24,6 +24,8 @@ class Container < ApplicationRecord
  include EventMixin
  include Metric::CiMixin

+  delegate :created_on, :to => :container_group
+


This solved a CB bug. I can move this to a different PR.

cc @simon3z

@zeari it would be preferable to have this in another PR, easier to track and merge (hopefully?).

zeari · 2017-10-15T10:01:04Z

do we need migration for existing rates when we are adding new metric allocated cpu cores?

The new 'allocated cores' rate detail should be in the default and default container image rate. When someone upgrades dont they run rake db:seed to get new fixtures?

Loicavenel · 2017-10-15T20:18:55Z

@zeari they run rake db:migrate

zeari · 2017-10-16T09:03:02Z

@zeari they run rake db:migrate

I think I get the point @lpichler. That wouldnt add the new Allocated CPU Cores rate row to existing custom rates

Loicavenel · 2017-10-16T10:25:46Z

@zeari @lpichler it will not even add it with no value? the only option will be to delete and re-create it?

miq-bot · 2017-10-20T21:45:42Z

This pull request is not mergeable. Please rebase and repush.

lpichler

👍 @gtanzillo

miq-bot · 2017-10-27T13:10:51Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-10-29T10:33:07Z

Checked commit zeari@9767bea with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 1 offense detected

spec/factories/chargeback_rate_detail.rb

❗ - Line 64, Col 13 - Style/ExtraSpacing - Unnecessary spacing detected.

zeari · 2017-10-30T12:15:34Z

@gtanzillo @lpichler please merge

lpichler

@blomquisg re-approved

miq-bot changed the title ~~charge for container limits in ChargebackContainerImage~~ [WIP] charge for container limits in ChargebackContainerImage Jul 20, 2017

miq-bot added the wip label Jul 20, 2017

miq-bot added the providers/containers label Jul 20, 2017

chessbyte assigned simon3z Jul 20, 2017

lpichler reviewed Jul 21, 2017

View reviewed changes

zeari force-pushed the chargeback_by_limit branch 2 times, most recently from fd0e8e5 to bd5935f Compare August 9, 2017 12:46

miq-bot added the dependencies label Aug 9, 2017

zeari force-pushed the chargeback_by_limit branch from bd5935f to c4e7a13 Compare August 9, 2017 12:54

miq-bot changed the title ~~[WIP] charge for container limits in ChargebackContainerImage~~ charge for container limits in ChargebackContainerImage Aug 17, 2017

miq-bot removed the wip label Aug 17, 2017

zeari changed the title ~~charge for container limits in ChargebackContainerImage~~ Charge for container limits in ChargebackContainerImage Aug 17, 2017

zeari force-pushed the chargeback_by_limit branch 3 times, most recently from 788ac81 to d1723a3 Compare August 17, 2017 14:53

miq-bot assigned lpichler and unassigned simon3z Aug 21, 2017

lpichler suggested changes Aug 28, 2017

View reviewed changes

zeari force-pushed the chargeback_by_limit branch from d1723a3 to ae58c26 Compare August 28, 2017 13:48

zeari commented Aug 28, 2017

View reviewed changes

zeari force-pushed the chargeback_by_limit branch 3 times, most recently from f560d2e to e55995f Compare August 31, 2017 10:54

zeari force-pushed the chargeback_by_limit branch 2 times, most recently from 429f2eb to f1e0252 Compare October 3, 2017 23:02

zeari force-pushed the chargeback_by_limit branch from f1e0252 to dcb23a5 Compare October 15, 2017 08:01

miq-bot removed the unmergeable label Oct 15, 2017

zeari force-pushed the chargeback_by_limit branch 2 times, most recently from a5ea21c to ebfdb2f Compare October 15, 2017 09:52

miq-bot added the unmergeable label Oct 20, 2017

zeari force-pushed the chargeback_by_limit branch from ebfdb2f to 931f6e7 Compare October 23, 2017 15:17

miq-bot removed the unmergeable label Oct 23, 2017

zeari force-pushed the chargeback_by_limit branch from 931f6e7 to 8efa95e Compare October 26, 2017 08:44

lpichler approved these changes Oct 26, 2017

View reviewed changes

zeari mentioned this pull request Oct 26, 2017

Add an 'Allocated cpu cores' chargeback rate detail to existing chargeback rates ManageIQ/manageiq-schema#108

Merged

miq-bot added the unmergeable label Oct 27, 2017

zeari force-pushed the chargeback_by_limit branch from 8efa95e to ca938a0 Compare October 29, 2017 09:48

miq-bot removed the unmergeable label Oct 29, 2017

zeari force-pushed the chargeback_by_limit branch from ca938a0 to 82305c7 Compare October 29, 2017 10:16

Charge for container limits in ChargebackContainerImage

9767bea

zeari force-pushed the chargeback_by_limit branch from 82305c7 to 9767bea Compare October 29, 2017 10:21

lpichler approved these changes Oct 30, 2017

View reviewed changes

gtanzillo approved these changes Oct 30, 2017

View reviewed changes

gtanzillo added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 30, 2017

gtanzillo merged commit 9252900 into ManageIQ:master Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charge for container limits in ChargebackContainerImage #15616

Charge for container limits in ChargebackContainerImage #15616

zeari commented Jul 20, 2017 •

edited

Loading

miq-bot commented Jul 20, 2017

zeari commented Jul 20, 2017

simon3z commented Jul 21, 2017

lpichler Jul 21, 2017

zeari Jul 21, 2017 •

edited

Loading

zeari commented Aug 9, 2017 •

edited

Loading

zeari commented Aug 17, 2017

lpichler commented Aug 21, 2017

lpichler left a comment

zeari Aug 28, 2017

lpichler Sep 4, 2017

zeari Aug 28, 2017

zeari Aug 28, 2017

simon3z Aug 31, 2017

zeari Aug 31, 2017

zeari commented Oct 15, 2017 •

edited

Loading

Loicavenel commented Oct 15, 2017

zeari commented Oct 16, 2017

Loicavenel commented Oct 16, 2017

miq-bot commented Oct 20, 2017

lpichler left a comment

miq-bot commented Oct 27, 2017

miq-bot commented Oct 29, 2017

zeari commented Oct 30, 2017 •

edited

Loading

lpichler left a comment

Charge for container limits in ChargebackContainerImage #15616

Charge for container limits in ChargebackContainerImage #15616

Conversation

zeari commented Jul 20, 2017 • edited Loading

miq-bot commented Jul 20, 2017

zeari commented Jul 20, 2017

simon3z commented Jul 21, 2017

lpichler Jul 21, 2017

Choose a reason for hiding this comment

zeari Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

zeari commented Aug 9, 2017 • edited Loading

zeari commented Aug 17, 2017

lpichler commented Aug 21, 2017

lpichler left a comment

Choose a reason for hiding this comment

zeari Aug 28, 2017

Choose a reason for hiding this comment

lpichler Sep 4, 2017

Choose a reason for hiding this comment

zeari Aug 28, 2017

Choose a reason for hiding this comment

zeari Aug 28, 2017

Choose a reason for hiding this comment

simon3z Aug 31, 2017

Choose a reason for hiding this comment

zeari Aug 31, 2017

Choose a reason for hiding this comment

zeari commented Oct 15, 2017 • edited Loading

Loicavenel commented Oct 15, 2017

zeari commented Oct 16, 2017

Loicavenel commented Oct 16, 2017

miq-bot commented Oct 20, 2017

lpichler left a comment

Choose a reason for hiding this comment

miq-bot commented Oct 27, 2017

miq-bot commented Oct 29, 2017

zeari commented Oct 30, 2017 • edited Loading

lpichler left a comment

Choose a reason for hiding this comment

zeari commented Jul 20, 2017 •

edited

Loading

zeari Jul 21, 2017 •

edited

Loading

zeari commented Aug 9, 2017 •

edited

Loading

zeari commented Oct 15, 2017 •

edited

Loading

zeari commented Oct 30, 2017 •

edited

Loading