support find and lazy_find by other fields than manager_ref

[cben]

Rationale:
Kubernetes API gives us permanently unique "UID"s, but expresses links to other objects via "name"s that are only only momentarily unique.
[https://kubernetes.io/docs/concepts/overview/working-with-objects/names/]

We want to keep using the UIDs are ems_ref and graph refresh's manager_ref.
But we need ability to lazy_find by name.
This PR is POC for extending InventoryCollection with extra indexes as suggested in
ManageIQ/manageiq-providers-kubernetes#30 (comment)

• incomplete, just the parts I needed
• missing tests & docs
• naming is bad...

@Ladas @agrare Would you have time to flesh this out?

Ideas that might make it friendlier:

• embed which index inside the foo__bar strings, so once constructed you don't need to pass around which index you're referring to ?
  then we can mix all indexes in one hash (this may be a decision that's hard reverse?).
• autodetect index from keys passed to find_by / lazy_find_by ?

[miq-bot]

Checked commit cben@c666ac2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 5 offenses detected

app/models/manager_refresh/inventory_collection.rb

• ⚠️ - Line 357, Col 53 - Lint/ShadowingOuterLocalVariable - Shadowing outer local variable - name.
• ⚠️ - Line 357, Col 59 - Lint/UnusedBlockArgument - Unused block argument - keys. If it's necessary, use _ or _keys as an argument name to indicate that it won't be used.
• ❗ - Line 357, Col 51 - Style/SpaceInsideBlockBraces - Space between { and | missing.
• ❗ - Line 650, Col 9 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.
• ❗ - Line 672, Col 9 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

[agrare]

@cben I like this a lot, I wonder if :manager_ref can just become a special case default of a more general index.

[cben]

Did you by chance make any progress on this / have advice how I should move this forward?

I want to either land something like this soon, or unblock ManageIQ/manageiq-providers-kubernetes#52 by temporarily using data_index...

[Ladas]

@agrare any naming idea? Also would be nice to get the interface right, so it's easier to do to refactoring of the inside.

@agrare @cben
maybe if can be just indexes?

:indexes => {
  :name_label_index => {
    :attributes => [:name, :label],
    :unique => true
  },
   :other_index => {
    :attributes => [:other],
    :unique => false
  },
}

or maybe we should wrap index as an object right away?

:indexes => [
  InventoryCollection::Index.new([:name, :label], :unique => true),
  InventoryCollection::Index.new([:other], :unique => false, :name => "other_index"
]

(we could also extract the @data_index into InventoryCollection::Data)

and we might not need to name the indexes at all I guess, we can just check if the combination of attributes is there as an index, even with O(c) if we use the attribute list as an index?

[agrare]

Yeah I think putting these into one :index or :indexes makes sense, if we are going to pass in names of indexes I think :additional_indexes would be better than :secondary_refs

As for extracting @data_index I think that it would be better to keep it private

[Ladas]

@cben

then we can mix all indexes in one hash (this may be a decision that's hard reverse?).

Right I am thinking that foo__bar can be changed to hash, or object really. The string encoding was there to spare some memory when having millions of records in 1 big hash. Since we go into stream saving, we can bloat the indexes a bit.

autodetect index from keys passed to find_by / lazy_find_by ?

yeah, should be enough to find by some combination of attributes and pick the index according to that, or raise exception that such index is missing

---

Would be nice if we can get the interface right, to avoid replacing it on many places later. But maybe we will anyway. :-)

Looks like this is all we need for the full refresh. We will need to extend the DB loading, for the partial/stream refresh.

[agrare]

@cben can you check if the Amazon and ansible tower refresher specs pass with these changes? If so I'll merge this and we can make the interface better in a follow-up.

[cben]

ansible_tower and amazon tests pass (but 39 skipped "AWS S3 is disabled").

I'm slightly embarassed to have this merged as-is, but will not at all object :-)

[Ladas]

@agrare ok, tests are passing, it's ok to merge. We or I will refactor it later. :-)

@cben just remove the WIP label :-)