From 32be9ece776f7fd20125ed9c09a4ca3706e23f9e Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Fri, 4 Jan 2019 15:01:27 -0600 Subject: [PATCH 1/6] [miq_provision_virt_workflow.rb] Skip debug loop 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. --- app/models/miq_provision_virt_workflow.rb | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index 0d96ce3cbc5..9388fc6238a 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -342,14 +342,7 @@ def allowed_templates(options = {}) @allowed_templates_filter = filter_id @allowed_templates_tag_filters = @values[:vm_tags] rails_logger('allowed_templates', 1) - if allowed_templates_list.blank? - _log.warn("Allowed Templates is returning an empty list") - else - _log.warn("Allowed Templates is returning <#{allowed_templates_list.length}> template(s)") - allowed_templates_list.each do |vm| - _log.debug("Allowed Template <#{vm.id}:#{vm.name}> GUID: <#{vm.guid}> UID_EMS: <#{vm.uid_ems}>") - end - end + log_allowed_template_list(allowed_templates_list) MiqPreloader.preload(allowed_templates_list, [:snapshots, :operating_system, :ext_management_system, {:hardware => :disks}]) @allowed_templates_cache = allowed_templates_list.collect do |template| @@ -1094,4 +1087,17 @@ def selected_host(src) raise _("Unable to find Host with Id: [%{id}]") % {:id => src[:host_id]} if src[:host].nil? [load_ar_obj(src[:host])] end + + def log_allowed_template_list(template_list) + if template_list.blank? + _log.warn("Allowed Templates is returning an empty list") + else + _log.warn("Allowed Templates is returning <#{template_list.length}> template(s)") + if _log.level == Logger::DEBUG # skip .each if not in DEBUG + template_list.each do |vm| + _log.debug("Allowed Template <#{vm.id}:#{vm.name}> GUID: <#{vm.guid}> UID_EMS: <#{vm.uid_ems}>") + end + end + end + end end From 0bf7d87c9941ecadc2177862079ff432c9922393 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Fri, 4 Jan 2019 15:02:21 -0600 Subject: [PATCH 2/6] [miq_provision_virt_workflow.rb] Limit .preload 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 | --- app/models/miq_provision_virt_workflow.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index 9388fc6238a..b438ced7e85 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -300,6 +300,7 @@ def tag_symbol :vm_tags end + VM_OR_TEMPLATE_EXTRA_COLS = %i[mem_cpu cpu_total_cores v_total_snapshots allocated_disk_storage].freeze def allowed_templates(options = {}) # Return pre-selected VM if we are called for cloning if [:clone_to_vm, :clone_to_template].include?(request_type) @@ -338,13 +339,13 @@ def allowed_templates(options = {}) end end - allowed_templates_list = source_vm_rbac_filter(vms, condition).to_a + allowed_templates_list = source_vm_rbac_filter(vms, condition, VM_OR_TEMPLATE_EXTRA_COLS).to_a @allowed_templates_filter = filter_id @allowed_templates_tag_filters = @values[:vm_tags] rails_logger('allowed_templates', 1) log_allowed_template_list(allowed_templates_list) - MiqPreloader.preload(allowed_templates_list, [:snapshots, :operating_system, :ext_management_system, {:hardware => :disks}]) + MiqPreloader.preload(allowed_templates_list, [:operating_system :ext_management_system]) @allowed_templates_cache = allowed_templates_list.collect do |template| create_hash_struct_from_vm_or_template(template, options) end @@ -358,9 +359,10 @@ def allowed_template_condition ["vms.template = ? AND vms.ems_id in (?)", true, self.class.provider_model.pluck(:id)] end - def source_vm_rbac_filter(vms, condition = nil) - MiqSearch.filtered(get_value(@values[:vm_filter]).to_i, VmOrTemplate, vms, - :user => @requester, :conditions => condition) + def source_vm_rbac_filter(vms, condition = nil, *extra_cols) + opts = { :user => @requester, :conditions => condition } + opts[:extra_cols] = extra_cols unless extra_cols.empty? + MiqSearch.filtered(get_value(@values[:vm_filter]).to_i, VmOrTemplate, vms, opts) end def allowed_provision_types(_options = {}) From c75af75c8e02cc89103a3094a338457c3ce961b4 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 Jan 2019 16:51:10 -0600 Subject: [PATCH 3/6] [miq_provision_virt_workflow.rb] Delay HashStruct gen 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 --- app/models/miq_provision_virt_workflow.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index b438ced7e85..6af58b5e030 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -1042,18 +1042,17 @@ def create_hash_struct_from_vm_or_template(vm_or_template, options) :v_total_snapshots => vm_or_template.v_total_snapshots, :evm_object_class => :Vm} data_hash[:cloud_tenant] = vm_or_template.cloud_tenant if vm_or_template.respond_to?(:cloud_tenant) - hash_struct = MiqHashStruct.new(data_hash) - hash_struct.operating_system = MiqHashStruct.new( - :product_name => vm_or_template.operating_system.product_name - ) if vm_or_template.operating_system - hash_struct.ext_management_system = MiqHashStruct.new( - :name => vm_or_template.ext_management_system.name - ) if vm_or_template.ext_management_system + if vm_or_template.operating_system + data_hash[:operating_system] = MiqHashStruct.new(:product_name => vm_or_template.operating_system.product_name) + end + if vm_or_template.ems_id + data_hash[:ext_management_system] = MiqHashStruct.new(:name => vm_or_template.ext_management_system.name) + end if options[:include_datacenter] == true - hash_struct.datacenter_name = vm_or_template.owning_blue_folder.try(:parent_datacenter).try(:name) + data_hash[:datacenter_name] = vm_or_template.owning_blue_folder.try(:parent_datacenter).try(:name) end - hash_struct + MiqHashStruct.new(data_hash) end def exit_pre_dialog From c8cc79972a85a6d81d622cd497b1277144f58734 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 Jan 2019 16:55:19 -0600 Subject: [PATCH 4/6] [miq_provision_virt_workflow.rb] Use EMS cache 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 | --- app/models/miq_provision_virt_workflow.rb | 30 ++++++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index 6af58b5e030..9bf2ab5a3cb 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -345,10 +345,12 @@ def allowed_templates(options = {}) rails_logger('allowed_templates', 1) log_allowed_template_list(allowed_templates_list) - MiqPreloader.preload(allowed_templates_list, [:operating_system :ext_management_system]) + MiqPreloader.preload(allowed_templates_list, [:operating_system]) + @_ems_allowed_templates_cache = {} @allowed_templates_cache = allowed_templates_list.collect do |template| create_hash_struct_from_vm_or_template(template, options) end + @_ems_allowed_templates_cache = nil @allowed_templates_cache end @@ -1045,16 +1047,36 @@ def create_hash_struct_from_vm_or_template(vm_or_template, options) if vm_or_template.operating_system data_hash[:operating_system] = MiqHashStruct.new(:product_name => vm_or_template.operating_system.product_name) end - if vm_or_template.ems_id - data_hash[:ext_management_system] = MiqHashStruct.new(:name => vm_or_template.ext_management_system.name) - end if options[:include_datacenter] == true data_hash[:datacenter_name] = vm_or_template.owning_blue_folder.try(:parent_datacenter).try(:name) end + assign_ems_data_to_data_hash(data_hash, vm_or_template) MiqHashStruct.new(data_hash) end + def assign_ems_data_to_data_hash(data_hash, vm_or_template) + # Handle EMS data, either with a cache, or with the relation (assumes it is + # preloaded usually) + if @_ems_allowed_templates_cache + # don't have a key, so don't attempt to add to the data_hash + if vm_or_template.ems_id + ems_id = vm_or_template.ems_id + + # only fetch the ems if not fetched previously + unless @_ems_allowed_templates_cache.key?(vm_or_template.ems_id) + @_ems_allowed_templates_cache[ems_id] = ExtManagementSystem.find(ems_id).try(:name) + end + + if @_ems_allowed_templates_cache[ems_id] + data_hash[:ext_management_system] = MiqHashStruct.new(:name => @_ems_allowed_templates_cache[ems_id]) + end + end + elsif vm_or_template.ems_id + data_hash[:ext_management_system] = MiqHashStruct.new(:name => vm_or_template.ext_management_system.name) + end + end + def exit_pre_dialog @running_pre_dialog = false @values[:pre_dialog_vm_tags] = @values[:vm_tags].dup From 69c94ad4de9d57d163efc7ffed71ca5a3e9b39f7 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 Jan 2019 17:04:55 -0600 Subject: [PATCH 5/6] [miq_provision_virt_workflow.rb] Specify vm cols 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 | --- app/models/miq_provision_virt_workflow.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index 9bf2ab5a3cb..a567725cf8b 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -339,6 +339,9 @@ def allowed_templates(options = {}) end end + # Only select the colums we need + vms = vms.select(:id, :name, :guid, :uid_ems, :ems_id) + allowed_templates_list = source_vm_rbac_filter(vms, condition, VM_OR_TEMPLATE_EXTRA_COLS).to_a @allowed_templates_filter = filter_id @allowed_templates_tag_filters = @values[:vm_tags] From f83c69f8b0a9a9efbbf99a9d0b2f1403525f16e8 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 Jan 2019 17:07:14 -0600 Subject: [PATCH 6/6] [miq_provision_virt_workflow.rb] Avoid respond_to? 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 | --- app/models/miq_provision_virt_workflow.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index a567725cf8b..35ae8cbc4c2 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -340,7 +340,7 @@ def allowed_templates(options = {}) end # Only select the colums we need - vms = vms.select(:id, :name, :guid, :uid_ems, :ems_id) + vms = vms.select(:id, :name, :guid, :uid_ems, :ems_id, :cloud_tenant_id) allowed_templates_list = source_vm_rbac_filter(vms, condition, VM_OR_TEMPLATE_EXTRA_COLS).to_a @allowed_templates_filter = filter_id @@ -1046,7 +1046,7 @@ def create_hash_struct_from_vm_or_template(vm_or_template, options) :allocated_disk_storage => vm_or_template.allocated_disk_storage, :v_total_snapshots => vm_or_template.v_total_snapshots, :evm_object_class => :Vm} - data_hash[:cloud_tenant] = vm_or_template.cloud_tenant if vm_or_template.respond_to?(:cloud_tenant) + data_hash[:cloud_tenant] = vm_or_template.cloud_tenant if vm_or_template.cloud_tenant_id if vm_or_template.operating_system data_hash[:operating_system] = MiqHashStruct.new(:product_name => vm_or_template.operating_system.product_name) end