Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353

NickLaMuro · 2019-01-11T19:15:39Z

This PR introduces a number of improvements to the MiqProvisionVirtWorkflow#allowed_templates, and specifically is beneficial to environments that have EMS's with lots of templates (Amazon in particular when public images are included).

These changes specifically do as much as possible without trying to change anything in the database, but a follow up PR (will be posted shortly) will take these changes further with a few database modifications.

Notable Changes

Limit preloading and prefer virtual attributes (60% drop in processing, 1Gig drop in MEM usage)
Use hash cache for EMS relations instead of preload (Additional 5% drop in processing time)
Limit VM columns pulled back (Additional 10% drop in processing time, 500MB drop in MEM usage)

More specific numbers in the commit messages.

Links

Steps for Testing/QA

The main pain points with this can be recreated with a single AWS EMS that has public images enabled, and has had it's initial refresh.

I was using a combination of this script for testing the full routes:

$ cat script_1
ActiveRecord::Base.logger = Logger.new(STDOUT).tap {|l| l.level = Logger::INFO }

Rails.application.load_console
Rails.env = ENV["RAILS_ENV"]
include Rails::ConsoleMethods
MiqUiWorker.preload_for_console # required


app.post "/dashboard/authenticate", :params => { :user_name => "admin", :user_password => "smartvm" }
app.get  "/vm_cloud/explorer"
csrf_token = app.response.body.match(/.*csrf-token.*content="(?<CSRF_TOKEN>[^"]*)"/)[:CSRF_TOKEN]

headers = { :"X-CSRF-Token" => csrf_token }
app.post "/vm_cloud/x_button?pressed=instance_miq_request_new", :headers => headers
app.post "/miq_request/pre_prov/?sel_id=3192",                  :headers => headers
app.post "/vm_cloud/pre_prov?button=continue",                  :headers => headers
$ bin/rails r script_1

And this one, which tested MiqProvisionVirtWorkflow#allowed_templates on it's own:

$ cat script_2
user     = User.find(1)
wf_klass = ManageIQ::Providers::Amazon::CloudManager::ProvisionWorkflow
workflow = wf_klass.new({}, user, { :initial_pass => true, :src_vm_id => [1] })

dialog_options =  {
  :initial_pass            => true,
  :src_vm_id               => [ 1, "[SOME AWS PUBLIC IMAGE NAME HERE]" ],
  :miq_request_dialog_name => "miq_provision_amazon_dialogs_template",
  :customize_enabled       => [ "enabled" ],
  :current_tab_key         => :requester
}

profile do # use whatever profiler you want here
  workflow.init_from_dialog dialog_options
end
$ bin/rails r script_2

NickLaMuro · 2019-01-11T19:33:20Z

@miq-bot add_label performance

@kbrock (and maybe @Fryguy): Ready for some 🍅 🍅 🍅 's on this one.

kbrock

This is nice.

Even without the performance improvements

app/models/miq_provision_virt_workflow.rb

lib/rbac/filterer.rb

app/models/miq_provision_virt_workflow.rb

NickLaMuro · 2019-01-16T18:48:58Z

@kbrock okay, fixed the code climate stuff my gaming the system. You can see the changes in the following commits:

7a04c48 - Skip debug loop
94cfac1 - Use EMS cache

NickLaMuro · 2019-01-16T21:33:18Z

@kbrock as mentioned, I think I am good with this, but wouldn't mind a second opinion from the automate team.

@syncrou I know you have looked at some of my code for this in the past, so would you mind taking a look briefly to see if anything jumps out at you and I am possibly breaking some things?

kbrock

This looks much nicer.
I'll give @syncrou a little time to check it out.
I'll merge tomorrow unless I hear otherwise

If we aren't in DEBUG mode for `_log` (evm.log), render the strings and loop through the list to "print debug statements. Also fixed some rubocop complexity issues by moving this logic into it's own method.

lib/rbac/filterer.rb

Use virtual_attributes where possible when fetching from the `vms` table to avoid memory bloat. Makes use of `Rbac`'s `:extra_cols` feature. Benchmark --------- **Before** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 110281 | 36 | 3005.5 | 364586 | | 106174 | 36 | 2786.2 | 364586 | | 106952 | 36 | 2812.0 | 364586 | **After** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 43988 | 33 | 2435.7 | 243133 | | 44452 | 33 | 2438.9 | 243133 | | 44009 | 33 | 2470.0 | 243133 |

In #create_hash_struct_from_vm_or_template, avoid generating the `MiqHashStruct` until the end of the method to prevent expensive method missing calls for every generated record. Also does a few things to improve things: - Use `.ems_id` instead of `.ext_management_system`, since it is a non-sql way of checking if there is a relation (performance) - Fixes some rubocop complaints by changing/inverting the if statements

