Implement tag mapping in graph refresh #382

cben · 2017-12-21T18:11:58Z

Depends on Additions for Amazon tag mapping in graph refresh manageiq#16734 — merged

Subsumes #384, done with lots of help from Ladislav & Julian 🌍

https://bugzilla.redhat.com/show_bug.cgi?id=1506404

cc @bronaghs @djberg96 @Ladas @agrare (in case anybody still here, and happy holidays)

cben · 2018-01-03T13:09:15Z

@juliancheal @Ladas This is still WIP but I think something like this...

Depends on couple core changes: ManageIQ/manageiq#16734

cben · 2018-01-03T13:20:12Z

rebased

cben · 2018-01-03T13:28:48Z

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

+      }
+
+      attributes[:targeted_arel] = lambda do |inventory_collection|
+        ids = inventory_collection.parent_inventory_collections.collect_concat(&:id)


I was trying to "simplify" this to something like I did containers and avoid going through Vm.vm_and_template_taggings relation, just because it scared me.

But add_inventory_collection requires :association => :foos to be set, this determines the name of the persister.foos accessor to the collection :-(
Anyway, now that I added that relation in ManageIQ/manageiq#16734 I should probably do what vm_and_template_labels does...

Ladas · 2018-01-03T13:59:50Z

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

+      }
+
+      attributes[:targeted_arel] = lambda do |inventory_collection|
+        ids = inventory_collection.parent_inventory_collections.collect_concat(&:id)


ids are not available here yet, so we need to use
manager_uuids = inventory_collection.parent_inventory_collections.collect(&:manager_uuids).map(&:to_a).flatten

manager uuids are :ems_ref columns of Vm in this case. So we need a nested query that transforms that to ids

Ladas · 2018-01-03T14:05:54Z

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

+        ids = inventory_collection.parent_inventory_collections.collect_concat(&:id)
+        Tagging.where(
+          'taggable_type' => 'VmOrTemplate',
+          'taggable_id'   => ids


so then here it would be

'taggable_id' => inventory_collection.parent.vm_and_templates.where(:ems_ref => manager_uuids)

Ladas · 2018-01-03T15:14:41Z

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

        Tagging.where(
          'taggable_type' => 'VmOrTemplate',
-          'taggable_id'   => ids
-        ).joins(:tag).merge(Tag.controlled_by_mapping)
+          'taggable_id'   => ems.vm_and_template_taggings.where(:ems_ref => manager_uuids)


just

ems.vm_and_templates.where(:ems_ref => manager_uuids)

since the manager uuids are fro ma table VmOrTemplate

thanks, fixed

Ladas · 2018-01-03T16:07:33Z

.../models/manageiq/providers/amazon/cloud_manager/vcr_specs/refresher_inventory_object_spec.rb

@@ -32,6 +33,7 @@
          end

          assert_common
+          assert_mapped_tags_on_vm


could we move this inside of the specific Vms asserts?

Especially assert_specific_vm_powered_on, assert_specific_vm_on_cloud_network_public_ip and assert_specific_vm_on_cloud_network

That way it will be automatically tested in old, new and targeted refresh

done for vms

For templates the only label I found at all is Name 😐
and both assert_specific_template* have 0 labels (not even Name) so only calling this function explicitly in some tests.
This means targeted image taggings refresh is not well tested.

Ladas · 2018-01-03T16:09:11Z

spec/models/manageiq/providers/amazon/aws_refresher_spec_common.rb

-  #   expect(vm.tags).to be_empty
-  # end
+  def assert_specific_labels_on_vm
+    vm = ManageIQ::Providers::Amazon::CloudManager::Vm.find_by(:name => "ladas_test_5")


We are usually testing only Vms having EmsRefreshSpec prefix, those are the ones nobody is touching (so we can re-record VCR easily)

Ladas · 2018-01-03T16:12:43Z

app/models/manageiq/providers/amazon/inventory/parser/cloud_manager.rb

    # The order of the below methods doesn't matter since they refer to each other using only lazy links
+    persister.collections[:tags_to_resolve] = @tag_mapper.tags_to_resolve_collection


cool, you already use custom IC here, I was about to ask about it :-)

