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

Graph refresh: links between entities #52

Merged
merged 1 commit into from
Jul 11, 2017
Merged

Conversation

cben
Copy link
Contributor

@cben cben commented Jun 26, 2017

Followup to #30, implements most entity links in get_*_graph.

Now kubernetes graph refresh passes full refresher tests!

  • Except tagging by labels which is skipped.

A few things starting with _ indicate still missing functionality that is not covered by refresher_spec, will add tests later.

Mostly using lazy_find by secodary indexes on name or (namespace, name).
(enabled by ManageIQ/manageiq#15447)

  • Still using @data_index for images & registries.
  • Used a local hash similar to data_index for endpoint/service matching.

@miq-bot miq-bot added the wip label Jun 26, 2017
return nil if hash.nil?
# Subtle: we can match replicators by project name - instead of project ems_ref -
# because when indexed their :container_project a itself lazy_find :by_name,
# so its .to_s is the project name.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ladas @agrare This troubles me ^^
Even without secondary refs, the indirection when manager_ref contains links to other entities, is magic when it works but scared me a bit...

Here I'd prefer to keep :namespace in the replicator data, and explicitly use it in indexes. I need to blacklist it so ActiveRecord .create doesn't complain. When I tried this before, InventoryCollection didn't let me blacklist things that are parts of manager_ref — can we relax this (or not enforce it for secondary indexes)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we can relax the condition for the secondary indexes. It make sense only for primary index, otherwise you would not be able to identify the same record later.

Yeah having lazy_link as part of the index is funky :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, switched to index by blacklisted :namespace & name.

@cben cben force-pushed the graph-links branch 7 times, most recently from 0bda567 to b031e16 Compare July 3, 2017 15:51
@cben
Copy link
Contributor Author

cben commented Jul 3, 2017

This now passes referher specs, except tag mapping :-)
But it depends on WIP ManageIQ/manageiq#15447, which is why Travis fails.
@enoodle @zeari @zakiva @moolitayer @yaacov @agrare please review.

  • There are still some disabled code parts (things starting with _), which means they lack tests. I'll work on this in followup PRs.
  • Endpoints parsing into container_groups<->container_services many-to-many uses a local data_index-like hash. This probably can be rewritten to directly populate an InventoryCollection for the join table; I'd prefer to tackle this in a later PR.

@cben cben changed the title [WIP] Graph refresh: links between entities Graph refresh: links between entities Jul 5, 2017
@miq-bot miq-bot removed the wip label Jul 5, 2017
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.

👍

@simon3z
Copy link
Contributor

simon3z commented Jul 5, 2017

@cben can you squash the commits? I think we're ready for merge.

@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2017

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

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@cben
Copy link
Contributor Author

cben commented Jul 5, 2017

Squashed & updated PR description.
But wait, another failure on Travis. => Should be fixed by #55

@cben cben closed this Jul 6, 2017
@cben cben reopened this Jul 6, 2017
@cben cben mentioned this pull request Jul 9, 2017
24 tasks
@cben cben force-pushed the graph-links branch 3 times, most recently from 37396e9 to 074ab01 Compare July 11, 2017 08:11
Now kubernetes graph refresh passes full refresher tests!
- Except tagging by labels which is skipped.

A few things starting with _ indicate still missing functionality that
is not covered by refresher_spec, will add tests later.

Mostly using lazy_find by secodary indexes on name or (namespace, name).
- Still using data_index for images & registries.
- Used a local hash similar to data_index for endpoint/service
matching.
@cben
Copy link
Contributor Author

cben commented Jul 11, 2017

@simon3z ready for merge?

  • fixed a wrong call in get_nodes_graph.
  • simplified lazy_find_* methods using lazy_find_by.
  • snuck in a few more :attributes_blacklist => [:namespace] so I can consistently [:namespace] without having to .delete(:namespace) in some cases.

@simon3z simon3z merged commit 600cda2 into ManageIQ:master Jul 11, 2017
@moolitayer moolitayer added this to the Sprint 65 Ending Jul 24, 2017 milestone Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants