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

Provide max memory on VM reconfigure #224

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

masayag
Copy link
Contributor

@masayag masayag commented Mar 8, 2018

Since ManageIQ doesn't support max memory, there is a need to provider a
value for it when modifying the vm memory limit.

The max memory will be calculated as follow:

  1. Fetch actual VM from RHV to get current max memory
  2. If new actual memory is higher than existing max memory, the max memory will
    be updated as well:
    a. If actual memory is lesser than 1 TB, then set max memory to 4 * actual memory
    b. If actual memory equals or greater than 1TB, set max memory the same value as
    actual memory

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

Since ManageIQ doesn't support max memory, there is a need to provider a
value for it when modifying the vm memory limit.

The max memory will be calculated as follow:
1. Fetch actual VM from RHV to get current max memory
2. If new actual memory is higher than existing max memory, the max memory will
   be updated as well:
 a. If actual memory is lesser than 1 TB, then set max memory to 4 * actual memory
 b. If actual memory equals or greater than 1TB, set max memory the same value as
    actual memory

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

miq-bot commented Mar 8, 2018

Checked commit masayag@42bfd8a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

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

:guaranteed => guaranteed
}
:guaranteed => guaranteed,
:max => (max if supports_max)
Copy link
Contributor

Choose a reason for hiding this comment

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

With this code if supports_max is false the value here will be nil. Should we not set it at all here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the .compact at the end of the hash will remove it from the map if the value is null.
The alternative is extracting it into a local variable and add 'max' conditionally which will add complexity to this method.

:guaranteed => guaranteed
}
:guaranteed => guaranteed,
:max => (max if supports_max)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@masayag
Copy link
Contributor Author

masayag commented Mar 8, 2018

@miq-bot add_labels bug, gaprindashvili/yes

@pkliczewski pkliczewski merged commit a3c14af into ManageIQ:master Mar 8, 2018
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

@masayag The BZ has 5.8.z flag, can/should this go to Fine branch too?

simaishi pushed a commit that referenced this pull request Mar 8, 2018
Provide max memory on VM reconfigure
(cherry picked from commit a3c14af)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553384
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

Gaprindashvili backport details:

$ git log -1
commit 35714ffa953a8aa586f6292da1cbeee84fe13dd0
Author: Piotr Kliczewski <[email protected]>
Date:   Thu Mar 8 13:31:44 2018 +0100

    Merge pull request #224 from masayag/specify_max_memory
    
    Provide max memory on VM reconfigure
    (cherry picked from commit a3c14af086a7012d4e139e0f25121ae547026883)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553384

@mwperina
Copy link

mwperina commented Mar 8, 2018

The BZ has 5.8.z flag, can/should this go to Fine branch too?

Yes, please backport also to 5.8 Satoe. Thanks
@simaishi

@agrare agrare added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 13, 2018
@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018

@masayag The VM reconfigure spec (#22) isn't in Fine branch, and this is conflicting. Should I backport PR 22, or just add describe "#vm_reconfigure" along with before part?

@masayag
Copy link
Contributor Author

masayag commented Apr 10, 2018

@borod108 Do you see any issue with backporting #22 to FINE branch ? It looks safe to me to backport it, but I'd like to be certain about it.

@borod108
Copy link
Contributor

borod108 commented Apr 10, 2018 via email

@masayag
Copy link
Contributor Author

masayag commented Apr 10, 2018

@masayag The VM reconfigure spec (#22) isn't in Fine branch, and this is conflicting. Should I backport PR 22, or just add describe "#vm_reconfigure" along with before part?

@simaishi according to Boris, it is safe to backport #22 as well. So please backport both to fine.

@simaishi
Copy link
Contributor

Fine backport (to manageiq repo) details:

$ git log -1
commit ef06120834f7ef7c39c7e98c715d953bac1935be
Author: Piotr Kliczewski <[email protected]>
Date:   Thu Mar 8 13:31:44 2018 +0100

    Merge pull request #224 from masayag/specify_max_memory
    
    Provide max memory on VM reconfigure
    (cherry picked from commit a3c14af086a7012d4e139e0f25121ae547026883)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565358

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.

7 participants