This creates a @_ems_allowed_templates_cache, which is used to keep track of the IDs of ems records that have been fetched, and prevents and additional loop over the @allowed_templates records to perform a preload of the associated ems records. In the case of a AWS Ems with 100k+ templates (after a refresh including public images), this is a non-trivial amount of work that is worth avoiding. Benchmark --------- **Before** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 44251 | 33 | 2479.0 | 243133 | | 43795 | 33 | 2433.2 | 243133 | | 41723 | 33 | 2468.1 | 243133 | **After** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 39488 | 33 | 2439.4 | 243133 | | 38879 | 33 | 2438.2 | 243133 | | 39256 | 33 | 2444.3 | 243133 |

NickLaMuro · 2019-08-10T21:54:39Z

@tinaafitz Could you or someone from the automate team also give this a look and confirm what is here is okay?

NickLaMuro · 2019-08-10T21:55:36Z

@miq-bot remove_label unmergeable

NickLaMuro · 2019-08-10T21:55:57Z

@miq-bot add_label ivanchuk/yes

tinaafitz · 2019-08-12T12:56:35Z

@mkanoor @gmcculloug @lfu Please review.

lfu

Nice job!

gmcculloug

Overall this looks good to me, but I suggest we hold off merging until @kbrock can review and comment on https://github.com/ManageIQ/manageiq/pull/18353/files#r312716130

NickLaMuro · 2019-08-16T21:52:33Z

@gmcculloug thanks!

but I suggest we hold off merging until @kbrock can review and comment on https://github.com/ManageIQ/manageiq/pull/18353/files#r312716130

Yeah, I agree. At this point, it isn't going to get merged any faster if we don't wait for Keenan to get back Monday, so let's just do that.

kbrock

LGTM. just the one comment/response

lib/rbac/filterer.rb

This reduces the number of columns returned from the VMs table to reduce what is returned and serialized from the DB. Benchmark --------- **Before** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 39488 | 33 | 2439.4 | 243133 | | 38879 | 33 | 2438.2 | 243133 | | 39256 | 33 | 2444.3 | 243133 | **After** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 28440 | 33 | 1929.5 | 243133 | | 28491 | 33 | 1933.6 | 243133 | | 28858 | 33 | 2631.9 | 243133 |

respond_to? calls in mass are slow since they require some object introspection, where doing a simple `nil` check is much quicker. By including just the `cloud_tenant_id` in the SELECT clause, it allows for the swapping of `respond_to?` with an `nil` check, with the functionality that existed preserved. Benchmark --------- **Before** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 28440 | 33 | 1929.5 | 243133 | | 28491 | 33 | 1933.6 | 243133 | | 28858 | 33 | 2631.9 | 243133 | **After** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 27107 | 33 | 1958.5 | 243133 | | 26803 | 33 | 1944.2 | 243133 | | 27642 | 33 | 1965.5 | 243133 |

miq-bot · 2019-08-19T19:52:37Z

Checked commits NickLaMuro/manageiq@32be9ec~...f83c69f with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

kbrock

app/models/miq_provision_virt_workflow.rb

NickLaMuro · 2019-08-19T21:57:33Z

@miq-bot add_label blocker

…_better_allowed_templates Performance improvements to MiqProvisionVirtWorkflow#allowed_templates (cherry picked from commit e1ed394) https://bugzilla.redhat.com/show_bug.cgi?id=1662972

simaishi · 2019-08-23T18:39:10Z

Ivanchuk backport details:

