Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set vm ancestry from relationship resource info #492

Merged
merged 5 commits into from
Jan 26, 2021

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jun 10, 2020

we are working on converting vm genealogy to use ancestry. In order to do this, we need to use the existing relationship resource information on all vms with genealogy rels of type VmOrTemplate to convert those resource ids into vm ids.

This PR 1) adds the necessary index and 2) gets the ancestry information from the relationships, splitting on slashes, then maps those back to the vm ancestry records.

the first commit's the original sql which joined to vms
the second commit removes that join so we no longer have a potential issue with the ordering

needs ManageIQ/manageiq#20788, tests are green: ManageIQ/manageiq-cross_repo-tests#225

@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 23861b4 to c44d780 Compare June 10, 2020 12:33
Fryguy
Fryguy previously requested changes Jun 10, 2020
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Great idea to get genealogy out of there!

@kbrock
Copy link
Member

kbrock commented Jun 10, 2020

I thought I'd explain the sql a little better.

Here is what we are doing:

  1. Get the relationship ancestry for vms (FROM vms JOIN relationships a_rels). We are only working with genealogy records and vms/templates.
    Cut the relationship ancestry string into an array of ancestry_relationship_ids (STRING_TO_ARRAY, ::BIGINT[]).
  2. Unravel the relationship_ancestry array to one ancestry_relationship_id per record (JOIN LATERAL UNNEST) to make it easier to manipulate. Ensure these ancestry_relationship_id rows are ordered from grandparent to child (WITH ORDINALITY, ORDER BY).
  3. Convert the ancestry_relationship_id values to an ancestry_vm_id (JOIN relationships res_rels). That way we know the values that will go into a vm's ancestry.
  4. Join the ancestry_vm_id rows into an array of ancestry_vm_ids per vm (ARRAY_AGG, GROUP BY) and convert the ancestry_vm_ids to a string (ARRAY_TO_STRING)

@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch 4 times, most recently from 9302c94 to 703ede1 Compare June 10, 2020 19:25
@d-m-u d-m-u changed the title [wip] set vm ancestry from relationship resource info set vm ancestry from relationship resource info Jun 10, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 703ede1 to 3accb28 Compare June 10, 2020 19:36
d-m-u added a commit to d-m-u/manageiq-cross_repo-tests that referenced this pull request Jun 14, 2020
d-m-u added a commit to d-m-u/manageiq-cross_repo-tests that referenced this pull request Jun 14, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 3accb28 to abcef5e Compare June 25, 2020 13:47
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2020

hey @Fryguy could I please ask you to review this again?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 29, 2020

could I please get someone to review this?

@d-m-u d-m-u changed the title set vm ancestry from relationship resource info [WIP] set vm ancestry from relationship resource info Jun 29, 2020
@miq-bot miq-bot added the wip label Jun 29, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from abcef5e to 95a5972 Compare July 4, 2020 05:04
@d-m-u d-m-u changed the title [WIP] set vm ancestry from relationship resource info set vm ancestry from relationship resource info Jul 10, 2020
@miq-bot miq-bot removed the wip label Jul 10, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 95a5972 to 0923423 Compare July 10, 2020 05:16
@kbrock
Copy link
Member

kbrock commented Jul 10, 2020

aside: we have some notes in gist - may want to bring that into this PR for future reference (or not...)

@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch 2 times, most recently from 5929279 to 8c5b559 Compare July 12, 2020 18:05
@d-m-u d-m-u changed the title set vm ancestry from relationship resource info [WIP] set vm ancestry from relationship resource info Jul 13, 2020
@miq-bot miq-bot added the wip label Jul 13, 2020
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 29, 2020

Okay, I'm marking this and the core pr (ManageIQ/manageiq#20788) as work in progress again because I'd like to rewrite the tests.

@d-m-u d-m-u changed the title set vm ancestry from relationship resource info [WIP] set vm ancestry from relationship resource info Nov 29, 2020
@miq-bot miq-bot added the wip label Nov 29, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch 2 times, most recently from c3a869f to 58287bd Compare November 29, 2020 20:41
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 29, 2020

@kbrock could I get another look at this please? I know you'd have let it get merged as it was but this is better and that was eating me.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks nice.

ship it

@d-m-u d-m-u changed the title [WIP] set vm ancestry from relationship resource info set vm ancestry from relationship resource info Dec 2, 2020
@miq-bot miq-bot removed the wip label Dec 2, 2020
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 58287bd to 5715ed3 Compare December 3, 2020 01:08
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 5715ed3 to 25acdaf Compare December 13, 2020 03:19
@d-m-u d-m-u requested review from kbrock and Fryguy December 13, 2020 03:20
@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from 25acdaf to 9bfda8b Compare December 13, 2020 04:40
@Fryguy Fryguy dismissed their stale review January 12, 2021 22:41

Changes applied

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good, but would like @agrare to also review and approve.

@Fryguy
Copy link
Member

Fryguy commented Jan 12, 2021

From a testing perspective, have you run this against a "real" customer database? I'm curious if it migrates correctly. If so, that would give a huge confidence boost. If it fails, then that's more test cases.

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 13, 2021

@Fryguy yes, @NickLaMuro was awesome and provided the customer database that was used to test this. Please see his earlier comment: #492 (comment)

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM after @Fryguy 's two suggestions are fixed

@d-m-u d-m-u force-pushed the set_vm_ancestry_from_relationships branch from ede1806 to 1d7517b Compare January 26, 2021 16:08
@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2021

Checked commits d-m-u/manageiq-schema@9db3c81~...1d7517b with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 1 offense detected

spec/migrations/20200607025146_add_ancestry_to_vm_spec.rb

@agrare agrare merged commit 4498394 into ManageIQ:master Jan 26, 2021
@d-m-u d-m-u deleted the set_vm_ancestry_from_relationships branch January 26, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants