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

Support memory limit for RHV #15591

Merged
merged 1 commit into from
Jul 25, 2017
Merged

Conversation

masayag
Copy link
Contributor

@masayag masayag commented Jul 18, 2017

RHV 4.1 and above supports memory limit.
This option is being exposed in the UI under the 'Hardware' section.

Depends on ManageIQ/manageiq-providers-ovirt#61

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

Before:
before

After:
after

@masayag
Copy link
Contributor Author

masayag commented Jul 18, 2017

@borod108 pls review

@borod108
Copy link

@masayag looks good, some specs are failing, could it be some how related? (I don't think so)
But I think we should wait for them to be green any way.

@masayag masayag closed this Jul 19, 2017
@masayag masayag reopened this Jul 19, 2017
@masayag
Copy link
Contributor Author

masayag commented Jul 19, 2017

@miq-bot assign @martinpovolny

@miq-bot miq-bot assigned martinpovolny and unassigned borod108 Jul 19, 2017
@masayag
Copy link
Contributor Author

masayag commented Jul 19, 2017

@masayag
Copy link
Contributor Author

masayag commented Jul 24, 2017

@h-kataria could you review (and if ok, to merge ?)

@h-kataria
Copy link
Contributor

@gmcculloug changes look good to me, just wanted to run them by you, let me know if they look good to you.

@masayag is there a before/after screenshot you can upload to this PR.

@masayag
Copy link
Contributor Author

masayag commented Jul 24, 2017

@h-kataria see updated PR message for screenshots.

:memory_limit:
:description: Memory (MB)
:required: false
:notes: (Max limit is determined by Operating System type and Arch)
Copy link
Member

Choose a reason for hiding this comment

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

Overall this looks good, but wondering why abbreviate "Arch" here? If this is space related I would abbreviate "OS" (more commonly used) and spelling out "Architecture". Otherwise don't abbreviate either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmcculloug fixed.

@h-kataria
Copy link
Contributor

Looks good to merge once @gmcculloug comment on the notes text is addressed

RHV 4.1 and above supports memory limit.
This option is being exposed in the UI under the 'Hardware' section.

https://bugzilla.redhat.com/show_bug.cgi?id=1461560
@miq-bot
Copy link
Member

miq-bot commented Jul 25, 2017

Checked commit masayag@35d118b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

@gmcculloug
Copy link
Member

Thanks, I'll merge now but please update the "After" image in the description to reflect the text change.

@gmcculloug gmcculloug added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 25, 2017
@masayag
Copy link
Contributor Author

masayag commented Jul 25, 2017

@gmcculloug Thanks, the image is update with the latest change.

@gmcculloug gmcculloug merged commit a8ef251 into ManageIQ:master Jul 25, 2017
@masayag masayag deleted the set_max_vm_memory branch June 27, 2018 20:30
@masayag
Copy link
Contributor Author

masayag commented Jun 27, 2018

@miq-bot add_label fine/yes

@simaishi
Copy link
Contributor

Backported to Fine via Backported to Fine via #17608

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.

9 participants