@cben could we move the IC definition into the defaults, and the init into the Persitors? Like we do with :vm_and_template_taggings

I could. Note that constructing the Mapper actually reads whole mappings table.

Is it OK to do it each Persister initialization?

Will it still be re-read each refresh? (eg. if it's only read once when refresh worker starts that would be bad, changing mapping would have no effect).

Conversely, can re-reading it each targetted refresh be a performance problem?

Will Persister initialization remain a good place with collector/persister split?

hum, ok, if it reads the whole mapping table, I will need to look on it more closely tomorrow. We will need to limit it only to Vms and Templates that are part of the targeted refresh

Is it OK to do it each Persister initialization?

Right now that would be the same as having it in parser, that is being called each refresh

Will it still be re-read each refresh? (eg. if it's only read once when refresh worker starts that would be bad, changing mapping would have no effect).

Yes, right now, the persister defines ICs dynamically (that might change in the future, as some providers use static definition, but there can be always a dynamic part)

Conversely, can re-reading it each targetted refresh be a performance problem?

Yes looking at this, it will definitely hit targeted refresh performance, with huge amount of tags present

Will Persister initialization remain a good place with collector/persister split?

There will be some refactoring of the format, but the Persistor should remain mostly the same.

https://bugzilla.redhat.com/show_bug.cgi?id=1506404

cben · 2018-01-03T18:00:40Z

Hooray! I was stuck on mysterious hardware,disk,network count mismatches, turned out I dropped a line in merging conflicts 🤦‍♂️
This works now for VMs 🎉
Adding images...

cben · 2018-01-03T22:35:12Z

PTAL. Added images, addressed most comments, except #382 (comment)

~~Don't understand~~ how refresher_other_region_spec.rb passes ~~tags check~~ when I didn't create_tag_mapping? EDIT: Ah, it has its own copy of assert_specific_vm_powered_on. Can't call assert_mapped_tags_on_vm there, that vm has no owner label.

Ladas · 2018-01-03T22:54:22Z

@cben yes, another copy of assert_specific_vm_powered_on. I think the specs are great now, we can ignore the other region specs

Ladas · 2018-01-04T09:40:06Z

app/models/manageiq/providers/amazon/inventory/parser/cloud_manager.rb

    # The order of the below methods doesn't matter since they refer to each other using only lazy links
+    persister.collections[:tags_to_resolve] = @tag_mapper.tags_to_resolve_collection


@cben ok, so continuing from #382 (comment)

lets move this to persister init, we can put shared method to ManageIQ::Providers::Amazon::Inventory::Persister and just call it in Cloud and TargetCollection Persisters for now

And we should there add a TODO for perf, that this will always load all ContainerLabelTagMapping (or other?). But this table is fairly small? It's the Tagging that is huge, right?

Mapper init is reading ContainerLabelTagMapping.all. It's fairly small, 1 per mapping defined in UI, independent of inventory size and number of tags created/assigned.
Moving...

was about to ask about TargetCollection then saw you already anticipated that :-)
Done, PTAL.

Ladas

@cben so lets just move the code as stated in https://github.com/ManageIQ/manageiq-providers-amazon/pull/382/files#r159611330

and I think we can merge. Just confirm that we are not loading the hugest table please. :-)

cben · 2018-01-04T12:02:37Z

I believe the "targeting" of the targeted_arel is not really tested. It does create the expected taggings, but are we sure it doesn't delete other taggings?
However, I'll leave this to you folks to test better if you wish.

