Archive nodes in graph refresh

[zeari]

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

@cben @moolitayer Please review
Trying to come up with specs

@miq-bot add_label gaprindashvili/yes

[cben]

fix LGTM, needs spec

[Ladas]

We should also include reconnect https://github.com/zeari/manageiq-providers-kubernetes/blob/6b738fdfbbaba6b8c0a558be57096e394ef9b28f/app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb#L141

and add specs for reconnect e.g. extending https://github.com/Ladas/manageiq-providers-kubernetes/blob/a93bcc90847e4a1cb65cb2e95625abe4a622f750/spec/models/manageiq/providers/kubernetes/container_manager/targeted_refresh/targeted_refresh_spec.rb#L49

and checking the node existing i nthe db is reconnected and not duplicated

[cben]

I don't know if you re-add a previously removed node to the cluster, will it have same UID? @elad661 you said yesterday you know how to add nodes, so maybe you have an idea? (this is not about a node being temporarily down, then it'd still exist in list of nodes but have NotReady condition)

[Ladas]

@cben right, that would be for more complex reconnect, which we could do, if there is something to match when UUID changes. The reconnect I am suggesting would be mostly for consistency, to avoid duplication, before the unique indexes are in place.

[elad661]

you said yesterday you know how to add nodes, so maybe you have an idea?

@cben I thought that the way to add a node to openshift (idk if it's different in kubernetes) is to run the installer again on the same cluster, with the new node listed in the inventory. It didn't work for a node that was already a member of the cluster before - my guess is you have to uninstall openshift on the node before you run the installer again, so it's probable that it'll have a new UID. This is only a guess, I haven't actually tried it (Ari gave up on his cluster after the first attempt, and just deployed a new one).

If needed, I can give it another try and see if the UID changes.

[cben]

@Ladas if uid changes, we don't want reconnect. I was hoping that it always changes, therefore we won't need reconnect ;-)

In any case, this is an edge case of little consequence - there is little conceptual difference between new node and rejoined node...
I'm in favor of merging and tracking in an issue.

[Ladas]

@cben I'll leave it to your expertise then. Unique indexes should be merged this release, so maybe we do not need to bother with reconnect. :-)

[Ladas]

@cben we should still add the specs though

[cben]

Test LGTM 👍.
Codeclimate can be ignored, InventoryCollection definitions are inherently "similar blocks of code".

[zeari]

ping @moolitayer Added a spec

[moolitayer]

Can we please add a count like in lines 504 505 for the container nodes?

[cben]

maybe also move the create after 1st refresh than before 2nd. Then we'd have:

• "1st refresh pretending it saw an extra node"
• counts: 2 active nodes
• 2nd refresh
• counts: 1 active 1 archived node

fits better into the mental mold than 1 active -> 1 active 1 archived?

[miq-bot]

Checked commits zeari/manageiq-providers-kubernetes@e6eca12~...d48271d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[moolitayer]

LGTM 👍

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 9073e19b108140be97560e3e194979ecd5c62b91
Author: Mooli Tayer <[email protected]>
Date:   Tue Jan 16 15:21:47 2018 +0200

    Merge pull request #209 from zeari/archive_nodes
    
    Archive nodes in graph refresh
    (cherry picked from commit 3087870235e1ded39b057c18fcd4223fafd9be6a)