Openshift graph refresh #34

cben · 2017-07-12T13:00:21Z

Depended on Changes needed for openshift graph refresh manageiq-providers-kubernetes#57 [merged]
Running all VCR tests, both older cassette and the newer deletion tests.

RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1470021

miq-bot · 2017-07-17T14:59:04Z

This pull request is not mergeable. Please rebase and repush.

cben · 2017-07-20T21:25:38Z

This is now ready. @enoodle @zakiva @zeari @Ladas @agrare please review.

Got delayed as enabling the deletion tests on graph refresh revealed some issues, notably with get_container_images=false mode, and extending that test revealed some more... (Yay tests!)

One tiny point I'm not fixing in this PR, because it's also this way with old refresh, is images tag column — in get_container_images=false mode it's not preserved like the rest of the metadata but "regresses" to the kubernetes parser value.
Fixing that in graph refresh would be easy matter of blacklist_attributes!. But I really want exactly same behavior. Fixing in both would be uglier: eca2260.

P.S. get_container_images=false mode is getting annoying to support, I would like to drop it in favor of store_unused_images=false mode proposed in #9.

P.S. @Ladas has some more fixes but they're not covered by current specs. We'll do them in later PR(s).

cben · 2017-07-24T12:32:35Z

ping . @enoodle @zakiva @zeari please review.

agrare · 2017-07-24T13:47:36Z

P.S. get_container_images=false mode is getting annoying to support, I would like to drop it in favor of store_unused_images=false mode proposed in #9.

I'm good with that now that we have a way of increasing the timeout on the collection

cben · 2017-07-26T07:25:05Z

@moolitayer @enoodle @yaacov @zgalor @zakiva @zeari Please review — this is blocking delivering graph refresh for Scale team to test, and some upcoming PRs...

Ladas

This looks great. 👍

I thought this was already in, I was wondering yesterday why we don't test the graph refresh in OpenShift gem. :-)

moolitayer · 2017-07-26T09:24:17Z

spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb

+      expect(ManageIQ::Providers::Openshift::ContainerManager::RefreshParser).not_to receive(:ems_inv_to_inv_collections)
+    end
+
+    include_examples "openshift refresher VCR tests"


moolitayer · 2017-07-26T09:49:27Z

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

+      end
+
+      # We rely on Kubernetes initialize_inventory_collections setting up our collections too.
+      # TODO: does it mean kubernetes refresh would wipe out the openshift tables?


@cben can you explain why? initialize_inventory_collections is filling in some metadata (and the ems) why would that wipe tables?

Yeah, if you would run kubernetes refresh on openshift provider, it would delete the tables you are not sending data for. But we don't do that (running another's manager type refresh), since that would break anywhere. :-)

The parser returns all @inv_collections.values as the collections to sync.
So it always includes e.g. ems.container_templates. Persistor doesn't know it's empty because we didn't fill it, or empty because all templates were deleted in openshift, if it finds any ems.container_templates in DB it will delete them.