Targeted query, as diff from normal ems.vm_and_template_taggings query:

 SELECT "taggings".* FROM "taggings"
 INNER JOIN "tags" ON "tags"."id" = "taggings"."tag_id"
 LEFT OUTER JOIN "classifications" ON "classifications"."tag_id" = "tags"."id" 
 INNER JOIN "vms" ON "taggings"."taggable_id" = "vms"."id" 
 WHERE "vms"."ems_id" = 12000000009214 
   AND "taggings"."taggable_type" = 'VmOrTemplate' 
   AND (("tags"."name" LIKE '/managed/amazon:%') OR ("tags"."name" LIKE '/managed/kubernetes:%'))
   AND "classifications"."read_only" = 't' 
   AND ("classifications"."parent_id" != 0) 
+  AND "taggings"."taggable_type" = 'VmOrTemplate' 
+  AND "taggings"."taggable_id" IN (SELECT "vms"."id" FROM "vms" WHERE "vms"."ems_id" = 12000000009214)

UPDATE: was a bug, fix below

cben · 2018-01-04T12:10:47Z

Hmm, that last SELECT seems wrong! I don't see a ems_ref constraint on vms. Debugging...

The taggable_type = 'VmOrTemplate' constraint is duplicate, I'm going to I drop it from targeted_arel.

Unneccessary for same reason as in vm_and_template_labels targeted_arel. Already covered by ems.vms_and_templates, which is :through => :vms_and_templates.

Was querying taggings of ALL vms, which would make targeted refresh delete most taggings.

miq-bot · 2018-01-04T13:07:12Z

Checked commits cben/manageiq-providers-amazon@6141841~...688cf2f with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 1 offense detected

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

❗ - Line 196, Col 24 - Style/ExtraSpacing - Unnecessary spacing detected.

cben · 2018-01-04T13:17:26Z

OK, this was silly. Turns out record.association(args) (note missing .where, see last commit) ignores the args, sort-of.

The reason associations accept args at all is side effect of a deprecated interface, good thing it's going away:

