From e651717bd207f5faab8a6cf456c5ec83a69cd72a Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 11:54:40 +0100 Subject: [PATCH 1/7] Do not store ActiveRecord object anywhere in saving code. Storing AR object around is causing memory bloat. While doing .reload on AR object is decreasing the memory bloat, it is adding a processing time, cause .reload does a SQL query per each object then. Therefore removing the AR object is decreasing memory required as well as the processing time. --- app/models/ems_refresh/save_inventory.rb | 2 -- app/models/ems_refresh/save_inventory_infra.rb | 1 - 2 files changed, 3 deletions(-) diff --git a/app/models/ems_refresh/save_inventory.rb b/app/models/ems_refresh/save_inventory.rb index 6243a05c40b..571e138851c 100644 --- a/app/models/ems_refresh/save_inventory.rb +++ b/app/models/ems_refresh/save_inventory.rb @@ -115,8 +115,6 @@ def save_vms_inventory(ems, hashes, target = nil) found.save! h[:id] = found.id - found.reload - h[:_object] = found rescue => err # If a vm failed to process, mark it as invalid and log an error h[:invalid] = invalids_found = true diff --git a/app/models/ems_refresh/save_inventory_infra.rb b/app/models/ems_refresh/save_inventory_infra.rb index e7401231bb2..06d442b0049 100644 --- a/app/models/ems_refresh/save_inventory_infra.rb +++ b/app/models/ems_refresh/save_inventory_infra.rb @@ -193,7 +193,6 @@ def save_hosts_inventory(ems, hashes, target = nil) found.save! h[:id] = found.id - h[:_object] = found rescue => err # If a host failed to process, mark it as invalid and log an error h[:invalid] = invalids_found = true From 119953387a17c3a559ca2ed337441a94abc29b0c Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 12:09:50 +0100 Subject: [PATCH 2/7] Use :id instead of :_object for vms saving Use :id instead of :_object for vms saving --- app/models/ems_refresh/save_inventory.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ems_refresh/save_inventory.rb b/app/models/ems_refresh/save_inventory.rb index 571e138851c..a70196a4e17 100644 --- a/app/models/ems_refresh/save_inventory.rb +++ b/app/models/ems_refresh/save_inventory.rb @@ -69,7 +69,7 @@ def save_vms_inventory(ems, hashes, target = nil) h[:cloud_network_id] = key_backup.fetch_path(:cloud_network, :id) h[:cloud_subnet_id] = key_backup.fetch_path(:cloud_subnet, :id) h[:cloud_tenant_id] = key_backup.fetch_path(:cloud_tenant, :id) - h[:cloud_tenants] = key_backup.fetch_path(:cloud_tenants).compact.map { |x| x[:_object] } if key_backup.fetch_path(:cloud_tenants, 0, :_object) + h[:cloud_tenant_ids] = key_backup.fetch_path(:cloud_tenants).compact.map { |x| x[:id] } if key_backup.fetch_path(:cloud_tenants, 0, :id) h[:orchestration_stack_id] = key_backup.fetch_path(:orchestration_stack, :id) begin raise MiqException::MiqIncompleteData if h[:invalid] From df7efdb447d2f8649085c1b4e50d67c99cb84b7e Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 12:14:38 +0100 Subject: [PATCH 3/7] Use :id instead of :_object for saving of cloud tenants Use :id instead of :_object for saving of cloud_tenants --- app/models/ems_refresh/save_inventory_cloud.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ems_refresh/save_inventory_cloud.rb b/app/models/ems_refresh/save_inventory_cloud.rb index 93b23fb95d8..9512f18ba50 100644 --- a/app/models/ems_refresh/save_inventory_cloud.rb +++ b/app/models/ems_refresh/save_inventory_cloud.rb @@ -90,7 +90,7 @@ def save_flavors_inventory(ems, hashes, target = nil) end hashes.each do |h| - h[:cloud_tenants] = (h.fetch_path(:cloud_tenants) || []).compact.map { |x| x[:_object] }.uniq + h[:cloud_tenant_ids] = (h.delete(:cloud_tenants) || []).compact.map { |x| x[:id] }.uniq end save_inventory_multi(ems.flavors, hashes, deletes, [:ems_ref]) From 776aee876d723db68249e85583eb40aa12d9483e Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 14:52:19 +0100 Subject: [PATCH 4/7] Nullify array of the to be 'deleted' objects Nullify array of the to be 'deleted' objects, this was causing a reference and the AR objects were not GCed then. --- app/models/ems_refresh/save_inventory.rb | 2 ++ app/models/ems_refresh/save_inventory_helper.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/models/ems_refresh/save_inventory.rb b/app/models/ems_refresh/save_inventory.rb index a70196a4e17..68610327daf 100644 --- a/app/models/ems_refresh/save_inventory.rb +++ b/app/models/ems_refresh/save_inventory.rb @@ -53,6 +53,8 @@ def save_vms_inventory(ems, hashes, target = nil) _log.info "#{log_header} Duplicate unique values found: #{dup_vms_uids.inspect}" unless dup_vms_uids.empty? invalids_found = false + # Clear vms, so GC can clean them + vms = nil ActiveRecord::Base.transaction do hashes.each do |h| diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index 618484ade98..ed778b060ed 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -43,6 +43,8 @@ def save_inventory_multi(association, hashes, deletes, find_key, child_keys = [] end deletes = deletes.to_a deletes_index = deletes.index_by { |x| x } + # Alow GC to clean the AR objects as they are removed from deletes_index + deletes = nil child_keys = Array.wrap(child_keys) remove_keys = Array.wrap(extra_keys) + child_keys From df636f21899db9c62f7ed77a97953dbbb20e594c Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 14:53:31 +0100 Subject: [PATCH 5/7] Do not build vm_and_templates through association Do not build vm_and_templates through association, rails are caching all the objects there, making it huge in memory. --- app/models/ems_refresh/save_inventory.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ems_refresh/save_inventory.rb b/app/models/ems_refresh/save_inventory.rb index 68610327daf..5d835ff9964 100644 --- a/app/models/ems_refresh/save_inventory.rb +++ b/app/models/ems_refresh/save_inventory.rb @@ -98,7 +98,7 @@ def save_vms_inventory(ems, hashes, target = nil) h[:location] = "unknown" if h[:location].blank? # build a type-specific vm or template - found = ems.vms_and_templates.build(h) + found = ems.vms_and_templates.klass.new(h) else vms_by_uid_ems[h[:uid_ems]].delete(found) h.delete(:type) From d0dc3e348cab13d7e39eb0251626ca938ebbca5c Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 20 Jan 2017 12:18:17 +0100 Subject: [PATCH 6/7] Use :id instead of :_object for NetworkManager saving Use :id instead of :_object for NetworkManager saving. We can also delete processing of cross-manager links ( orchestration_stacks, vms, availability_zones, cloud_tenants) since those will be always AR objects. Only :device can be both Hash and AR object, so it's handled appropriatelly. The ignored cross-manager links processing was needed only when there were CloudManagers without NetworkManagers, so this is not backportable to Darga. Conflicts: app/models/ems_refresh/save_inventory_network.rb --- .../ems_refresh/save_inventory_network.rb | 132 +++++++++--------- 1 file changed, 68 insertions(+), 64 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_network.rb b/app/models/ems_refresh/save_inventory_network.rb index ca970e6eeea..fa7c7f10472 100644 --- a/app/models/ems_refresh/save_inventory_network.rb +++ b/app/models/ems_refresh/save_inventory_network.rb @@ -72,12 +72,6 @@ def save_cloud_networks_inventory(ems, hashes, target = nil) [] end - hashes.each do |h| - %i(cloud_tenant orchestration_stack).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end - end - save_inventory_multi(ems.cloud_networks, hashes, deletes, @@ -96,10 +90,6 @@ def save_network_groups_inventory(ems, hashes, target = nil) [] end - hashes.each do |h| - h[:orchestration_stack_id] = h.fetch_path(:orchestration_stack, :id) - end - save_inventory_multi(ems.network_groups, hashes, deletes, @@ -111,14 +101,11 @@ def save_network_groups_inventory(ems, hashes, target = nil) def save_cloud_subnets_inventory(network, hashes) hashes.each do |h| - %i(availability_zone parent_cloud_subnet).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end - + h[:cloud_network_id] = h.fetch_path(:cloud_network, :id) if h.key?(:cloud_network) h[:ems_id] = network.ems_id end - save_inventory_multi(network.cloud_subnets, hashes, :use_association, [:ems_ref], nil, [:network_router]) + save_inventory_multi(network.cloud_subnets, hashes, :use_association, [:ems_ref], nil, [:network_router, :cloud_network]) network.save! store_ids_for_new_records(network.cloud_subnets, hashes, :ems_ref) @@ -135,15 +122,16 @@ def save_security_groups_inventory(ems, hashes, target = nil) end hashes.each do |h| - %i(cloud_tenant cloud_network orchestration_stack network_group).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:cloud_network_id] = h.fetch_path(:cloud_network, :id) + h[:network_group_id] = h.fetch_path(:network_group, :id) end - save_inventory_multi(ems.security_groups, hashes, + save_inventory_multi(ems.security_groups, + hashes, deletes, [:ems_ref], - :firewall_rules) + :firewall_rules, + [:cloud_network, :network_group]) store_ids_for_new_records(ems.security_groups, hashes, :ems_ref) # Reset the source_security_group_id for the firewall rules after all @@ -168,12 +156,11 @@ def save_floating_ips_inventory(ems, hashes, target = nil) end hashes.each do |h| - %i(vm cloud_tenant cloud_network network_port).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:cloud_network_id] = h.fetch_path(:cloud_network, :id) + h[:network_port_id] = h.fetch_path(:network_port, :id) end - save_inventory_multi(ems.floating_ips, hashes, deletes, [:ems_ref]) + save_inventory_multi(ems.floating_ips, hashes, deletes, [:ems_ref], nil, [:cloud_network, :network_port]) store_ids_for_new_records(ems.floating_ips, hashes, :ems_ref) end @@ -211,15 +198,16 @@ def save_network_routers_inventory(ems, hashes, target = nil) end hashes.each do |h| - %i(cloud_tenant cloud_network network_group).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:cloud_network_id] = h.fetch_path(:cloud_network, :id) + h[:network_group_id] = h.fetch_path(:network_group, :id) end save_inventory_multi(ems.network_routers, hashes, deletes, - [:ems_ref]) + [:ems_ref], + nil, + [:cloud_network, :network_group]) store_ids_for_new_records(ems.network_routers, hashes, :ems_ref) end @@ -251,15 +239,24 @@ def save_network_ports_inventory(ems, hashes, target = nil, mode = :refresh) hashes.compact! hashes.each do |h| - %i(cloud_tenant device cloud_subnet).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) + device = h.fetch_path(:device) + if device.kind_of?(Hash) + h.delete(:device) + + h[:device_id] = device[:id] + h[:device_type] = device[:type].constantize.base_class.name end - h[:security_groups] = (h.fetch_path(:security_groups) || []).map { |x| x.try(:[], :_object) }.compact.uniq + h[:security_group_ids] = (h.delete(:security_groups) || []).map { |x| x.try(:[], :id) }.compact.uniq h[:source] = mode end - save_inventory_multi(ems.network_ports, hashes, deletes, [:ems_ref], :cloud_subnet_network_ports) + save_inventory_multi(ems.network_ports, + hashes, + deletes, + [:ems_ref], + :cloud_subnet_network_ports, + [:cloud_subnet]) store_ids_for_new_records(ems.network_ports, hashes, :ems_ref) end @@ -268,12 +265,15 @@ def save_cloud_subnet_network_ports_inventory(network_port, hashes) deletes = network_port.cloud_subnet_network_ports.reload.dup hashes.each do |h| - %i(cloud_subnet).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:cloud_subnet_id] = h.fetch_path(:cloud_subnet, :id) end - save_inventory_multi(network_port.cloud_subnet_network_ports, hashes, deletes, [:cloud_subnet, :address]) + save_inventory_multi(network_port.cloud_subnet_network_ports, + hashes, + deletes, + [:cloud_subnet_id, :address], + nil, + [:cloud_subnet]) end def save_load_balancer_pool_members_inventory(ems, hashes, target = nil) @@ -286,12 +286,6 @@ def save_load_balancer_pool_members_inventory(ems, hashes, target = nil) [] end - hashes.each do |h| - %i(vm).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end - end - save_inventory_multi(ems.load_balancer_pool_members, hashes, deletes, @@ -321,12 +315,13 @@ def save_load_balancer_pool_member_pools_inventory(load_balancer_pool, hashes) deletes = load_balancer_pool.load_balancer_pool_member_pools.reload.dup hashes.each do |h| - %i(load_balancer_pool_member).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:load_balancer_pool_member_id] = h.fetch_path(:load_balancer_pool_member, :id) end - save_inventory_multi(load_balancer_pool.load_balancer_pool_member_pools, hashes, deletes, + save_inventory_multi(load_balancer_pool.load_balancer_pool_member_pools, + hashes, deletes, + [:load_balancer_pool_member_id], + nil, [:load_balancer_pool_member]) end @@ -341,16 +336,15 @@ def save_load_balancer_listeners_inventory(ems, hashes, target = nil) end hashes.each do |h| - %i(load_balancer).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:load_balancer_id] = h.fetch_path(:load_balancer, :id) end save_inventory_multi(ems.load_balancer_listeners, hashes, deletes, [:ems_ref], - :load_balancer_listener_pools) + :load_balancer_listener_pools, + [:load_balancer]) store_ids_for_new_records(ems.load_balancer_listeners, hashes, :ems_ref) end @@ -358,12 +352,15 @@ def save_load_balancer_listener_pools_inventory(load_balancer_listener, hashes) deletes = load_balancer_listener.load_balancer_listener_pools.reload.dup hashes.each do |h| - %i(load_balancer_pool).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:load_balancer_pool_id] = h.fetch_path(:load_balancer_pool, :id) end - save_inventory_multi(load_balancer_listener.load_balancer_listener_pools, hashes, deletes, [:load_balancer_pool]) + save_inventory_multi(load_balancer_listener.load_balancer_listener_pools, + hashes, + deletes, + [:load_balancer_pool_id], + nil, + [:load_balancer_pool]) end def save_load_balancer_health_checks_inventory(ems, hashes, target = nil) @@ -377,16 +374,16 @@ def save_load_balancer_health_checks_inventory(ems, hashes, target = nil) end hashes.each do |h| - %i(load_balancer load_balancer_listener).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:load_balancer_id] = h.fetch_path(:load_balancer, :id) + h[:load_balancer_listener_id] = h.fetch_path(:load_balancer_listener, :id) end save_inventory_multi(ems.load_balancer_health_checks, hashes, deletes, [:ems_ref], - :load_balancer_health_check_members) + :load_balancer_health_check_members, + [:load_balancer, :load_balancer_listener]) store_ids_for_new_records(ems.load_balancer_health_checks, hashes, :ems_ref) end @@ -394,19 +391,26 @@ def save_load_balancer_health_check_members_inventory(load_balancer_health_check deletes = load_balancer_health_check.load_balancer_health_check_members.reload.dup hashes.each do |h| - %i(load_balancer_pool_member).each do |relation| - h[relation] = h.fetch_path(relation, :_object) if h.fetch_path(relation, :_object) - end + h[:load_balancer_pool_member_id] = h.fetch_path(:load_balancer_pool_member, :id) end - save_inventory_multi(load_balancer_health_check.load_balancer_health_check_members, hashes, deletes, + save_inventory_multi(load_balancer_health_check.load_balancer_health_check_members, + hashes, + deletes, + [:load_balancer_pool_member_id], + nil, [:load_balancer_pool_member]) end def link_cloud_subnets_to_network_routers(hashes) + return if hashes.blank? + + cloud_subnets = CloudSubnet.where(:id => hashes.map { |x| x[:id] }.compact.uniq).find_each.index_by(&:id) + hashes.each do |hash| - network_router = hash.fetch_path(:network_router, :_object) - hash[:_object].update_attributes(:network_router => network_router) + network_router = hash.fetch_path(:network_router, :id) + cloud_subnet = cloud_subnets[hash[:id]] + cloud_subnet.update_attributes(:network_router_id => network_router) if cloud_subnet end end end From 8307f2af5fdb634532c29f0b7ad17401298ea1b9 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Tue, 24 Jan 2017 09:28:54 +0100 Subject: [PATCH 7/7] Associate subnets with routers with less queries Associate subnets with routers with less queries, 50% of the DB queries exactly. --- app/models/ems_refresh/save_inventory_network.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_network.rb b/app/models/ems_refresh/save_inventory_network.rb index fa7c7f10472..3c476e47f5c 100644 --- a/app/models/ems_refresh/save_inventory_network.rb +++ b/app/models/ems_refresh/save_inventory_network.rb @@ -405,12 +405,8 @@ def save_load_balancer_health_check_members_inventory(load_balancer_health_check def link_cloud_subnets_to_network_routers(hashes) return if hashes.blank? - cloud_subnets = CloudSubnet.where(:id => hashes.map { |x| x[:id] }.compact.uniq).find_each.index_by(&:id) - hashes.each do |hash| - network_router = hash.fetch_path(:network_router, :id) - cloud_subnet = cloud_subnets[hash[:id]] - cloud_subnet.update_attributes(:network_router_id => network_router) if cloud_subnet + CloudSubnet.where(:id => hash[:id]).update_all(:network_router_id => hash.fetch_path(:network_router, :id)) end end end