[V2V] Add CPU and network throttling in model #18576

ghost · 2019-03-20T11:30:37Z

Now that we introduced default values for CPU and network limits in the settings and that the throttling mechanism is backported into the model, it is time to add the model code to leverage the CPU and network limits at the conversion host level.

This pull request adds:

New methods in the ConversionHost class to compute the CPU and network limits. It divides the limit per host by the number of active migration tasks.
New method in the ConversionHost class to apply the limits for a specific task. It creates a throttling file on the conversion host. The path of the file is provided by virt-v2v-wrapper and stored in the task.
New method in the ServiceTemplateTransformationPlanTask to apply the limits. It collects all the limits in a hash and asks the conversion host to write them in the throttling file.
Extend the InfraConversionJob class to ask the task to apply the limits. This allows to revisit the limits on every poll.

Currently all limits are at the conversion host level, so it looks like the apply_virtv2v_limits method should belong to the ConversionHost class, but later we may have limits on different scopes. This allows better flexibility to concentrate the hash build in the task.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1690851
Depends On: ManageIQ/manageiq-gems-pending#426

ghost · 2019-03-20T11:32:21Z

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

djberg96 · 2019-03-20T12:24:52Z

app/models/conversion_host.rb

+    value == 'unlimited' ? value : "#{value.to_i / active_tasks.size}"
+  end
+
+  def apply_virtv2v_limits(path, limits)


Are there any reasonable defaults we can set for path and/or limits?

Nope. Path is stored in the task and specific to a single migration, so no default. And limits is built by the task object from different sources, so the default can be {}.

djberg96 · 2019-03-20T12:27:05Z

app/models/conversion_host.rb

@@ -56,6 +56,22 @@ def ipaddress(family = 'ipv4')
    resource.ipaddresses.detect { |ip| IPAddr.new(ip).send("#{family}?") }
  end

+  def get_cpu_limit


Do we need get_, or can we just redefine the existing method?

I'm neither a big fan of get_. It's an attribute of ConversionHost, but should default to Settings.transformation.limits.cpu_limit_per_host which we implemented in a previous PR. Can we override an attribute with a method ?

Yep, I think this is the way: http://joshfrankel.me/blog/overriding-activerecord-getters-with-the-same-attribute-name/

djberg96 · 2019-03-20T12:27:35Z

app/models/infra_conversion_job.rb

@@ -104,6 +104,11 @@ def poll_conversion

    case v2v_status
    when 'active'
+      begin
+        migration_task.apply_virtv2v_limits if migration_task.options.fetch_path(:virtv2v_wrapper, 'throttling_file')


Where is throttling_file coming from? Is that a configurable thing? Or is hard coding it ok?

It comes from the output of virt-v2v-wrapper, like state_file. So, pretty much hard coded.

@fdupont-redhat, 2 question

are we doing this apply_virtv2v_limits in every polling, like every 15s, regardless limits changed or not?

so this control flow is infra_conversion_job -> miq_request_task.apply_virtv2v_limits -> conversion_host.apply_virtv2v_limits? seems convoluted. This is the reason I asked to re-consider where we want to keep the throttling logic.

Agree with @jameswn. Seems like too many hoops.

jameswnl · 2019-03-20T12:54:23Z

app/models/conversion_host.rb

+  end
+
+  def get_network_limit
+    value = network_limit || Setting.transformation.limits.network_limit_per_host


jameswnl · 2019-03-20T13:00:06Z

app/models/service_template_transformation_plan_task.rb

@@ -233,6 +233,16 @@ def get_conversion_state
    update_options(updates)
  end

+  # Applies the limits for the task.
+  # CPU and network limits are set at the host level but other limits may not.
+  def apply_virtv2v_limits


I am thinking where this and forthcoming throttling code should reside.
May be a mixin or a sub-module of infra_conversion_job?

For me, the logic should reside in the task as it has all the knowledge of the execution context. The infra_conversion_job is mainly a state machine, and should remain pretty stupid and delegate any intelligence to the task. As whether it should be in a mixin, I'm wondering how much code can be reused by other classes.

jameswnl · 2019-03-26T16:45:55Z

app/models/infra_conversion_job.rb

@@ -104,6 +104,11 @@ def poll_conversion

    case v2v_status
    when 'active'
+      begin
+        migration_task.apply_virtv2v_limits if migration_task.options.fetch_path(:virtv2v_wrapper, 'throttling_file')


@fdupont-redhat, 2 question

are we doing this apply_virtv2v_limits in every polling, like every 15s, regardless limits changed or not?

so this control flow is infra_conversion_job -> miq_request_task.apply_virtv2v_limits -> conversion_host.apply_virtv2v_limits? seems convoluted. This is the reason I asked to re-consider where we want to keep the throttling logic.

