From e6965429cafda9f08ba7e147d71906a620804bcc Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 May 2018 16:36:45 -0500 Subject: [PATCH 1/2] [WIP][POC] Add Host.all_unique_downcased_domains This query is meant to be a query does the work of `Host#domain` in the query it self, and return it as a column, along with the `id` column as well. The advantage of this is it is a much faster way of collecting the domains for a given host, and doesn't require nearly as many attributes to be returned from the DB, creating object allocations we just don't use (`hosts` has over 30+ columns on it). It also handles filtering uniq domains in the DB by using SELECT DISTINCT ON (domain) ... In the query, but this has a disadvantage in which it `DISTINCT ON` doesn't play nicely with `ActiveRecord#select` (hence the `gsub`ing). --- app/models/host.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/models/host.rb b/app/models/host.rb index 1e849eebbee..15aa689d525 100644 --- a/app/models/host.rb +++ b/app/models/host.rb @@ -194,6 +194,17 @@ def self.failover where(:failover => true) end + def self.all_unique_downcased_domains(includes = nil) + select_rows = select("lower(regexp_replace(split_part(hostname, ',', 1), '[^\.]*\.', '')) as domain") + .select(:id).to_sql + .sub!("SELECT", "DISTINCT ON (domain)") + .sub!(/ FROM.*$/, '') + + query = select(select_rows).where.not(:hostname => nil) + query = query.includes(includes) if includes + query + end + def authentication_check_role 'smartstate' end From bdc70aca2fe1453bb23d480d2c261ea405f78af9 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Wed, 9 May 2018 16:47:32 -0500 Subject: [PATCH 2/2] Use all_unique_downcased_domains in MiqProvisionVirtWorkflow Makes use of `Host.all_unique_downcased_domains` in the `MiqProvisionVirtWorkflow#allowed_domains` method to return only a list of the domain objects, pre-formatted and reduced to unique entries in SQL, reducing object allocations by reducing what is returned from the DB, the number of unneeded rows that needed to be type casted in ruby, and the temp objects generated to build the `domain` string in ruby. The `includes` option of that method is also taken advantage of if the `option[:platform]` is set, though, to me it seems to not make sense since it seems like you could have multiple hosts on the same "domain", but have different platforms for each, so this might return different results when filtered on depending on the order and records in the DB (seems like a bug). Metrics ------- Benchmarks were taken by monitoring the UI requests using the `manageiq_performance` gem. The route monitored was selecting the `"Lifecycle" -> "Publish this VM to a Template"` menu button from a VM show page. The metrics shown are against database with a fairly large EMS, with about 600 hosts, and are 3 subsequent requests against that route. **Before** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 18613 | 2062 | 1463.7 | 70017 | | 17695 | 2062 | 1475.5 | 70017 | | 17774 | 2062 | 1578.4 | 70017 | **After** | ms | queries | query (ms) | rows | | ---: | ---: | ---: | ---: | | 19377 | 2062 | 1401.6 | 66781 | | 17789 | 2062 | 1450.4 | 66781 | | 17874 | 2062 | 1615.5 | 66781 | --- app/models/miq_provision_virt_workflow.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index ae9643595e1..f8c8c94d4f0 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -542,13 +542,15 @@ def allowed_domains(options = {}) @domains ||= begin domains = {} if @values[:forced_sysprep_domain_name].blank? - Host.all.each do |host| - domain = host.domain.to_s.downcase - next if domain.blank? || domains.key?(domain) - # Filter by host platform or is proxy is active + includes = [:operating_system, :hardware] if options[:platform] + Host.all_unique_downcased_domains(includes).each do |host| + # Filter by host platform + # FIXME: Does this even make sense? The domain could be applied to + # hosts with multiple different opperating systems, right? And only + # the first one pulled in will be what is filtered? next unless options[:platform].nil? || options[:platform].include?(host.platform) - next unless options[:active_proxy].nil? || host.is_proxy_active? == options[:active_proxy] - domains[domain] = domain + + domains[host.attributes["domain"]] = host.attributes["domain"] end else @values[:forced_sysprep_domain_name].to_miq_a.each { |d| domains[d] = d }