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

[V2V] Add default setting for CPU and network limits per conversion host, and max concurrent tasks per EMS #18528

Conversation

ghost
Copy link

@ghost ghost commented Mar 6, 2019

In the current implementation, we are able to control the max number of concurrent conversions per conversion host via the settings. For advanced throttling, we need to add the same for CPU and network limits. We also need the max number of concurrent conversions per EMS, which is today a default value in the throttling code. This PR adds them to config/settings.yml, under transformation / limits.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1686169

@ghost
Copy link
Author

ghost commented Mar 6, 2019

@miq-bot add_label transformation, enhancement, hammer/yes
@miq-bot add_reviewer @djberg96
@miq-bot add_reviewer @agrare

@djberg96
Copy link
Contributor

djberg96 commented Mar 7, 2019

Hm, do we need the word "unlimited" there? If we have the key with no value, then I think it's implicit that it's not set, therefore unlimited. Or perhaps even just false.

@ghost
Copy link
Author

ghost commented Mar 7, 2019

@djberg96 It's string that we then pass to virt-v2v-wrapper. When discussing with Tomas, he said that he prefers unlimited to clearly state, rather than nil or false. It's more explicit. We could make it a default in the ServiceTemplateTransformationPlanTask, but I thought that keeping it in a single place would be nicer.

@agrare agrare self-assigned this Mar 7, 2019
@agrare
Copy link
Member

agrare commented Mar 7, 2019

@fdupont-redhat I don't see where this is being used, is there another PR that will use this value? I agree with @djberg96 it looks weird to see a string value in what I'd expect to be an integer/float field, and prior to passing to the v2v-wrapper we could do cpu_limit || "unlimited"

@mturley
Copy link
Contributor

mturley commented Mar 7, 2019

@fdupont-redhat I think we need max_concurrent_tasks_per_provider here too (mockups here: https://docs.google.com/presentation/d/1BoMkoClBSxATQRPRYt7zuTgRYjaNCIE0y5xCGv5akL4/edit#slide=id.g3f0eb56eb7_0_1).

@ghost
Copy link
Author

ghost commented Mar 7, 2019

@agrare the expected value is a string to accomodate evolution in the format on virt-v2v-wrapper side. Also, we could have had the same kind of default value for max_concurrent_tasks, but it was finally set in Settings: https://github.com/ManageIQ/manageiq/blob/master/app/models/conversion_host.rb#L36.

@ghost
Copy link
Author

ghost commented Mar 7, 2019

@mturley good point. I'll add it, because today's implementation based on custom attribute is dirty.

@ghost ghost changed the title [V2V] Add default setting for CPU and network limits per conversion host [V2V] Add default setting for CPU and network limits per conversion host, and max concurrent tasks per EMS Mar 7, 2019
@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2019

Checked commits fabiendupont/manageiq@a05f532~...fd4d5e7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@agrare agrare merged commit a921fcf into ManageIQ:master Mar 7, 2019
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 7, 2019
@ghost ghost deleted the v2v_add_default_throttling_setting_cpu_and_network branch March 7, 2019 16:24
simaishi pushed a commit that referenced this pull request Mar 8, 2019
…ing_setting_cpu_and_network

[V2V] Add default setting for CPU and network limits per conversion host, and max concurrent tasks per EMS

(cherry picked from commit a921fcf)

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

simaishi commented Mar 8, 2019

Hammer backport details:

$ git log -1
commit a1b667b45fbc38ef4a169d16f1fa6a513433c1a8
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Mar 7 11:11:44 2019 -0500

    Merge pull request #18528 from fdupont-redhat/v2v_add_default_throttling_setting_cpu_and_network
    
    [V2V] Add default setting for CPU and network limits per conversion host, and max concurrent tasks per EMS
    
    (cherry picked from commit a921fcfd995c1bda55b54b2d7bc43f1a6176530e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686874

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