jameswnl · 2019-03-26T16:48:51Z

app/models/service_template_transformation_plan_task.rb

+      :cpu     => conversion_host.get_cpu_limit,
+      :network => conversion_host.get_network_limit
+    }
+    conversion_host.apply_virtv2v_limits(options[:virtv2v_wrapper]['throttling_file'], limits)


This construction of limits seems redundant. If this self object has no opinion on how the limits are constructed, why not let conversion_host do it internally?

I think I kind of agree with @jameswnl. Seems like this could be refactored away or delegated.

agrare · 2019-04-09T00:33:22Z

hey guys what is the status of this? I don't see any updates for a couple of weeks and it is still WIP

mturley · 2019-04-16T13:51:38Z

@fdupont-redhat @djberg96 @jameswnl any updates on this one? We're holding off on merging ManageIQ/manageiq-v2v#915 until it's ready

ghost · 2019-04-17T22:39:16Z

@djberg96 @jameswnl I took time to revisit and rethink this, and I moved the limits calculation into InfraConversionThrottler class, and added a dispatch to update and apply the limits to all jobs. This simplifies the design. I still need to add the specs.

ghost · 2019-04-18T10:27:43Z

@miq-bot remove-label wip
@djberg96 @agrare could you review, please ?

djberg96

Nice!

Some comments would be good if you have some time to add them.

lib/infra_conversion_throttler.rb

ghost · 2019-04-18T15:51:47Z

@miq-bot add-reviewer @agrare

agrare · 2019-04-18T16:39:47Z

lib/infra_conversion_throttler.rb

+  # Applying the limits is done via the conversion_host which handles the writing.
+  def self.apply_limits
+    running_conversion_jobs.each do |ch, jobs|
+      number_of_jobs = ch.active_tasks.size


Hm if ConversionHost#active_tasks got out of sync with the running jobs somehow and was 0 then this division will blow up...maybe set this to 1 if it is 0?

Good catch. jobs.size is safer because jobs can't be empty inside the loop.

agrare · 2019-04-18T16:41:49Z

lib/infra_conversion_throttler.rb

