Delete Container/AWS labels that no longer exist on the provider.

[bronaghs]

When a label (tag) has been deleted on a resource on AWS, that label should be deleted in the MIQ database too but instead it persists.

Steps to reproduce:

1. Execute an EMS refresh in MIQ
2. Open an instance in MIQ and observe the contents of the Labels box in the Summary page
3. Delete one of the tags on the instance using the AWS console
4. Execute an EMS refresh in MIQ
5. Observe the tag deleted in step 3) still persists.

@cben -

1. Does this bug also apply to labels in Containers too?
2. Will this fix break anything on the Containers side?

[bronaghs]

@miq-bot add_label wip, providers, bug
@miq-bot assign @cben

[cben]

Haven't tested yet but I think this will be no-op for containers, as we don't do targeted (all this if target.kind_of? boilerplate is unused), for us target is always EMS.

So Amazon refresh now also relies on save_inventory_container.rb ?
Ah, I see, https://github.com/ManageIQ/manageiq/pull/14202/files#diff-090b07f911cee85c2acff15e6bda1032R42 added :labels to child_keys in save_vms_inventory,
and despite apparent namespacing they're all mixed in to EmsRefresh.

What do you think of moving save_labels_inventory, save_custom_attribute_attribute_inventory to a place where it's clear they are shared, eg. save_inventory.rb ?

[bronaghs]

@cben - yes, they need to be moved. Just not before feature freeze :-)

[cben]

If you don't want to move these functions to save_inventory.rb, please add a comment that they're also serving Amazon refresh.
(The move should be a trivial textual move, there is nothing else to refactor. Again, these modules all end up in one namespace.)

[simon3z]

@cben @bronaghs +1 to move this in save_inventory.rb.

[bronaghs]

Hi @cben, @simon3z
Agreed. In fact, there are multiple places where we are re-using the containers tagging code, we will make a separate PR that will move it all to a generic place.

[bronaghs]

@djberg96 - l heard on SU that you are working on refactoring the shared code referred to here. Can you include the code used in this PR?

[djberg96]

@bronaghs Not this particular code atm. I've got a couple PR's in to do a project-wide renaming of ContainerLabelTagMapping to ResourceLabelTagMapping, but that's it for now.

[cben]

Sorry. Finally tested this.
This is not a no-op — we did have the bug, and this fixes it! Thanks 🏅 🙇‍♂️

Target was not the ems as I thought — it was nil, so deletes is []!
Our boilerplate that we believed inactive, just "preparation" in hope one day we'll do refresh with target != EMS, was actually biting us :-(
The reason is that this (and many many functions here) is called from save_child_inventory which is called from save_inventory_multi with child_keys, which never passes down the target:
https://github.com/ManageIQ/manageiq/blob/62eddfb06ef00/app/models/ems_refresh/save_inventory_helper.rb#L57

@simon3z @moolitayer @zakiva This seems bad. Many things are not getting deleted in our refresh! Also, we need refresher specs where things actually change between 2 refreshes.

LGTM for this PR, let's not block you any further, seems we'll need a big followup...

[bronaghs]

@cben - glad this fixed this issue for you too.
FYI this Amazon spec tests the cleanup of deleted objects, this is probably a good example for your team to start from.

[moolitayer]

@cben so all entities that are not called on first level from save_ems_container_inventory are not being deleted when they are removed? please create a bz for this issue.

[moolitayer]

@cben so all entities that are not called on first level from save_ems_container_inventory are not being deleted when they are removed? please create a bz for this issue.

Never mind, I created https://bugzilla.redhat.com/show_bug.cgi?id=1436132

[miq-bot]

Checked commits bronaghs/manageiq@29cf052~...676022e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

[chessbyte]

@cben @bronaghs bump

[miq-bot]

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

[cben]

This was covered by the finally merged #15182 (also find/backported).
Sorry, forgot that it also affects amazon provider and forgot to cc you.
Now may be a good time to revive and re-run ManageIQ/manageiq-providers-amazon#205 tests :)

[bronaghs]

This was fixed by #15182 (thanks @cben )
Closing.