Test improvements for #14606

[cben]

Checked what happens to previous image data when image collection is disabled (#14606).
=> Unused images are disconnected; both those and still-used ones retain metadata.

• stub_settings is risky, as it omits all other settings, and things may
  rely on default settings for sane behavior. Changed to stub_settings_merge.

@miq-bot add-label providers/containers, test
cc @agrare @Ladas @Fryguy

[Ladas]

@cben yeah, the images should be disconnected (ems_id foreign key set to nil), though when we allow them again, they will not be reconnected, we will create another record for the image

[cben]

Yes, I expected they'll be disconnected but test ~~suggests they aren't~~. `expect(@container_image.ext_management_system).to eq(@ems)` However let me improve test to really confirm, `ContainerImage.count` is not the condition I want...

[Ladas]

@cben right so, I don't see it on a first sight how is disabling of :container_images behaving, but this is how the old saving works:
https://github.com/Ladas/manageiq/blob/6e2ed956d8ec84ec0131e9efce9587ad4edd3c85/app/models/ems_refresh/save_inventory_container.rb#L12-L12

if you pass hashes[:container_images] = nil, or not even have the key :container_images, it will skip the processing, so leaving what was there intact

if you pass hashes[:container_images] = [], it will disconnect all container_images. And it should also disconnect missing container_images.

Can you verify what are we passing there?

[miq-bot]

Checked commit cben@9b77bec with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

[cben]

save_container_images_inventory array size 40
save_container_images_inventory array size 12
  will not delete previously collected metadata if get_container_images = false

save_container_images_inventory array size 12
  will skip container_images if get_container_images = false

save_container_images_inventory array size 40
save_container_images_inventory array size 40
  will perform a full refresh on openshift

=> Aha, unused images are disconnected; both those and still-used ones retain metadata.
This is the perfect behavior IMO.
Updated the test to verify all this.

[Ladas]

@cben right, but since we don reconnect the image, then this might break some reports for container.container_image relation?

container.container_image.id # => '1'
container.container_image.disconnect_inv
# refresh
container.container_image.id # => '2'
# and we still have disconnected image with id 1 in the db

but this happens also elsewhere (except saving VmOrTemplate in the old refresh), we were talking about this with @Fryguy , I would kind of like doing https://github.com/Ladas/manageiq/blob/4ad0054c6a92d2b6ee63437b4f1508fb0a6952e5/app/models/container_definition.rb#L16-L16 in general, we could base the generic reconnect logic on that, with tweaks for region migrations, where it could reconnect from multiple managers

[cben]

An image should never be disconnected while referenced by container(s) — refresh parser emits these images even when get_openshift_images is false, and I believe these tests cover that.
ContainerImage#disconnect_inv does set old_ems_id.

@Ladas Do you mean what happens after both container and image were disconnected? I think their relation should still hold, container's container_image_id is not erased.
@zeari I suspect until recently images were not really getting disconnected. In theory yes, the but I'm not sure they ever disappear from /oapi/v1/images (?). Now we have several options where they'll disconnect as soon as no container runs them — do we have / do you think we need more tests for reports with this scenario?

@Ladas or do you mean what happens after container and image are disconnected and then a new container appears using this image (or get_openshift_images is turned back on) — will the existing record get reconnected to the EMS or will we get a new image record?
@zeari I believe we want to reconnect the existing record, having many records for same hash would wreak havoc with reports, right?
Good question, I'll add a test...

[Ladas]

@cben "Do you mean what happens after both container and image were disconnected? I think their relation should still hold, container's container_image_id is not erased." that is correct

"or do you mean what happens after container and image are disconnected and then a new container appears using this image (or get_openshift_images is turned back on) — will the existing record get reconnected to the EMS or will we get a new image record?" this is the case, unless there is a reconnect logic, it will create a duplicate record of a container image (so in the end there can be many disconnected duplicates of the same image)

you can try with the example in #14661 (comment) . It will probably be a good example for a spec too, to manually disconnect 1 record and check the next refresh just reconnects it (the :id must be the same).

---

that being said, the generic reconnect logic is not part of the old refresh nor Graph refresh. So we will need to add it. The most effective way should be, to pass AREL limiting where we can look for disconnected entities and then do 1 query to fetch IDs of records that are planned to be created. Then it would do a reconnect instead of the create.

Should I prepare that logic for you?

[cben]

Indeed they are not reconnected :-( cc @simon3z @agrare
Failing spec: 79d76f9
After turning get_container_images = false, 28 out of 40 images get disconnected, 12 remain;
then setting back get_container_images = true, 28 new records are created.

save_container_images_inventory calls save_inventory_multi with association = ems.container_images, which contains just the 12 connected images, the disconnected 28 are not indexed into record_index, so they become new_records...

EDIT: tried passing
association = ContainerImage.where('ems_id = ? or old_ems_id = ?', ems.id, ems.id)
but that doesn't support .push.
Would need separate params for association to refresh, and extra disconnected entities to revive or something like that.

[Ladas]

@cben could you create a PR with the spec, then I can help you to modify the saving code.

We need another relation than association, otherwise it would be disconnecting your disconnected things all the times. So we need a ContainerImage.where('ems_id = ? or old_ems_id = ?', ems.id, ems.id) only for the update_attributes code path

[cben]

Created #14808 with a fix, except it's not good.
I relied on association.push to reconnect, but that only restores ems_id doesn't clear old_ems_id, deleted_on. Let's move discussion there.

Can we merge this PR? It's just tests for stuff that's already in. (the reconnection test is not here since it's failing).

[Ladas]

yes, this looks good to be merged 👍

[zeari]

@zeari I suspect until recently images were not really getting disconnected. In theory yes, the but I'm not sure they ever disappear from /oapi/v1/images (?). Now we have several options where they'll disconnect as soon as no container runs them — do we have / do you think we need more tests for reports with this scenario?

as long as youre not severing the relationship container.container_image then we should be fine in reporting. (disconnecting them from the provider is ok)

@zeari I believe we want to reconnect the existing record, having many records for same hash would wreak havoc with reports, right?

depends on your definition of havoc. We would probably get some duplicate lines on the report but again, nothing would break if the relationships are intact. A unique key for a row in a chargeback report would be "#{project.id}_#{image.id}_#{timestamp_key}"which uses rails AR ids.

[simaishi]

Fine backport details:

$ git log -1
commit 0c1fb6841d28c243d85e1833496aa6444a358718
Author: Adam Grare <agrare@redhat.com>
Date:   Wed Apr 19 08:40:27 2017 -0400

    Merge pull request #14661 from cben/does-get_openshift_images-false-delete-spec
    
    Test improvements for #14606
    (cherry picked from commit 6354a4e4ae23a394ee02ef543c385dd61f47fb8e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1484548

[simaishi]

Backported to Euwe via #15910