Targeted refresh #153

slemrmartin · 2019-01-11T12:08:52Z

Targeted refresh for refreshing only specified inventory (and its subgraph) - like for amazon, azure etc.

depends on: AnsibleTower, EmbeddedAnsible targeted refresh support manageiq#18367
dependent: Targeted refresh for Embedded ansible provider manageiq#18429
- needs to be merged at the same time, because embedded_ansible depends on ansible tower

agrare · 2019-01-18T13:55:56Z

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

@@ -1,3 +1,3 @@
-class ManageIQ::Providers::AnsibleTower::AutomationManager::Refresher < ManageIQ::Providers::BaseManager::Refresher
+class ManageIQ::Providers::AnsibleTower::AutomationManager::Refresher < ManageIQ::Providers::BaseManager::ManagerRefresher


@slemrmartin is this required? I'm trying to remove ManagerRefresher in another PR

@agrare it raises error when I'm refreshing ConfigurationScriptSource with Refresher.
I think there is error in collector_inventory_for_targets(). In ManagerRefresher, it derives Collector class in Inventory, there it tries to do by itself (https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/base_manager/refresher.rb#L141). This construction doesn't work for TargetCollection.
Then it calls parse_targeted_inventory with collector == nil.

sorry, for InventoryRefresh::Target it doesn't work...it contantizes ManageIQ::Providers::AnsibleTower::Inventory::Collector::Target instead of ManageIQ::Providers::AnsibleTower::Inventory::Collector::TargetCollection

agrare · 2019-01-18T13:57:24Z

app/models/manageiq/providers/ansible_tower/shared/inventory/collector/target_collection.rb

+
+  def initialize(_manager, _target)
+    super
+  end


Why is this needed if all it does is super ?

it's not, I'll remove it

it's needed now :) it adds dependent targets

agrare · 2019-01-18T14:07:42Z

@jameswnl @Ladas can you review?

Ladas · 2019-01-22T08:43:50Z

app/models/manageiq/providers/ansible_tower/shared/inventory/collector/target_collection.rb

+        [nil]
+      end
+    else
+      endpoint.all(:id__in => refs.join(','))


check if there is a limit for this, e.g. for amazon, you can search for max 200 ids. Then you can do something like the multi_query here https://github.com/Ladas/manageiq-providers-amazon/blob/4197be8d1dee08a18c87483cf5ada03bebb6f5c1/app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb#L23 , which is just an iterator

@Ladas hm, I didn't find it in documentation so I need to make additional spec

@Ladas The only limit I found is URI length, then it returns HTTP 414 Request uri too large. It can be 2083 characters and depends on length of tower api's URL.
I thing splitting to 200-length query should be enough (manager-refs are IDS, so we can expect it should match this condition theoretically for 8-digit comma-separated ids).

Ladas · 2019-01-22T08:49:59Z

app/models/manageiq/providers/ansible_tower/shared/inventory/collector/target_collection.rb

+
+  # @param collection [Symbol] inventory collection name (as identified in persister's definitions)
+  def references(collection)
+    manager_ref = manager_ref_by_collection(collection)


hm I would probably do this explicitly, rather than than building tmp persister? Most of the manage_ref are the same, aren't they?

I want to have definition of manager_ref on 1 place rather than repeating the same on two places and ansible tower inventories doesn't have manager_ref column for all inventories

Refactoring to Shared modules

slemrmartin · 2019-02-01T16:16:16Z

@agrare @Ladas review please.
I have to keep [wip] flag because of embedded ansible (I don't test it if it's working now - there is core dependency on this code)

miq-bot · 2019-02-01T16:17:34Z

Some comments on commits slemrmartin/manageiq-providers-ansible_tower@9de8377~...04d61e4

spec/vcr_cassettes/manageiq/providers/ansible_tower/automation_manager/refresher.yml

💣 💥 🔥 🚒 - 1009 - Detected cfme
💣 💥 🔥 🚒 - 1039 - Detected cloudforms
💣 💥 🔥 🚒 - 1044 - Detected cfme
💣 💥 🔥 🚒 - 1048 - Detected cfme
💣 💥 🔥 🚒 - 1074 - Detected cfme
💣 💥 🔥 🚒 - 1080 - Detected cloudforms
💣 💥 🔥 🚒 - 1091 - Detected cfme
💣 💥 🔥 🚒 - 1121 - Detected cloudforms
💣 💥 🔥 🚒 - 1157 - Detected cfme
💣 💥 🔥 🚒 - 1287 - Detected cloudforms
💣 💥 🔥 🚒 - 1370 - Detected cloudforms
💣 💥 🔥 🚒 - 1471 - Detected cloudforms
💣 💥 🔥 🚒 - 1563 - Detected cloudforms
💣 💥 🔥 🚒 - 1603 - Detected cloudforms
💣 💥 🔥 🚒 - 1644 - Detected cloudforms
💣 💥 🔥 🚒 - 1684 - Detected cloudforms
💣 💥 🔥 🚒 - 2260 - Detected cloudforms
💣 💥 🔥 🚒 - 2302 - Detected cloudforms
💣 💥 🔥 🚒 - 2342 - Detected cloudforms
💣 💥 🔥 🚒 - 458 - Detected cloudforms
💣 💥 🔥 🚒 - 493 - Detected cfme
💣 💥 🔥 🚒 - 543 - Detected cloudforms
💣 💥 🔥 🚒 - 580 - Detected cfme
💣 💥 🔥 🚒 - 630 - Detected cloudforms
💣 💥 🔥 🚒 - 635 - Detected cfme
💣 💥 🔥 🚒 - 639 - Detected cfme
💣 💥 🔥 🚒 - 665 - Detected cfme
💣 💥 🔥 🚒 - 671 - Detected cloudforms
💣 💥 🔥 🚒 - 6755 - Detected cfme
💣 💥 🔥 🚒 - 682 - Detected cfme
💣 💥 🔥 🚒 - 712 - Detected cloudforms
💣 💥 🔥 🚒 - 717 - Detected cfme
💣 💥 🔥 🚒 - 721 - Detected cfme
💣 💥 🔥 🚒 - 747 - Detected cfme
💣 💥 🔥 🚒 - 753 - Detected cloudforms
💣 💥 🔥 🚒 - 7639 - Detected cloudforms
💣 💥 🔥 🚒 - 764 - Detected cfme
💣 💥 🔥 🚒 - 7640 - Detected cloudforms
💣 💥 🔥 🚒 - 7649 - Detected cloudforms
💣 💥 🔥 🚒 - 7653 - Detected cloudforms
💣 💥 🔥 🚒 - 7718 - Detected cloudforms
💣 💥 🔥 🚒 - 794 - Detected cloudforms
💣 💥 🔥 🚒 - 799 - Detected cfme
💣 💥 🔥 🚒 - 803 - Detected cfme
💣 💥 🔥 🚒 - 829 - Detected cfme
💣 💥 🔥 🚒 - 835 - Detected cloudforms
💣 💥 🔥 🚒 - 846 - Detected cfme
💣 💥 🔥 🚒 - 876 - Detected cloudforms
💣 💥 🔥 🚒 - 881 - Detected cfme
💣 💥 🔥 🚒 - 885 - Detected cfme
💣 💥 🔥 🚒 - 911 - Detected cfme
💣 💥 🔥 🚒 - 917 - Detected cloudforms
💣 💥 🔥 🚒 - 928 - Detected cfme
💣 💥 🔥 🚒 - 957 - Detected cloudforms
💣 💥 🔥 🚒 - 962 - Detected cfme
💣 💥 🔥 🚒 - 966 - Detected cfme
💣 💥 🔥 🚒 - 992 - Detected cfme
💣 💥 🔥 🚒 - 998 - Detected cloudforms

miq-bot · 2019-02-01T16:17:40Z

Checked commits slemrmartin/manageiq-providers-ansible_tower@9de8377~...04d61e4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
16 files checked, 8 offenses detected

lib/tasks_private/spec_helper.rake

❗ - Line 4, Col 35 - Layout/SpaceAroundOperators - Surrounding space missing for operator ||.

spec/support/ansible_shared/automation_manager/refresh_configuartion_script_source.rb

❗ - Line 34, Col 71 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 35, Col 71 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 55, Col 59 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 56, Col 59 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/support/ansible_shared/automation_manager/refresh_targeted.rb

⚠️ - Line 1, Col 77 - Lint/UnusedBlockArgument - Unused block argument - ems_type. If it's necessary, use _ or _ems_type as an argument name to indicate that it won't be used.
❗ - Line 129, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 211, Col 99 - Style/HashSyntax - Use hash rockets syntax.

slemrmartin · 2019-02-01T16:30:27Z

spec/support/ansible_shared/automation_manager/refresh_configuartion_script_source.rb

@@ -47,7 +49,11 @@
        configuration_script_payloads = configuration_script_source.configuration_script_payloads

        VCR.use_cassette(cassette_path) do
-          EmsRefresh.refresh([[configuration_script_source.class.to_s, configuration_script_source.id]])
+          # EmsRefresh.refresh([[configuration_script_source.class.to_s, configuration_script_source.id]])


@jameswnl @agrare @Ladas previous "like-targeted" refresh of configuration_script_sources have its own collector/parser/persister classes but I don't know if it uses InventoryRefresh::Target in real or not.
I should remove these 3 classes.
But I'm not sure how is it used in production, if it's ok to work only with InventoryRefresh::Target or not (ConfigurationScriptSource is not on allowed list in EmsRefresh.get_target_objects )

So if the class wasn't listed in https://github.com/Ladas/manageiq/blob/3227d2b7f73da39db5f6b52b079977e2e32312e9/app/models/ems_refresh.rb#L136 , I suppose it never worked.

Since the target wouldn't go through EmsRefresh.queue_refresh, which is used in production workflow

anyway, this change is needed there but brokes embedded ansible specs from core (this is shared example) so there is cycle dependency

Also added hello_workflow configuration_workflow to populate_tower rake task

Ladas

👍 looks good

@agrare this will break core specs so we should merge it together with the core PR

slemrmartin · 2019-02-06T10:39:39Z

looks good

@agrare this will break core specs so we should merge it together with the core PR

https://github.com/ManageIQ/manageiq/blob/master/spec/models/manageiq/providers/embedded_ansible/automation_manager/refresher_configuration_script_source_spec.rb

this is one of the evil spec :)