These collections should always be empty in DB for a kubernetes provider, so it's not really a problem. I want to see where @Ladas's upcoming restructurings (including ManageIQ/manageiq-providers-kubernetes#73) end up before moving openshift collections here...

But already had to move image labels to initialize_inventory_collections here, for this very reason, so I'll move this comment to the super call there.

right, I moved the initialize_inventory_collections a bit, now it's under Openshift Persister
#39

moolitayer · 2017-07-26T10:07:08Z

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

+        inventory["project"].each do |data|
+          h = parse_project(data)
+          # TODO: Assumes full refresh, and running after get_namespaces_graph.
+          #   Would be a problem with partial refresh.


Must run after get_namespaces_graph` seems like a fair assumption (we have lots of those)

Can you rephrase the TODO's as assertions?
(I mean something like # Must run after get_namespaces_graph, # need modification for partial refresh, etc)
I'm not sure there is actually something todo here right now, so let's bring in as little TODO as possible.

Old refresh had these assumptions (order & full refresh) everywhere.
Graph refresh is pretty close to not having any :-), and there are concrete wins to making them independent.
But yeah, there is more to track for partial refresh, I'll make assertions not TODOs.

Add ems_refresh.openshift.inventory_object_refresh setting. Run refresher_spec file for both old and graph refresh. Graph refresh preserve images metadata with get_container_images=false (except tag column).

cben · 2017-07-26T11:49:53Z

Rewrote comments, PTAL.

miq-bot · 2017-07-26T11:54:18Z

Checked commit cben@94f3a5a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

zakiva

Changes look good 👍 (pending Travis)

cben · 2017-07-27T07:37:56Z

Kicking Travis as master is now green

moolitayer · 2017-07-27T09:17:01Z

@cben travis failure seems to be related to ContainerDefinition removal

moolitayer

LGTM 👍

cben · 2017-07-27T09:19:18Z

Strange, I thought ContainerDefinition has been all fixed, checking... => Ari fixed in #40

Manually cherry-picked out of ManageIQ/manageiq-providers-openshift#34 Useful to better test the backported get_container_images option (ManageIQ#14606). https://bugzilla.redhat.com/show_bug.cgi?id=1484548

miq-bot added the wip label Jul 12, 2017

cben force-pushed the graph-openshift branch from 2a0e7cd to 59068cb Compare July 12, 2017 13:34

This was referenced Jul 13, 2017

Changes needed for openshift graph refresh ManageIQ/manageiq-providers-kubernetes#57

Merged

Containers graph refresh ManageIQ/manageiq-providers-kubernetes#27

Closed

miq-bot added the unmergeable label Jul 17, 2017

cben force-pushed the graph-openshift branch from 59068cb to f70f730 Compare July 17, 2017 16:18

miq-bot removed the unmergeable label Jul 17, 2017

cben force-pushed the graph-openshift branch from f70f730 to e096a65 Compare July 17, 2017 16:25

cben closed this Jul 18, 2017

cben reopened this Jul 18, 2017

cben force-pushed the graph-openshift branch 4 times, most recently from edfe550 to dc4f1e2 Compare July 20, 2017 21:02

cben changed the title ~~[WIP] Openshift graph refresh~~ Openshift graph refresh Jul 20, 2017

miq-bot removed the wip label Jul 20, 2017

cben closed this Jul 24, 2017

cben reopened this Jul 24, 2017

Ladas approved these changes Jul 26, 2017

View reviewed changes

moolitayer reviewed Jul 26, 2017

View reviewed changes

Ladas mentioned this pull request Jul 26, 2017

Add openshift persister and test also batch saver strategy #39

Merged

2 tasks

moolitayer reviewed Jul 26, 2017

View reviewed changes

Implement graph refresh

94f3a5a

Add ems_refresh.openshift.inventory_object_refresh setting. Run refresher_spec file for both old and graph refresh. Graph refresh preserve images metadata with get_container_images=false (except tag column).

cben force-pushed the graph-openshift branch from dc4f1e2 to 94f3a5a Compare July 26, 2017 11:49

zakiva approved these changes Jul 26, 2017

View reviewed changes

cben closed this Jul 27, 2017

cben reopened this Jul 27, 2017

moolitayer approved these changes Jul 27, 2017

View reviewed changes

cben closed this Jul 27, 2017

cben reopened this Jul 27, 2017

zeari approved these changes Jul 27, 2017

View reviewed changes

moolitayer added the enhancement label Jul 27, 2017

moolitayer merged commit 35adfc1 into ManageIQ:master Jul 27, 2017

moolitayer added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 9, 2017

cben mentioned this pull request Aug 28, 2017

[FINE] openshift refresher_spec: test more container_image fields ManageIQ/manageiq#15899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openshift graph refresh #34

Openshift graph refresh #34

cben commented Jul 12, 2017 •

edited

Loading

miq-bot commented Jul 17, 2017

cben commented Jul 20, 2017

cben commented Jul 24, 2017

agrare commented Jul 24, 2017

cben commented Jul 26, 2017

Ladas left a comment

moolitayer Jul 26, 2017

moolitayer Jul 26, 2017

Ladas Jul 26, 2017

cben Jul 26, 2017

Ladas Jul 26, 2017

moolitayer Jul 26, 2017

cben Jul 26, 2017

cben commented Jul 26, 2017

miq-bot commented Jul 26, 2017

zakiva left a comment

cben commented Jul 27, 2017

moolitayer commented Jul 27, 2017

moolitayer left a comment

cben commented Jul 27, 2017 •

edited

Loading

Openshift graph refresh #34

Openshift graph refresh #34

Conversation

cben commented Jul 12, 2017 • edited Loading

miq-bot commented Jul 17, 2017

cben commented Jul 20, 2017

cben commented Jul 24, 2017

agrare commented Jul 24, 2017

cben commented Jul 26, 2017

Ladas left a comment

Choose a reason for hiding this comment

moolitayer Jul 26, 2017

Choose a reason for hiding this comment

moolitayer Jul 26, 2017

Choose a reason for hiding this comment

Ladas Jul 26, 2017

Choose a reason for hiding this comment

cben Jul 26, 2017

Choose a reason for hiding this comment

Ladas Jul 26, 2017

Choose a reason for hiding this comment

moolitayer Jul 26, 2017

Choose a reason for hiding this comment

cben Jul 26, 2017

Choose a reason for hiding this comment

cben commented Jul 26, 2017

miq-bot commented Jul 26, 2017

zakiva left a comment

Choose a reason for hiding this comment

cben commented Jul 27, 2017

moolitayer commented Jul 27, 2017

moolitayer left a comment

Choose a reason for hiding this comment

cben commented Jul 27, 2017 • edited Loading

cben commented Jul 12, 2017 •

edited

Loading

cben commented Jul 27, 2017 •

edited

Loading