+        throttling_file_path = migration_task.options.fetch_path(:virtv2v_wrapper, 'throttling_file')
+        next unless throttling_file_path
+        limits = {
+          :cpu     => cpu_limit == 'unlimited' ? cpu_limit : (cpu_limit.to_i / number_of_jobs).to_s,


It is minor but this calculation doesn't depend on anything in this loop so you could calculate it up here

Something like

cpu_limit = ch.cpu_limit || Settings.transformation.limits.cpu_limit_per_host cpu_limit = (cpu_limit.to_i / number_of_jobs).to_s unless cpu_limit == "unlimited"

agrare · 2019-04-18T17:47:06Z

lib/infra_conversion_throttler.rb

@@ -40,16 +40,21 @@ def self.running_conversion_jobs
  # Applying the limits is done via the conversion_host which handles the writing.
  def self.apply_limits
    running_conversion_jobs.each do |ch, jobs|
-      number_of_jobs = ch.active_tasks.size
+      number_of_jobs = jobs.size


👍 even better

agrare

LGTM

miq-bot · 2019-04-18T17:56:09Z

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

…ts_to_virtv2v [V2V] Add CPU and network throttling in model (cherry picked from commit ce7c67c) https://bugzilla.redhat.com/show_bug.cgi?id=1702085

simaishi · 2019-04-22T22:27:09Z

Hammer backport details:

$ git log -1
commit b34ee8b44080745a2441cbc3bf3be890258637d3
Author: Adam Grare <[email protected]>
Date:   Thu Apr 18 14:15:24 2019 -0400

    Merge pull request #18576 from fdupont-redhat/v2v_apply_resource_limits_to_virtv2v
    
    [V2V] Add CPU and network throttling in model
    
    (cherry picked from commit ce7c67c404127843801f22471f1d286383a9ef17)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1702085

…rce_limits_to_virtv2v" This reverts commit b34ee8b.

simaishi · 2019-05-07T22:27:45Z

Reverted Hammer backport

commit 9d0322d76c646d4f3a15fc36b10caa6972b013b7
Author: Satoe Imaishi <[email protected]>
Date:   Tue May 7 18:26:33 2019 -0400

    Revert "Merge pull request #18576 from fdupont-redhat/v2v_apply_resource_limits_to_virtv2v"
    
    This reverts commit b34ee8b44080745a2441cbc3bf3be890258637d3.

miq-bot changed the title ~~Add CPU and network throttling in model~~ [WIP] Add CPU and network throttling in model Mar 20, 2019

miq-bot added enhancement hammer/yes v2v wip labels Mar 20, 2019

miq-bot requested review from jameswnl and djberg96 March 20, 2019 11:32

ghost changed the title ~~[WIP] Add CPU and network throttling in model~~ [V2V] Add CPU and network throttling in model Mar 20, 2019

ghost changed the title ~~[V2V] Add CPU and network throttling in model~~ [WIP] [V2V] Add CPU and network throttling in model Mar 20, 2019

djberg96 reviewed Mar 20, 2019

View reviewed changes

jameswnl reviewed Mar 20, 2019

View reviewed changes

agrare self-assigned this Mar 20, 2019

jameswnl reviewed Mar 26, 2019

View reviewed changes

fabiendupont added 5 commits April 17, 2019 17:24

Add CPU and network throttling in model

5531207

Add default value for limits. Fix typo.

c6eb80e

Rename methods prefix with get_

a7f0c15

Fix rubocop complaints

a004803

Move logic to InfraConversionThrottler

a770fe2

ghost force-pushed the v2v_apply_resource_limits_to_virtv2v branch from 3323486 to a770fe2 Compare April 17, 2019 22:05

fabiendupont added 2 commits April 17, 2019 18:12

Fix limits hash

ba1f630

Use next to skip iteration.

2953c6f

fabiendupont added 2 commits April 18, 2019 05:55

Add specs. Better error handling in ConversionHost.

b765b06

Fix rubocop complaints

5c1053e

miq-bot changed the title ~~[WIP] [V2V] Add CPU and network throttling in model~~ [V2V] Add CPU and network throttling in model Apr 18, 2019

miq-bot removed the wip label Apr 18, 2019

djberg96 approved these changes Apr 18, 2019

View reviewed changes

Do not apply limits if they have not changed. Added comments to code

4fd56d2

djberg96 reviewed Apr 18, 2019

View reviewed changes

lib/infra_conversion_throttler.rb Show resolved Hide resolved

miq-bot requested a review from agrare April 18, 2019 15:52

agrare reviewed Apr 18, 2019

View reviewed changes

Moving calculation in conversion_host scope

fec9f5e

agrare reviewed Apr 18, 2019

View reviewed changes

agrare approved these changes Apr 18, 2019

View reviewed changes

Fix rubocop complaints

bb72895

agrare merged commit ce7c67c into ManageIQ:master Apr 18, 2019

agrare added this to the Sprint 110 Ending Apr 29, 2019 milestone Apr 18, 2019

mturley mentioned this pull request Apr 18, 2019

[#911] Add validation to migration throttling inputs and add CPU throttling field ManageIQ/manageiq-v2v#915

Merged

simaishi added hammer/backported and removed hammer/yes labels Apr 22, 2019

simaishi added a commit that referenced this pull request May 7, 2019

Revert "Merge pull request #18576 from fdupont-redhat/v2v_apply_resou…

9d0322d

…rce_limits_to_virtv2v" This reverts commit b34ee8b.

simaishi added hammer/yes and removed hammer/backported labels May 7, 2019

Fryguy added hammer/yes? and removed hammer/yes labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Add CPU and network throttling in model #18576

[V2V] Add CPU and network throttling in model #18576

ghost commented Mar 20, 2019

ghost commented Mar 20, 2019

djberg96 Mar 20, 2019

ghost Mar 20, 2019

djberg96 Mar 20, 2019

ghost Mar 20, 2019

djberg96 Mar 20, 2019

djberg96 Mar 20, 2019

ghost Mar 20, 2019

jameswnl Mar 26, 2019

djberg96 Apr 16, 2019 •

edited

Loading

jameswnl Mar 20, 2019

jameswnl Mar 20, 2019

ghost Mar 20, 2019

jameswnl Mar 26, 2019

jameswnl Mar 26, 2019

djberg96 Apr 16, 2019

agrare commented Apr 9, 2019

mturley commented Apr 16, 2019

ghost commented Apr 17, 2019

ghost commented Apr 18, 2019

djberg96 left a comment

ghost commented Apr 18, 2019

agrare Apr 18, 2019

ghost Apr 18, 2019

agrare Apr 18, 2019 •

edited

Loading

ghost Apr 18, 2019

agrare Apr 18, 2019

agrare left a comment

miq-bot commented Apr 18, 2019

simaishi commented Apr 22, 2019

simaishi commented May 7, 2019

[V2V] Add CPU and network throttling in model #18576

[V2V] Add CPU and network throttling in model #18576

Conversation

ghost commented Mar 20, 2019

ghost commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Apr 9, 2019

mturley commented Apr 16, 2019

ghost commented Apr 17, 2019

ghost commented Apr 18, 2019

djberg96 left a comment

Choose a reason for hiding this comment

ghost commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

miq-bot commented Apr 18, 2019

simaishi commented Apr 22, 2019

simaishi commented May 7, 2019

djberg96 Apr 16, 2019 •

edited

Loading

agrare Apr 18, 2019 •

edited

Loading