Enhanced dependency and references scanning #13995

Ladas · 2017-02-21T10:28:52Z

Scanning as a next step allows us to make sure all dependencies are there also we can add scanning of all references, to optimize strategies for finding in DB.

Implements:
https://www.pivotaltracker.com/story/show/139357527

Ladas · 2017-02-21T10:32:34Z

@miq-bot assign @agrare

Add scanner for scannign dependencies and references of the inventory collections.

Before the saving, we need to scan the inventory collections

Scanning of IventoryCollections that scans for dependendies and refrerences. Based on those, we can change the strategies for finding in the DB to find only refrerenced by doing 1 DB query.

load_from_db method name changed

We don't need to actualize dependendencies per assignement, it's being solved in the separate scanning step.

Custom db finder using a selection

Test that custom finder with scanned references is working

Add an InventoryCollection for VMs crosslinks

Use an InventoryCollection for VMs crosslinks, this reduces number of DB queries from N to 1.

Ancestry associations are not defined in models, we will hardcode them, until there is a better way of a dependency scan.

Autofix rubocop issues

Ladas · 2017-02-21T13:54:11Z

@miq-bot remove_label wip

miq-bot · 2017-02-21T13:58:06Z

Checked commits Ladas/manageiq@3c17a0b~...7912ea9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
10 files checked, 6 offenses detected

app/models/manager_refresh/inventory_collection.rb

⚠️ - Line 65, Col 7 - Lint/EmptyWhen - Avoid when branches without a body.
❗ - Line 380, Col 9 - Style/IdenticalConditionalBranches - Move projection = nil out of the conditional.
❗ - Line 383, Col 9 - Style/IdenticalConditionalBranches - Move projection = nil out of the conditional.

spec/models/manager_refresh/save_inventory/graph_of_inventory_collections_targeted_refresh_spec.rb

❗ - Line 321, Col 46 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 710, Col 74 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.
❗ - Line 717, Col 74 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.

kbrock

the find_in_db code has me a little intimidated.

But I don't need to understand every line here and the rest of this looks great.

kbrock · 2017-02-21T17:20:34Z

app/models/manageiq/providers/ansible_tower/inventory/parser/automation_manager.rb

@@ -20,8 +20,7 @@ def configured_systems
      inventory_object.hostname = host.name
      inventory_object.virtual_instance_ref = host.instance_id
      inventory_object.inventory_root_group = persister.inventory_root_groups.lazy_find(host.inventory_id.to_s)
-      # TODO(lsmola) get rid of direct DB access
-      inventory_object.counterpart = Vm.find_by(:uid_ems => host.instance_id)
+      inventory_object.counterpart = persister.vms.lazy_find(host.instance_id)


reads so much better too

kbrock · 2017-02-21T17:22:38Z

app/models/manageiq/providers/ansible_tower/inventory/parser/automation_manager.rb

@@ -34,15 +33,13 @@ def configuration_scripts
      inventory_object.variables = job_template.extra_vars_hash
      inventory_object.inventory_root_group = persister.inventory_root_groups.lazy_find(job_template.inventory_id.to_s)

-      authentications = []
+      inventory_object.authentications = []


did this change actually do anything?

yeah, so now the scanning is as a separate step and you can do inventory_object.authentications = [] and inventory_object.authentications << xy

before, the dependency scan was done on assignment, so inventory_object.authentications = [] would do the scan, but inventory_object.authentications << xy would not, since you access the [] directly

kbrock · 2017-02-21T17:23:14Z

app/models/manageiq/providers/ansible_tower/inventory/persister/automation_manager.rb

+      :model_class => Vm,
+      :arel        => Vm,
+      :strategy    => :local_db_find_references,
+      :manager_ref => [:uid_ems]


:mode => :lazy

😄

aka - this looks great