…resh" This reverts commit f9d95fb, reversing changes made to dfcd8e8.

Revert "Merge pull request #153 from slemrmartin/targeted-refresh"

…resh" This reverts commit f9d95fb, reversing changes made to dfcd8e8.

it's revert of revert's PR ManageIQ#153

…eted-refresh"" This reverts commit 7165fec.

Revert "Revert "Merge pull request #153 from slemrmartin/targeted-refresh"

miq-bot added the wip label Jan 11, 2019

slemrmartin mentioned this pull request Jan 16, 2019

AnsibleTower, EmbeddedAnsible targeted refresh support ManageIQ/manageiq#18367

Merged

slemrmartin force-pushed the targeted-refresh branch 3 times, most recently from d5f2302 to d831f1f Compare January 16, 2019 14:20

agrare self-assigned this Jan 17, 2019

agrare reviewed Jan 18, 2019

View reviewed changes

Ladas reviewed Jan 22, 2019

View reviewed changes

slemrmartin added 2 commits February 1, 2019 17:12

Targeted refresh

9de8377

Support for EmbeddedAnsible

b190e11

Refactoring to Shared modules

slemrmartin force-pushed the targeted-refresh branch from d831f1f to 04d61e4 Compare February 1, 2019 16:13

slemrmartin commented Feb 1, 2019