[4] pry(main)> ems.hosts
  Host Load (3.0ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."ems_id" = $1  [["ems_id", 7]]

[5] pry(main)> ems.hosts(:ems_ref => 'ignored')
DEPRECATION WARNING: Passing an argument to force an association to reload is now deprecated and will be removed in Rails 5.1. Please call `reload` on the result collection proxy instead. (called from <main> at (pry):5)
  Host Load (0.4ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."ems_id" = $1  [["ems_id", 7]]

Fixed targeted query, again as diff from ems.vm_and_template_taggings query:

 SELECT "taggings".* FROM "taggings" 
 INNER JOIN "tags" ON "tags"."id" = "taggings"."tag_id" 
 LEFT OUTER JOIN "classifications" ON "classifications"."tag_id" = "tags"."id" 
 INNER JOIN "vms" ON "taggings"."taggable_id" = "vms"."id" 
 WHERE "vms"."ems_id" = 12000000010061 
   AND "taggings"."taggable_type" = 'VmOrTemplate' 
   AND (("tags"."name" LIKE '/managed/amazon:%') OR ("tags"."name" LIKE '/managed/kubernetes:%'))
   AND "classifications"."read_only" = 't' 
   AND ("classifications"."parent_id" != 0)
+  AND "taggings"."taggable_id" IN (SELECT "vms"."id" FROM "vms" 
+                                   WHERE "vms"."ems_id" = 12000000010061 
+                                     AND "vms"."ems_ref" IN ('i-c72af2f6', 'ami-2051294a'))

I think this supports the need for a targeted-after-full-doesn't-delete spec :-), but will leave that to you.
I believe this is ready now.

Ladas · 2018-01-04T13:20:19Z

app/models/manageiq/providers/amazon/inventory_collection_default/cloud_manager.rb

@@ -193,7 +193,7 @@ def vm_and_template_taggings(extra_attributes = {})
        manager_uuids = inventory_collection.parent_inventory_collections.collect(&:manager_uuids).map(&:to_a).flatten
        ems = inventory_collection.parent
        ems.vm_and_template_taggings.where(
-          'taggable_id'   => ems.vms_and_templates(:ems_ref => manager_uuids)
+          'taggable_id'   => ems.vms_and_templates.where(:ems_ref => manager_uuids)


Ha that'll do it

Ladas

Thank you @cben this is awesome work. 👍 🥇 🎉

You are right, we should have specs to make sure targeted refresh doesn't delete anything out of it's scope. I think we can do simple spec just by combining existing full refresh with each targeted refresh spec we have. (so using existing VCRs)

agrare

👍 thanks @cben !!

juliancheal · 2018-01-04T13:31:33Z

👍 thanks @cben

Implement tag mapping in graph refresh (cherry picked from commit 53cc7b0) https://bugzilla.redhat.com/show_bug.cgi?id=1531291

simaishi · 2018-01-04T23:20:22Z

Gaprindashvili backport details:

$ git log -1
commit 7bdb89b65ad4f39dfe65ab60ef62d99ea2f820cc
Author: Ladislav Smola <[email protected]>
Date:   Thu Jan 4 14:31:04 2018 +0100

    Merge pull request #382 from cben/tag-mapping
    
    Implement tag mapping in graph refresh
    (cherry picked from commit 53cc7b01248aa5addc0e8d36d1f1dc152de75acf)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1531291

miq-bot added the wip label Dec 21, 2017

cben force-pushed the tag-mapping branch from e9af8b8 to 3c70945 Compare December 22, 2017 07:26

cben mentioned this pull request Dec 27, 2017

tag mapping with graph refresh ManageIQ/manageiq-providers-kubernetes#126

Closed

23 tasks

cben mentioned this pull request Jan 3, 2018

Additions for Amazon tag mapping in graph refresh ManageIQ/manageiq#16734

Merged

2 tasks

cben force-pushed the tag-mapping branch from 5f065db to b2be4c5 Compare January 3, 2018 13:19

cben commented Jan 3, 2018

View reviewed changes

Ladas reviewed Jan 3, 2018

View reviewed changes

juliancheal mentioned this pull request Jan 3, 2018

[WIP] Using the new tag mapping for graph refresh #384

Closed

Ladas reviewed Jan 3, 2018

View reviewed changes

cben force-pushed the tag-mapping branch from e0c40b2 to 4f1b26e Compare January 3, 2018 15:50

Ladas reviewed Jan 3, 2018

View reviewed changes

cben and others added 3 commits January 3, 2018 19:27

Check tag mapping in VCR refresher specs

6141841

https://bugzilla.redhat.com/show_bug.cgi?id=1506404

Tag mapping in graph refresh: find/create Tags

8468ba4

Tag mapping in graph refresh: assign Taggings

bfb0cdf

cben force-pushed the tag-mapping branch from 4f1b26e to bfb0cdf Compare January 3, 2018 17:58

cben changed the title ~~[WIP] Check tag mapping in VCR refresher specs~~ Implement tag mapping in VCR refresher specs Jan 3, 2018

cben changed the title ~~Implement tag mapping in VCR refresher specs~~ Implement tag mapping in graph refresh Jan 3, 2018

miq-bot removed the wip label Jan 3, 2018

map image tags

e0b279d

cben force-pushed the tag-mapping branch from 518509f to 5d8aa65 Compare January 3, 2018 22:11

cben added 2 commits January 4, 2018 00:22

Check tag mapping on images

281bad5

Check VM mapped tags from assert_specific_vm_*

1ae5cf7

cben force-pushed the tag-mapping branch from 5d8aa65 to 1ae5cf7 Compare January 3, 2018 22:22

Ladas reviewed Jan 4, 2018

View reviewed changes

Ladas self-assigned this Jan 4, 2018

Ladas added bug gaprindashvili/yes labels Jan 4, 2018

Ladas suggested changes Jan 4, 2018

View reviewed changes

Move tag Mapper from parser to persister

2b85fe1

cben force-pushed the tag-mapping branch from 5a3e701 to 2b85fe1 Compare January 4, 2018 11:52

cben added 2 commits January 4, 2018 14:59

drop redundant taggable_type constraint

307a608

Unneccessary for same reason as in vm_and_template_labels targeted_arel. Already covered by ems.vms_and_templates, which is :through => :vms_and_templates.

Fix taggings targeted_arel to actually be targeted!

688cf2f

Was querying taggings of ALL vms, which would make targeted refresh delete most taggings.

Ladas reviewed Jan 4, 2018

View reviewed changes

Ladas approved these changes Jan 4, 2018

View reviewed changes

agrare approved these changes Jan 4, 2018

View reviewed changes

Ladas merged commit 53cc7b0 into ManageIQ:master Jan 4, 2018

Ladas added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 4, 2018

simaishi pushed a commit that referenced this pull request Jan 4, 2018

Merge pull request #382 from cben/tag-mapping

7bdb89b

Implement tag mapping in graph refresh (cherry picked from commit 53cc7b0) https://bugzilla.redhat.com/show_bug.cgi?id=1531291

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 4, 2018

juliancheal mentioned this pull request Feb 28, 2018

[FINE] Fine tag mapping #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tag mapping in graph refresh #382

Implement tag mapping in graph refresh #382

cben commented Dec 21, 2017 •

edited

Loading

cben commented Jan 3, 2018 •

edited

Loading

cben commented Jan 3, 2018

cben Jan 3, 2018

Ladas Jan 3, 2018

Ladas Jan 3, 2018

Ladas Jan 3, 2018 •

edited

Loading

cben Jan 3, 2018

Ladas Jan 3, 2018

cben Jan 3, 2018

Ladas Jan 3, 2018

Ladas Jan 3, 2018

Ladas Jan 3, 2018 •

edited

Loading

cben Jan 3, 2018

Ladas Jan 3, 2018

Ladas Jan 4, 2018

cben commented Jan 3, 2018 •

edited

Loading

cben commented Jan 3, 2018 •

edited

Loading

Ladas commented Jan 3, 2018

Ladas Jan 4, 2018

cben Jan 4, 2018

cben Jan 4, 2018

Ladas left a comment

cben commented Jan 4, 2018 •

edited

Loading

cben commented Jan 4, 2018

miq-bot commented Jan 4, 2018

cben commented Jan 4, 2018

Ladas Jan 4, 2018

agrare Jan 4, 2018

Ladas left a comment

agrare left a comment

juliancheal commented Jan 4, 2018

simaishi commented Jan 4, 2018

		# The order of the below methods doesn't matter since they refer to each other using only lazy links
		persister.collections[:tags_to_resolve] = @tag_mapper.tags_to_resolve_collection

Implement tag mapping in graph refresh #382

Implement tag mapping in graph refresh #382

Conversation

cben commented Dec 21, 2017 • edited Loading

cben commented Jan 3, 2018 • edited Loading

cben commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jan 3, 2018 • 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

Ladas Jan 3, 2018 • 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

cben commented Jan 3, 2018 • edited Loading

cben commented Jan 3, 2018 • edited Loading

Ladas commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

cben commented Jan 4, 2018 • edited Loading

cben commented Jan 4, 2018

miq-bot commented Jan 4, 2018

cben commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

juliancheal commented Jan 4, 2018

simaishi commented Jan 4, 2018

cben commented Dec 21, 2017 •

edited

Loading

cben commented Jan 3, 2018 •

edited

Loading

Ladas Jan 3, 2018 •

edited

Loading

Ladas Jan 3, 2018 •

edited

Loading

cben commented Jan 3, 2018 •

edited

Loading

cben commented Jan 3, 2018 •

edited

Loading

cben commented Jan 4, 2018 •

edited

Loading