Map Amazon labels to tags #14436

djberg96 · 2017-03-21T17:31:30Z

This PR maps custom attributes on the AWS side to tags on the ManageIQ side based on mappings that have been setup by a user.

See also ManageIQ/manageiq-providers-amazon#183 which sets the :tags attribute on Vm's and Images, which you will need for this PR to have any noticeable effect.

Once you have your Gemfile pointing at that fork and the "aws_tags3" branch, you can perform the following steps to see it in action:

Within ManageIQ go to Configuration -> Region -> Map Tags. Add an entry, selecting "Amazon::Vm" as the entity, type in "owner" as the resource label, and whatever you want for the category, e.g. "amazon_vm_owner". Then refresh your Amazon provider (or add one if you haven't already). Any VM with a custom attribute of "owner" will now show up as a company tag on the instance view.

Use the "US East - Northern Virginia" region from our Dev environment if you want to make sure you get some VM's with an "owner" attribute.

Ladas · 2017-03-22T09:57:57Z

app/models/ems_refresh/save_inventory.rb

+    return unless object.kind_of?(VmOrTemplate)
+
+    tags = hash[:tags].collect do |tag_hash|
+      ContainerLabelTagMapping.find_or_create_tag(tag_hash)


hm, this method looks super ineffective for being run multiple times for each vm_or_template (huge n+1)

https://github.com/Ladas/manageiq/blob/3231e3993e01b3b8078ec5efada9c9c66b20479c/app/models/container_label_tag_mapping.rb#L75-L75

@kbrock seems to be super ineffective to be call here find_or_create_tag

@Ladas Keep in mind that it's not going to map every tag, only those that were matched in the refresh parser. In practice I wouldn't expect more than 1-2 per VM. Is my guess incorrect? Also, I don't see a way around it, so suggestions welcome.

right, we should rewrite the find_or_create_tag, so we don't do the n+1 queries there

so we should first process all the tags to pickup the unique combinations. Then load all existing in one query and create the missing.

If this could lead to huge number of AR object in memory, it would be nice to store only id then.

is the find_or_create_tag used in some refresh already?

I see, it's in https://github.com/Ladas/manageiq/blob/b5f2f37f3beab86f3f71f127bb3c2ae0b0ea4294/app/models/ems_refresh/save_inventory_container.rb#L430-L430

Maybe we should just move this whole save_tags_inventory here?

@kbrock are you fine with using the ineffective way and then fixing the whole ContainerLabelTagMapping separately? Also we should extract it from ContainerLabelTagMapping, since it's a generic tagging problem, not just for Containers?

@Ladas Unless the ems is a ContainerManager, that won't get invoked. I guess I'm not sure what or how you're looking to use that.

n+1 — hehe, you found where I buried the bodies ;-\

Confusingly, all SaveInventoryFoo modules are mixed into one namespace, doesn't matter in which file they are. Please move save_tags_inventory here from save_inventory_container.rb, rather than creating 2nd identically named method.

@cben It does have the additional restriction of limiting to VmOrTemplate, though. Or do you feel that's not necessary?

Ah, didn't notice VmOrTemplate. But IIUC you can't really have 2 methods with same name, only one wins (probably the save_containers one without restriction, or tests would fail).
Why do you need this restriction? You control which models get tagged by choosing where to calls to save_tags_inventory (typically via :tags in child keys lists). Also, in what situation would you expect to have computed map_labels, only to throw it away here?

djberg96 · 2017-03-22T16:04:04Z

@cben thoughts?

djberg96 · 2017-03-22T19:23:12Z

@Ladas, ok, I simplified it to use retag_entity, mostly similar to the equivalent container method.

@cben, I don't suppose there's a version of retag_entity that enforces policy out there, is there? I poked around a bit and didn't see anything. It's not critical for this release, but would be nice.

Ladas · 2017-03-23T08:51:19Z

@djberg96 @cben so as I try to explain here ManageIQ/manageiq-providers-amazon#194 (comment) , we should not be probably doing tagging in a refresh at all. Since we will not be able to apply the policies later (correctly)

So I would be rather if we would move tagging to post refresh, using ^ for creating non existent tags and then queuing separate jobs, that would take care of correct tag assignment.

A correct interface for assigning a tag should be https://github.com/Ladas/manageiq/blob/cf9e566a86adafd9d9717499b13e6b4492d2c317/app/models/classification.rb#L107-L107 (and unclassify_by_tag) since it is being used in automate. @Fryguy is that correct interface for the Classification? Seems like https://github.com/Ladas/manageiq/blob/cf9e566a86adafd9d9717499b13e6b4492d2c317/app/models/classification.rb#L127 is also invoking the right policies.

Remove the save_tags_inventory method, use the one in save_inventory.rb. Remove type check, add 3rd argument for backwards compatibility. Update taggings tests, add node tests.

djberg96 · 2017-03-26T17:07:53Z

@cben, as per your suggestion, I moved the save_tags_inventory method out of save_inventory_container.rb and removed the type check, so everything should be using the same method now, although I did add a check to allow either a hash[:tags] or a plain array of hashes.

Regarding the specs, I don't explicitly test the mappings since those have their own tests already. Instead the data array assumes the results of the mappings. Is there any issue with that? One thing I wasn't sure about was how to map/test for "ALL" on the container side. If you have any suggestions there please let me know.

And please double check this to make sure I haven't broken anything on the container side. :)

Ladas · 2017-03-27T06:59:54Z

app/models/ems_refresh/save_inventory.rb

+  #
+  def save_tags_inventory(object, collection, _target = nil)
+    return if collection.blank?
+    tags = collection.is_a?(Hash) ? collection[:tags] : collection


do we actually use the Hash somewhere? The saving code should also pass it as a child, so collection[:tags] should be always passed, right?

bronaghs · 2017-03-27T14:56:41Z

@cben - can you approve this PR so we can get it in today before the feature freeze? Thanks

bronaghs · 2017-03-27T15:00:02Z

@Fryguy - this story has been committed to the Fine release and needs merged today. Once @cben approves this PR @djberg96 will remove the WIP label. Can you please review. thanks.

bronaghs · 2017-03-27T15:00:17Z

https://www.pivotaltracker.com/story/show/140981377

Ladas · 2017-03-27T15:01:11Z

@djberg96 would be nice to fix the rubocop issues?

@blomquisg looks good to merge, but we should create the issues for the Policies and the Performance at the same time :-)

