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

Delegate vm ram_size to hardware #12938

Merged
merged 2 commits into from
Dec 9, 2016

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 1, 2016

High level:
All aggressive_vcpus_recommended_change_pct use ram_size or cpu_total_cores (see #12911) for calculations.

This sets us up to have 2 of the 3 parameters sql friendly.

Low level:

ram_size (also called memory_mb and mem_cpu) is delegated from vms to hardware.
Currently this delegate is implemented in ruby, this change exposes it to use a delegator and to implement the arel.

Also, for ram_size there is a secondary parameter to look at vm#state. I could only ever find this method call from within the Vm class itself. Separating these methods apart allows us to simplify the calculation and use pure sql for ram_size.

related to #12733 and #12911

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

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2016

Checked commits kbrock/manageiq@7b089db~...9e1a481 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🍰


def ram_size_in_bytes(check_state = false)
ram_size(check_state).to_i * 1.megabyte
state == 'on' ? ram_size : 0
Copy link
Member Author

@kbrock kbrock Dec 9, 2016

Choose a reason for hiding this comment

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

note: this is defining ram_size_by_state not ram_size_in_bytes

display in github is confusing

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 good 👍

@gtanzillo gtanzillo added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 9, 2016
@gtanzillo gtanzillo merged commit b4c4570 into ManageIQ:master Dec 9, 2016
@kbrock kbrock deleted the vm_hardware_ram_size branch December 10, 2016 19:12
kbrock added a commit to kbrock/manageiq that referenced this pull request Dec 12, 2016
@himdel
Copy link
Contributor

himdel commented Dec 13, 2016

@kbrock I'm seeing a

[----] F, [2016-12-13T13:36:03.477887 #13059:2ac2c1aadf08] FATAL -- : Error caught: [ActionView::Template::Error] undefined method `*' for nil:NilClass
/home/himdel/manageiq/app/models/hardware.rb:44:in `ram_size_in_bytes'
/home/himdel/manageiq/lib/extensions/ar_virtual.rb:93:in `ram_size_in_bytes'
/home/himdel/manageiq/app/models/vm_or_template.rb:1535:in `provisioned_storage'
/home/himdel/manageiq/app/models/vm_or_template.rb:1547:in `uncommitted_storage'
/home/himdel/manageiq/app/helpers/vm_helper/textual_summary.rb:644:in `textual_usage_overcommitted'
/home/himdel/manageiq/app/helpers/textual_summary_helper.rb:16:in `expand_textual_summary'
/home/himdel/manageiq/app/helpers/textual_summary_helper.rb:39:in `block in expand_textual_group'
/home/himdel/manageiq/app/helpers/textual_summary_helper.rb:39:in `map'
/home/himdel/manageiq/app/helpers/textual_summary_helper.rb:39:in `expand_textual_group'
/home/himdel/manageiq/app/views/shared/summary/_textual.html.haml:1:in `_app_views_shared_summary__textual_html_haml__2157153568822230949_70200233559440'

while trying to open a summary of a (openstack) template.

I'm thinking this could be related to 9e1a481 (the bit in Hardware).. Do we need a (memory_mb || 0) * 1.megabyte?

@himdel
Copy link
Contributor

himdel commented Dec 13, 2016

When (if) backporting to euwe, please do together with #13125

@simaishi
Copy link
Contributor

Backported to Euwe via #14167

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