ansible refresh parser as graph refresh #13585

durandom · 2017-01-19T16:12:38Z

Converted current ansible refresh to new graph refresh

cc @Ladas @juliancheal @blomquisg
@miq-bot add_label providers/ansible, wip

durandom · 2017-01-19T16:14:42Z

app/models/manageiq/providers/ansible_tower/configuration_manager.rb

@@ -16,6 +16,9 @@ class ManageIQ::Providers::AnsibleTower::ConfigurationManager < ManageIQ::Provid
           :with_provider_connection,
           :to => :provider

+  # FIXME: shouldnt be needed. Its here because otherwise the STI type column is the base class in refresh
+  has_many :configured_systems, :dependent => :destroy, :foreign_key => "manager_id"


@Ladas can I somehow force the class on the collection right now? Or maybe we can tweak get_entity_builder

I've deleted the 'get_entity_builder', does that help? :-)

So should be enough to have the right class in the InventoryCollection constructor, or send a :type, if you store multiple under one IC

And yeah, has_many takes :class_name I think

durandom · 2017-01-19T16:15:33Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresh_worker/collector.rb

+  def initialize(ems, target, _options = nil)
+    @ems        = ems
+    @connection = ems.connect
+    @target     = target


so, yeah, target is not used yet. Not sure how targeted refresh would work. Maybe I drop it here

also, I am getting into a point that targeted refresh goes through managers

so e.g. a proper targeted refresh of a VM needs info from CloudManager, NetworkManager and StorageManager now

durandom · 2017-01-19T16:16:34Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresh_worker/parser.rb

+  end
+
+  def inventory_groups
+    # FIXME: memoization  should not be needed.


@Ladas this could be done, if all those individual InventoryCollections would be linked under one e.g. InventoryObject

well, I've started to store them in Inventory under AWS, but probably that is not ideal

durandom · 2017-01-19T16:17:21Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresh_worker/parser.rb

+    @inventory_groups ||= ManagerRefresh::InventoryCollection.new(
+      ManageIQ::Providers::ConfigurationManager::InventoryRootGroup,
+      :association => :inventory_root_groups,
+      :parent      => ems,


@Ladas see above, if this collection would be s child of an InventoryObject this parent could be infered from that

yeah, I do fill this automatically in my Inventory helpers in AWS now

durandom · 2017-01-19T16:18:40Z

app/models/manageiq/providers/configuration_manager.rb

@@ -6,6 +6,7 @@ class ManageIQ::Providers::ConfigurationManager < ::ExtManagementSystem
  has_many :configuration_profiles,       :dependent => :destroy, :foreign_key => "manager_id"
  has_many :configuration_scripts,        :dependent => :destroy, :foreign_key => "manager_id"
  has_many :inventory_groups,             :dependent => :destroy, :foreign_key => "ems_id", :inverse_of => :manager
+  has_many :inventory_root_groups,        :dependent => :destroy, :foreign_key => "ems_id", :inverse_of => :manager


@Ladas same here, ideally it would work without the need of adding that assoc

well, this is an easy way to define the SQL join, you can do that using :arel in the InventoryCollection :-)

miq-bot · 2017-01-19T16:33:20Z

@durandom Cannot apply the following label because they are not recognized: providers/ansible

durandom · 2017-01-19T16:51:50Z

@miq-bot assign @blomquisg
@miq-bot add_label providers/ansible_tower

durandom · 2017-01-20T13:44:30Z

@miq-bot remove_label wip

durandom · 2017-01-20T13:44:52Z

@blomquisg please review

Ladas · 2017-01-23T08:43:29Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresh_worker/parser.rb

@@ -0,0 +1,80 @@
+class ManageIQ::Providers::AnsibleTower::ConfigurationManager::RefreshWorker::Parser
+  attr_accessor :inventory, :ems


can be just attr_reader?

Ladas · 2017-01-23T08:51:44Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresh_worker/parser.rb

+      :association => :configured_systems,
+      # FIXME: array should not be needed
+      # FIXME: manager_ref should not be needed here
+      :manager_ref => [:manager_ref],


I think you need to define this. In ideal world, there would be a DB unique index here, then we can take manager_ref from that.

blomquisg · 2017-01-25T20:05:07Z

Waiting for #13630 ... then this PR will have to reflect the move to AutomationManager.

blomquisg · 2017-01-25T20:05:46Z

This looks fairly straightforward, btw. As soon as the (re)modeling is done, we can move on this pretty quickly I think.

bdunne · 2017-01-25T20:28:51Z

app/models/manageiq/providers/ansible_tower/configuration_manager/refresher.rb

+          [target, inventory]
+        end
+
+        targets_with_data


This variable isn't necessary

bdunne · 2017-01-25T20:32:35Z

Overall, 👍 LGTM

miq-bot · 2017-01-26T15:11:30Z

This pull request is not mergeable. Please rebase and repush.

durandom · 2017-01-27T21:59:25Z