miq-bot · 2017-03-27T16:13:42Z

Checked commits https://github.com/djberg96/manageiq/compare/eaf2277d27b0b25188c9d4b839e345547e599929~...e7334400a6367083e65ff9bc9e4663809f9d19c6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 👍

djberg96 · 2017-03-27T16:38:16Z

@miq-bot remove_label wip

bronaghs · 2017-03-27T16:41:24Z

@Fryguy @blomquisg - this is good to go and has to be merged today to meet the feature freeze deadline, thanks.
cc @chessbyte

cben

LGTM. Nice test 👍

cben · 2017-03-27T18:54:23Z

spec/models/ems_refresh/save_inventory/save_tags_inventory_spec.rb

+
+    @tag1 = FactoryGirl.create(:tag, :name => '/managed/amazon:vm:owner')
+    @tag2 = FactoryGirl.create(:tag, :name => '/managed/kubernetes:container_node:stuff')
+    @tag3 = FactoryGirl.create(:tag, :name => '/managed/kubernetes:foo') # All


[Completely doesn't matter in this test: An actual "All" mapping would use /managed/kubernetes::foo with double colon.]

cben · 2017-03-27T19:05:36Z

app/models/ems_refresh/save_inventory.rb

+  # {:category_tag_id=>139, :entry_name=>"foo", :entry_description=>"bar"}
+  #
+  # The +collection+ argument can either be a Hash, in which case the argument
+  # should have a single :tags key, or a simple Array of hashes.


I'm a little surprised you need to deal with {:tags => ...} form, IIUC ManageIQ/manageiq-providers-amazon#183 creates :labels and :tags on same level, you have :labels and :tags in child keys on same level, and save_child_inventory calls save_#{k}_inventory on hashes[k] so I'd expect save_tags_inventory to not see the :tags, just the value.

But I don't mind.

@cben This is something we plan to refactor in the future, i.e. generate the tags on the core side rather than the refresher side.

agrare

LGTM

Fryguy · 2017-03-27T21:04:31Z

Thanks @agrare ... I've been swamped in meetings and customer issues :(

miq-bot added the wip label Mar 21, 2017

Ladas reviewed Mar 22, 2017

View reviewed changes

Map labels to tags.

eaf2277

chessbyte assigned Fryguy Mar 22, 2017

chessbyte added core enhancement providers/amazon labels Mar 22, 2017

djberg96 added 2 commits March 24, 2017 14:23

Initial commit.

668adfd

Did some refactoring, added some tests.

93937df

Remove the save_tags_inventory method, use the one in save_inventory.rb. Remove type check, add 3rd argument for backwards compatibility. Update taggings tests, add node tests.

djberg96 added 3 commits March 26, 2017 14:36

Restore backwards compatibility with original Container handling.

90d0f39

Rubocop fixes.

ccd3f0f

Updated variable names and comments.

27eff65

Ladas reviewed Mar 27, 2017

View reviewed changes

djberg96 mentioned this pull request Mar 27, 2017

Add tags for VMs and Images ManageIQ/manageiq-providers-amazon#183

Merged

1 task

More rubocop fixes.

e733440

djberg96 changed the title ~~[WIP] Map Amazon labels to tags~~ Map Amazon labels to tags Mar 27, 2017

miq-bot removed the wip label Mar 27, 2017

cben approved these changes Mar 27, 2017

View reviewed changes

agrare approved these changes Mar 27, 2017

View reviewed changes

agrare merged commit e788b2b into ManageIQ:master Mar 27, 2017

agrare added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017

djberg96 deleted the tag_mappings3 branch July 17, 2017 18:48

cben mentioned this pull request Aug 20, 2017

CustomAttribute->Tag mapping should go through Policy? #15861

Closed

4 tasks

Fryguy added the providers/cloud label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map Amazon labels to tags #14436

Map Amazon labels to tags #14436

djberg96 commented Mar 21, 2017

Ladas Mar 22, 2017

djberg96 Mar 22, 2017

Ladas Mar 22, 2017

Ladas Mar 22, 2017

djberg96 Mar 22, 2017

cben Mar 22, 2017

djberg96 Mar 22, 2017

cben Mar 22, 2017

djberg96 commented Mar 22, 2017

djberg96 commented Mar 22, 2017

Ladas commented Mar 23, 2017

djberg96 commented Mar 26, 2017 •

edited

Loading

Ladas Mar 27, 2017

bronaghs commented Mar 27, 2017

bronaghs commented Mar 27, 2017

bronaghs commented Mar 27, 2017

Ladas commented Mar 27, 2017

miq-bot commented Mar 27, 2017

djberg96 commented Mar 27, 2017 •

edited

Loading

bronaghs commented Mar 27, 2017 •

edited

Loading

cben left a comment

cben Mar 27, 2017

cben Mar 27, 2017

djberg96 Mar 27, 2017

agrare left a comment

Fryguy commented Mar 27, 2017

Map Amazon labels to tags #14436

Map Amazon labels to tags #14436

Conversation

djberg96 commented Mar 21, 2017

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

djberg96 commented Mar 22, 2017

djberg96 commented Mar 22, 2017

Ladas commented Mar 23, 2017

djberg96 commented Mar 26, 2017 • edited Loading

Choose a reason for hiding this comment

bronaghs commented Mar 27, 2017

bronaghs commented Mar 27, 2017

bronaghs commented Mar 27, 2017

Ladas commented Mar 27, 2017

miq-bot commented Mar 27, 2017

djberg96 commented Mar 27, 2017 • edited Loading

bronaghs commented Mar 27, 2017 • edited Loading

cben 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

agrare left a comment

Choose a reason for hiding this comment

Fryguy commented Mar 27, 2017

djberg96 commented Mar 26, 2017 •

edited

Loading

djberg96 commented Mar 27, 2017 •

edited

Loading

bronaghs commented Mar 27, 2017 •

edited

Loading