View reviewed changes

slemrmartin mentioned this pull request Feb 4, 2019

Targeted refresh for Embedded ansible provider ManageIQ/manageiq#18429

Merged

1 task

Targeted refresh with batch saver_strategy support

bac0298

Also added hello_workflow configuration_workflow to populate_tower rake task

slemrmartin force-pushed the targeted-refresh branch from 04d61e4 to bac0298 Compare February 4, 2019 16:19

Batch queries to ansible tower rest api

1812424

slemrmartin changed the title ~~[WIP] Targeted refresh~~ Targeted refresh Feb 5, 2019

Ladas approved these changes Feb 6, 2019

View reviewed changes

Ladas added refactoring enhancement and removed wip refactoring labels Feb 6, 2019

agrare merged commit f9d95fb into ManageIQ:master Feb 6, 2019

agrare added a commit to agrare/manageiq-providers-ansible_tower that referenced this pull request Feb 6, 2019

Revert "Merge pull request ManageIQ#153 from slemrmartin/targeted-ref…

7165fec

…resh" This reverts commit f9d95fb, reversing changes made to dfcd8e8.

agrare mentioned this pull request Feb 6, 2019

Revert "Merge pull request #153 from slemrmartin/targeted-refresh" #156

Merged

jameswnl added a commit that referenced this pull request Feb 6, 2019

