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 #61

Merged
merged 1 commit into from
Jul 19, 2017
Merged

Conversation

masayag
Copy link
Contributor

@masayag masayag commented Jul 16, 2017

Memory limit feature was introduced in RHV 4.1.
It is already supported by manageiq as part of the vm provision process.
The patch adds support for total memory for providers of version 4.1 and
above, both for VM provision and for the VM refresh process.

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

@@ -108,4 +108,19 @@ def filter_allowed_hosts(all_hosts)
ovirt_services = ManageIQ::Providers::Redhat::InfraManager::OvirtServices::Builder.new(ems).build(:use_highest_supported_version => true).new(:ems => ems)
ovirt_services.filter_allowed_hosts(self, all_hosts)
end

def validate_memory_limit(_field, values, dlg, fld, _value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

usually, generic validation methods are defined in a higher parent-class, however, since this method contains a specific RHV logic, I defined it in this file.

@masayag
Copy link
Contributor Author

masayag commented Jul 16, 2017

@miq-bot assign @borod108

limited = get_value(values[:memory_limit])
return nil unless limited

ems_version = source_ems.api_version[/\d+\.\d+/x]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this and the next line should be in a method, like "supported_in_version?" or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return if vm_memory.zero? && memory_limit.zero?

limit_message = ", total: <#{memory_limit}>" unless memory_limit.zero?
_log.info "Setting memory to:<#{vm_memory.inspect}>#{limit_message}"
Copy link
Contributor

Choose a reason for hiding this comment

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

rubocop wanted parentheses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@borod108 borod108 left a comment

Choose a reason for hiding this comment

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

Great, just acouple of small things.

@masayag
Copy link
Contributor Author

masayag commented Jul 17, 2017

@masayag masayag force-pushed the set_max_vm_memory branch 3 times, most recently from 0bd0c0b to 70577b9 Compare July 17, 2017 12:41
@masayag
Copy link
Contributor Author

masayag commented Jul 17, 2017

@borod108 while this patch is verified to work with use_ovirt_engine_sdk = true, the total memory will be ignored if using v3 (use_ovirt_engine_sdk = false).

Do you think we should block this action in such case ? Or just ignore the provided parameter ?

@borod108
Copy link
Contributor

@masayag I think it should be either blocked, or at least a warning should be shown or it should be mentioned in the description of this field. But the best option, I think, is to block it...

@@ -146,4 +146,9 @@ def unsupported_migration_options
def supports_migrate_for_all?(vms)
vms.map(&:ems_cluster).uniq.compact.size == 1
end

def version_higher_than?(version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matobet perhaps this can be useful for 'import vm' version checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly! CC @smelamud

@masayag
Copy link
Contributor Author

masayag commented Jul 18, 2017

@borod108 so i added the following to this patch:

  1. Nullify memory limit if not using the SDK - this should maintain the UI behavior for RHV >= 4.1 that doesn't use the sdk
  2. Throw an error message in case of unsupported RHV version tries to pass a parameter for memory limit

Memory limit feature was introduced in RHV 4.1.
It is already supported by manageiq as part of the vm provision process.
The patch adds support for total memory for providers of version 4.1 and
above.

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

miq-bot commented Jul 18, 2017

Checked commit masayag@51d4cd3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 2 offenses detected

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

app/models/manageiq/providers/redhat/infra_manager/provision_workflow.rb

@masayag
Copy link
Contributor Author

masayag commented Jul 19, 2017

@miq-bot assign @oourfali

@masayag masayag deleted the set_max_vm_memory branch June 27, 2018 20:27
@masayag
Copy link
Contributor Author

masayag commented Jun 27, 2018

@miq-bot add_label fine/yes

@miq-bot miq-bot assigned borod108 and oourfali and unassigned oourfali and borod108 Jun 27, 2018
@simaishi
Copy link
Contributor

Backported to Fine via ManageIQ/manageiq#17608

This pull request was closed.
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.

6 participants