$ git log -1
commit 1a1a2147ce289159621d158e40ce610625cb6603
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Aug 19 17:55:51 2019 -0400

    Merge pull request #18353 from NickLaMuro/miq_provision_virt_workflow_better_allowed_templates
    
    Performance improvements to MiqProvisionVirtWorkflow#allowed_templates
    
    (cherry picked from commit e1ed394bf103e5dd921748b2ee5508e6bd1a454e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1662972

reverts f83c69f from ManageIQ#18353 fixes https://bugzilla.redhat.com/show_bug.cgi?id=1746931

issue from ManageIQ#18353 fixes https://bugzilla.redhat.com/show_bug.cgi?id=1746931

miq-bot added the wip label Jan 11, 2019

NickLaMuro mentioned this pull request Jan 11, 2019

[POC][WIP] Make use of platform column to further increase the speed of #allowed_templates #18354

Closed

1 task

miq-bot added the performance label Jan 11, 2019

NickLaMuro force-pushed the miq_provision_virt_workflow_better_allowed_templates branch 3 times, most recently from 2d3c993 to 84a3280 Compare January 15, 2019 00:47

NickLaMuro mentioned this pull request Jan 16, 2019

Use OpenStruct over MiqHashStruct #17359

Closed

kbrock approved these changes Jan 16, 2019

View reviewed changes

kbrock reviewed Jan 16, 2019

View reviewed changes

app/models/miq_provision_virt_workflow.rb Outdated Show resolved Hide resolved

NickLaMuro force-pushed the miq_provision_virt_workflow_better_allowed_templates branch from 84a3280 to 2ce540b Compare January 16, 2019 18:44

NickLaMuro changed the title ~~[WIP] Performance improvements to MiqProvisionVirtWorkflow#allowed_templates~~ Performance improvements to MiqProvisionVirtWorkflow#allowed_templates Jan 16, 2019

miq-bot removed the wip label Jan 16, 2019

NickLaMuro mentioned this pull request Jan 16, 2019

[WIP] update MiqReport::Search to include all columns #18176

Closed

kbrock approved these changes Jan 16, 2019

View reviewed changes

kbrock added automate hammer/no labels Jan 16, 2019

miq-bot added the unmergeable label Aug 8, 2019

[miq_provision_virt_workflow.rb] Skip debug loop

32be9ec

If we aren't in DEBUG mode for `_log` (evm.log), render the strings and loop through the list to "print debug statements. Also fixed some rubocop complexity issues by moving this logic into it's own method.

NickLaMuro force-pushed the miq_provision_virt_workflow_better_allowed_templates branch from 2ce540b to e00cd2a Compare August 10, 2019 21:46

NickLaMuro commented Aug 10, 2019

View reviewed changes

lib/rbac/filterer.rb Outdated Show resolved Hide resolved

NickLaMuro added 3 commits August 10, 2019 16:52

NickLaMuro force-pushed the miq_provision_virt_workflow_better_allowed_templates branch from e00cd2a to 88c1cb7 Compare August 10, 2019 21:53

NickLaMuro requested a review from kbrock August 10, 2019 21:54

miq-bot added ivanchuk/yes and removed unmergeable labels Aug 10, 2019

lfu approved these changes Aug 16, 2019

View reviewed changes

gmcculloug reviewed Aug 16, 2019

View reviewed changes

kbrock approved these changes Aug 19, 2019

View reviewed changes

lib/rbac/filterer.rb Outdated Show resolved Hide resolved

NickLaMuro added 2 commits August 19, 2019 14:49

NickLaMuro force-pushed the miq_provision_virt_workflow_better_allowed_templates branch from 88c1cb7 to f83c69f Compare August 19, 2019 19:49

gmcculloug self-assigned this Aug 19, 2019

kbrock approved these changes Aug 19, 2019

View reviewed changes

app/models/miq_provision_virt_workflow.rb Show resolved Hide resolved

app/models/miq_provision_virt_workflow.rb Outdated Show resolved Hide resolved

gmcculloug merged commit e1ed394 into ManageIQ:master Aug 19, 2019

gmcculloug added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 19, 2019

miq-bot added the blocker label Aug 19, 2019

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Aug 23, 2019

kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 30, 2019

Reintroduce workflow respond_to? call

b570cc9

reverts f83c69f from ManageIQ#18353 fixes https://bugzilla.redhat.com/show_bug.cgi?id=1746931

kbrock mentioned this pull request Aug 31, 2019

Workflow cloud_tenant fix #19237

Merged

kbrock added a commit to kbrock/manageiq that referenced this pull request Sep 3, 2019

Fix cloud workflow

c988392

issue from ManageIQ#18353 fixes https://bugzilla.redhat.com/show_bug.cgi?id=1746931

NickLaMuro mentioned this pull request Jun 25, 2020

Provision dialog for ec2 with public images fails - high worker memory utilization ManageIQ/manageiq-automation_engine#446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353

Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353

NickLaMuro commented Jan 11, 2019 •

edited

Loading

NickLaMuro commented Jan 11, 2019

kbrock left a comment

NickLaMuro commented Jan 16, 2019

NickLaMuro commented Jan 16, 2019

kbrock left a comment

NickLaMuro commented Aug 10, 2019

NickLaMuro commented Aug 10, 2019

NickLaMuro commented Aug 10, 2019

tinaafitz commented Aug 12, 2019

lfu left a comment

gmcculloug left a comment

NickLaMuro commented Aug 16, 2019 •

edited

Loading

kbrock left a comment

miq-bot commented Aug 19, 2019

kbrock left a comment

NickLaMuro commented Aug 19, 2019

simaishi commented Aug 23, 2019

Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353

Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353

Conversation

NickLaMuro commented Jan 11, 2019 • edited Loading

Notable Changes

Links

Steps for Testing/QA

NickLaMuro commented Jan 11, 2019

kbrock left a comment

Choose a reason for hiding this comment

NickLaMuro commented Jan 16, 2019

NickLaMuro commented Jan 16, 2019

kbrock left a comment

Choose a reason for hiding this comment

NickLaMuro commented Aug 10, 2019

NickLaMuro commented Aug 10, 2019

NickLaMuro commented Aug 10, 2019

tinaafitz commented Aug 12, 2019

lfu left a comment

Choose a reason for hiding this comment

gmcculloug left a comment

Choose a reason for hiding this comment

NickLaMuro commented Aug 16, 2019 • edited Loading

kbrock left a comment

Choose a reason for hiding this comment

miq-bot commented Aug 19, 2019

kbrock left a comment

Choose a reason for hiding this comment

NickLaMuro commented Aug 19, 2019

simaishi commented Aug 23, 2019

NickLaMuro commented Jan 11, 2019 •

edited

Loading

NickLaMuro commented Aug 16, 2019 •

edited

Loading