doing non lazy will degrade to SQL query per find, unless you set the references upfront :-(

kbrock · 2017-02-21T17:25:33Z

app/models/manager_refresh/inventory_collection.rb

-        send("process_strategy_#{strategy_name}")
-      when :find_missing_in_local_db
+      when :local_db_cache_all
+        self.data_collection_finalized = true


I'm assuming this means "I actually hit the db"

this is more a flag for 'no more references are coming' so we can do the query and cache it

kbrock · 2017-02-21T17:26:11Z

app/models/manager_refresh/inventory_collection/scanner.rb

@@ -0,0 +1,19 @@
+module ManagerRefresh
+  class InventoryCollection
+    class Scanner


even more classes?

Yes, a class for every problem? :-D I might move this elsewhere, when scanning will turn to more generic preprocessing.

kbrock · 2017-02-21T17:30:00Z

app/models/manager_refresh/inventory_collection.rb

+      # selected << :type if model_class.new.respond_to? :type
+      # load_from_db.select(selected).find_each do |record|
+
+      # Use the cached db_data_index only data_collection_finalized?, meaning no new reference can occur


thanks for this comment - I was just about to ask this very question

kbrock · 2017-02-21T17:31:08Z

app/models/manager_refresh/inventory_collection.rb

+    #
+    # @param manager_uuid [String] a manager_uuid of the InventoryObject we search in the local DB
+    def find_in_db(manager_uuid)
+      # TODO(lsmola) selected need to contain also :keys used in other InventoryCollections pointing to this one, once


what does "once we get list of all keys" mean

Not sure why this is commented out and what is needed to uncomment it.

right, this commented code is pretty old actually, I just moved it. It's for the next step when I will track what attributes are being referenced and I will select only those

cben · 2018-01-04T18:47:16Z

app/models/manager_refresh/inventory_object_lazy.rb

+    def association?(key)
+      # TODO(lsmola) remove this if there will be better dependency scan, probably with transitive dependencies filled
+      # in a second pass
+      return true if [:parent, :genelogy_parent].include?(key)


typo — s/genelogy/genealogy/
Good luck figuring out the consequences :)

hehe, we solve those dependencies manually so hopefully no consequence. :-)

Ladas changed the title ~~Enhanced dependency and references scanning~~ [WIP](Depends on #13926) Enhanced dependency and references scanning Feb 21, 2017

Ladas force-pushed the enhanced_dependency_and_references_scanning branch from 5a246fe to 35da1fa Compare February 21, 2017 10:29

miq-bot added the wip label Feb 21, 2017

miq-bot assigned agrare Feb 21, 2017

miq-bot added the providers/ansible_tower label Feb 21, 2017

Ladas added 11 commits February 21, 2017 14:53

Add scanner for scannign dependencies and references

3c17a0b

Add scanner for scannign dependencies and references of the inventory collections.

Before the saving, we need to scan the inventory collections

8d970be

Before the saving, we need to scan the inventory collections

Scanning of IventoryCollections including changed strategies

a9055c0

Scanning of IventoryCollections that scans for dependendies and refrerences. Based on those, we can change the strategies for finding in the DB to find only refrerenced by doing 1 DB query.

load_from_db method name changed

3f7aee2

load_from_db method name changed

We don't need to actualize dependendencies per assignement

5be35f7

We don't need to actualize dependendencies per assignement, it's being solved in the separate scanning step.

Custom db finder using a selection

525d611

Custom db finder using a selection

Test that custom finder with scanned references is working

58364fe

Test that custom finder with scanned references is working

Add an InventoryCollection for VMs crosslinks

39c483b

Add an InventoryCollection for VMs crosslinks

Use an InventoryCollection for VMs crosslinks

4b4393b

Use an InventoryCollection for VMs crosslinks, this reduces number of DB queries from N to 1.

Ancestry associations are not defined in models

c079e57

Ancestry associations are not defined in models, we will hardcode them, until there is a better way of a dependency scan.

Autofix rubocop issues

7912ea9

Autofix rubocop issues

Ladas force-pushed the enhanced_dependency_and_references_scanning branch from 4ab6d12 to 7912ea9 Compare February 21, 2017 13:53

Ladas changed the title ~~[WIP](Depends on #13926) Enhanced dependency and references scanning~~ Enhanced dependency and references scanning Feb 21, 2017

miq-bot removed the wip label Feb 21, 2017

kbrock reviewed Feb 21, 2017

View reviewed changes

durandom mentioned this pull request Feb 22, 2017

Renamed refresh strategies ManageIQ/manageiq-providers-amazon#146

Merged

agrare merged commit 601f4ca into ManageIQ:master Feb 22, 2017

agrare added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 22, 2017

agrare added enhancement euwe/no labels Feb 22, 2017

cben reviewed Jan 4, 2018

View reviewed changes

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced dependency and references scanning #13995

Enhanced dependency and references scanning #13995

Ladas commented Feb 21, 2017

Ladas commented Feb 21, 2017

Ladas commented Feb 21, 2017

miq-bot commented Feb 21, 2017

kbrock left a comment

kbrock Feb 21, 2017

kbrock Feb 21, 2017

Ladas Feb 21, 2017

kbrock Feb 21, 2017

Ladas Feb 22, 2017

kbrock Feb 21, 2017

Ladas Feb 21, 2017

kbrock Feb 21, 2017

Ladas Feb 21, 2017

kbrock Feb 21, 2017

kbrock Feb 21, 2017

Ladas Feb 21, 2017

cben Jan 4, 2018

Ladas Jan 4, 2018

Enhanced dependency and references scanning #13995

Enhanced dependency and references scanning #13995

Conversation

Ladas commented Feb 21, 2017

Ladas commented Feb 21, 2017

Ladas commented Feb 21, 2017

miq-bot commented Feb 21, 2017

kbrock left a 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

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