Merge pull request #156 from agrare/revert_153_targeted-refresh

d4af529

Revert "Merge pull request #153 from slemrmartin/targeted-refresh"

slemrmartin deleted the targeted-refresh branch February 7, 2019 08:20

slemrmartin restored the targeted-refresh branch February 7, 2019 08:21

slemrmartin pushed a commit to slemrmartin/manageiq-providers-ansible_tower that referenced this pull request Feb 11, 2019

Revert "Merge pull request ManageIQ#153 from slemrmartin/targeted-ref…

e40dcd0

…resh" This reverts commit f9d95fb, reversing changes made to dfcd8e8.

slemrmartin added a commit to slemrmartin/manageiq-providers-ansible_tower that referenced this pull request Feb 11, 2019

Comment to make PR possible

99f3020

it's revert of revert's PR ManageIQ#153

slemrmartin mentioned this pull request Feb 11, 2019

Comment to make PR possible #158

Closed

slemrmartin added a commit to slemrmartin/manageiq-providers-ansible_tower that referenced this pull request Feb 11, 2019

Revert "Revert "Merge pull request ManageIQ#153 from slemrmartin/targ…

3666676

…eted-refresh"" This reverts commit 7165fec.

slemrmartin mentioned this pull request Feb 11, 2019

Revert "Revert "Merge pull request #153 from slemrmartin/targeted-refresh" #159

Merged

agrare added a commit that referenced this pull request Feb 11, 2019

Merge pull request #159 from slemrmartin/targeted-refresh-again

2a739f0

Revert "Revert "Merge pull request #153 from slemrmartin/targeted-refresh"

slemrmartin mentioned this pull request Feb 12, 2019

Targeted refresh spec for EmbeddedAnsible ManageIQ/manageiq#18447

Merged

slemrmartin deleted the targeted-refresh branch February 12, 2019 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targeted refresh #153

Targeted refresh #153

slemrmartin commented Jan 11, 2019 •

edited

Loading

agrare Jan 18, 2019 •

edited

Loading

slemrmartin Jan 24, 2019

slemrmartin Jan 24, 2019

agrare Jan 18, 2019

slemrmartin Jan 24, 2019

slemrmartin Feb 1, 2019

agrare commented Jan 18, 2019

Ladas Jan 22, 2019

slemrmartin Feb 1, 2019

slemrmartin Feb 5, 2019 •

edited

Loading

Ladas Jan 22, 2019

slemrmartin Jan 24, 2019

slemrmartin commented Feb 1, 2019

miq-bot commented Feb 1, 2019

miq-bot commented Feb 1, 2019

slemrmartin Feb 1, 2019

Ladas Feb 4, 2019

slemrmartin Feb 4, 2019 •

edited

Loading

Ladas left a comment

slemrmartin commented Feb 6, 2019

		@@ -1,3 +1,3 @@
		class ManageIQ::Providers::AnsibleTower::AutomationManager::Refresher < ManageIQ::Providers::BaseManager::Refresher
		class ManageIQ::Providers::AnsibleTower::AutomationManager::Refresher < ManageIQ::Providers::BaseManager::ManagerRefresher

Targeted refresh #153

Targeted refresh #153

Conversation

slemrmartin commented Jan 11, 2019 • edited Loading

agrare Jan 18, 2019 • 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

agrare commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin commented Feb 1, 2019

miq-bot commented Feb 1, 2019

miq-bot commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

slemrmartin commented Feb 6, 2019

slemrmartin commented Jan 11, 2019 •

edited

Loading

agrare Jan 18, 2019 •

edited

Loading

slemrmartin Feb 5, 2019 •

edited

Loading

slemrmartin Feb 4, 2019 •

edited

Loading