Had to squash all commits into one for easier rebasing on top of master.
But this is just the last version with @bdunne remarks addressed.
So, should be good to go

miq-bot · 2017-01-27T22:42:48Z

Checked commit durandom@3f21c56 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks good. 🍰

bdunne

👍 LGTM

bdunne · 2017-01-30T16:21:50Z

app/models/manageiq/providers/ansible_tower/automation_manager/refresher.rb

+      def parse_targeted_inventory(ems, target, inventory)
+        log_header = format_ems_for_logging(ems)
+        _log.debug "#{log_header} Parsing inventory..."
+        hashes, = Benchmark.realtime_block(:parse_inventory) do


Usually I would expect to see a placeholder for the unused assignment.

Hrm, we seem to waffle on this a bit on our codebase:

>$ git grep ", _ =" | wc -l 4 >$ git grep ", =" | wc -l 29

durandom commented Jan 19, 2017

View reviewed changes

miq-bot added the wip label Jan 19, 2017

durandom force-pushed the ansible_inventory_refresh branch from c786ee3 to 8352bae Compare January 19, 2017 16:45

miq-bot assigned blomquisg Jan 19, 2017

miq-bot added the providers/ansible_tower label Jan 19, 2017

durandom force-pushed the ansible_inventory_refresh branch from 8352bae to fae7952 Compare January 20, 2017 13:12

durandom changed the title ~~[WIP] ansible refresh parser as graph refresh~~ ansible refresh parser as graph refresh Jan 20, 2017

durandom mentioned this pull request Jan 20, 2017

[WIP] Manager refresh inventory build #13601

Closed

miq-bot removed the wip label Jan 20, 2017

Ladas reviewed Jan 23, 2017

View reviewed changes

bdunne requested changes Jan 25, 2017

View reviewed changes

miq-bot added the unmergeable label Jan 26, 2017

durandom force-pushed the ansible_inventory_refresh branch from e54297b to 81f6327 Compare January 27, 2017 21:56

miq-bot removed the unmergeable label Jan 27, 2017

ansible refresh parser as graph refresh

3f21c56

durandom force-pushed the ansible_inventory_refresh branch from 81f6327 to 3f21c56 Compare January 27, 2017 22:36

bdunne approved these changes Jan 30, 2017

View reviewed changes

bdunne reviewed Jan 30, 2017

View reviewed changes

blomquisg merged commit 8aceeb1 into ManageIQ:master Jan 30, 2017

blomquisg added the enhancement label Jan 30, 2017

blomquisg added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 30, 2017

durandom deleted the ansible_inventory_refresh branch March 17, 2017 18:14

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ansible refresh parser as graph refresh #13585

ansible refresh parser as graph refresh #13585

durandom commented Jan 19, 2017 •

edited by miq-bot

Loading

durandom Jan 19, 2017

Ladas Jan 19, 2017 •

edited

Loading

durandom Jan 19, 2017

Ladas Jan 19, 2017

durandom Jan 19, 2017

Ladas Jan 19, 2017

durandom Jan 19, 2017

Ladas Jan 19, 2017

durandom Jan 19, 2017

Ladas Jan 19, 2017 •

edited

Loading

miq-bot commented Jan 19, 2017

durandom commented Jan 19, 2017

durandom commented Jan 20, 2017

durandom commented Jan 20, 2017

Ladas Jan 23, 2017 •

edited

Loading

Ladas Jan 23, 2017

blomquisg commented Jan 25, 2017

blomquisg commented Jan 25, 2017

bdunne Jan 25, 2017

durandom Jan 26, 2017

bdunne commented Jan 25, 2017

miq-bot commented Jan 26, 2017

durandom commented Jan 27, 2017

miq-bot commented Jan 27, 2017

bdunne left a comment

bdunne Jan 30, 2017

blomquisg Jan 30, 2017

		@@ -0,0 +1,80 @@
		class ManageIQ::Providers::AnsibleTower::ConfigurationManager::RefreshWorker::Parser
		attr_accessor :inventory, :ems

ansible refresh parser as graph refresh #13585

ansible refresh parser as graph refresh #13585

Conversation

durandom commented Jan 19, 2017 • edited by miq-bot Loading

Choose a reason for hiding this comment

Ladas Jan 19, 2017 • edited Loading

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

Ladas Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Jan 19, 2017

durandom commented Jan 19, 2017

durandom commented Jan 20, 2017

durandom commented Jan 20, 2017

Ladas Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg commented Jan 25, 2017

blomquisg commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Jan 25, 2017

miq-bot commented Jan 26, 2017

durandom commented Jan 27, 2017

miq-bot commented Jan 27, 2017

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Jan 19, 2017 •

edited by miq-bot

Loading

Ladas Jan 19, 2017 •

edited

Loading

Ladas Jan 19, 2017 •

edited

Loading

Ladas Jan 23, 2017 •

edited

Loading