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

Fix chargeback report when VM is destroyed #16598

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Dec 5, 2017

Fix some cases when Vm is destroyed => then relation Vm - MetricRollup(thru MetricRollup#resource) is destroyed but chargeback report was counting with some attributes of the VM - which doesn't exist so I am adding here trys.

Error message

➜  manageiq git:(master) ✗ ./tools/generate_report.rb 'CHEM_chargeback'
** override_gem: manageiq-ui-classic, [{:path=>"/Users/liborpichler/manageiq/manageiq-ui-classic"}], caller: /Users/liborpichler/manageiq/manageiq/bundler.d/Gemfile.dev.rb
** Using session_store: ActionDispatch::Session::MemCacheStore
Generating report... CHEM_chargeback
/Users/liborpichler/manageiq/manageiq/app/models/chargeback_vm.rb:175:in `init_extra_fields': undefined method `ems_ref' for nil:NilClass (NoMethodError)
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback.rb:72:in `initialize'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback.rb:24:in `new'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback.rb:24:in `block in build_results_for_report_chargeback'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:27:in `block (2 levels) in for_report'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:25:in `each'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:25:in `block in for_report'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:9:in `each'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:9:in `each_cons'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback/consumption_history.rb:9:in `for_report'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback.rb:20:in `build_results_for_report_chargeback'
	from /Users/liborpichler/manageiq/manageiq/app/models/chargeback_vm.rb:62:in `build_results_for_report_ChargebackVm'
	from /Users/liborpichler/manageiq/manageiq/app/models/miq_report/generator.rb:199:in `_generate_table'
	from /Users/liborpichler/manageiq/manageiq/app/models/miq_report/generator.rb:173:in `block in generate_table'
	from /Users/liborpichler/manageiq/manageiq/app/models/user.rb:246:in `with_user'
	from /Users/liborpichler/manageiq/manageiq/app/models/miq_report/generator.rb:173:in `generate_table'
	from /Users/liborpichler/manageiq/manageiq/app/models/miq_report/generator/async.rb:94:in `_async_generate_table'
	from ./tools/generate_report.rb:29:in `<main>'

cc @gtanzillo
@miq-bot assign @jrafanie
@miq-bot add_label bug, chargeback, gaprindashvili/yes

end

it "calculates allocated cpu cost and metric values" do
skip('this case is needs to be fixed in new chargeback') if Settings.new_chargeback
Copy link
Member

Choose a reason for hiding this comment

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

typo: is needs, remove the is

@@ -190,6 +190,22 @@

subject { ChargebackVm.build_results_for_report_ChargebackVm(options).first.first }

context 'when Vm(resource) is destroyed' do
Copy link
Member

Choose a reason for hiding this comment

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

Vm(resource) is hard to follow if you're not familiar with chargeback. Would when the Vm resource of a consumption is destroyed be better?

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, it sounds better to me 👍

@jrafanie
Copy link
Member

jrafanie commented Dec 5, 2017

Minor nitpicks but LGTM when fixed.

hours_in_day changed because
hours_in_day is calculated as:
hours_in_day = end_of_day - [start_date(first_metric_rollup.timestamp), @vm1.created_on].compact.min

and when @VM1 doesn't exist so we have to take into account
that there is used start_date(first_metric_rollup.timestamp)
which is set about few hours later then @vm1.created_on
as was used before
@lpichler lpichler force-pushed the fix_chargeback_report_when_vm_destroyed branch from 2197e31 to a9415f4 Compare December 5, 2017 17:45
@lpichler
Copy link
Contributor Author

lpichler commented Dec 5, 2017

@jrafanie thanks, updated 👍

@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2017

Checked commits lpichler/manageiq@a0e33b4~...a9415f4 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@jrafanie jrafanie merged commit 01cc0ea into ManageIQ:master Dec 5, 2017
simaishi pushed a commit that referenced this pull request Dec 11, 2017
…_destroyed

Fix chargeback report when VM is destroyed
(cherry picked from commit 01cc0ea)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 30dd58324859d64c2fd5a2afdd56c7dbacccfe2b
Author: Joe Rafaniello <[email protected]>
Date:   Tue Dec 5 14:11:23 2017 -0500

    Merge pull request #16598 from lpichler/fix_chargeback_report_when_vm_destroyed
    
    Fix chargeback report when VM is destroyed
    (cherry picked from commit 01cc0ea4437a0810eaf3a7eb989e93d533cae319)

@agrare agrare added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 12, 2017
@lpichler lpichler deleted the fix_chargeback_report_when_vm_destroyed branch March 28, 2018 16:06
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.

5 participants