Container Provider Refresh Refactor Image Registries Collection #13724

enoodle · 2017-02-01T11:40:04Z

After collecting images from Openshift new image registries are added
and should be collected. The previous registries are also cleared to
prevent duplicates.

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

enoodle · 2017-02-01T11:40:17Z

cc @zeari

enoodle · 2017-02-01T11:43:14Z

@miq-bot add_label bug, providers/containers

enoodle · 2017-02-01T11:53:17Z

an alternative solution could be to move the collection of openshift images to happen before the kubernetes parsing ( in super ).

simon3z · 2017-02-01T23:38:54Z

@enoodle do we have a bug for this and clear steps to know what was the problem, how to reproduce, etc.? I am sure we'll be asked all these info anyway.

enoodle · 2017-02-02T09:40:13Z

@simon3z I opened a BZ and added to the description

simon3z · 2017-02-02T10:05:20Z

@zeari can you review?
@enoodle @zeari we want to try and merge this before Monday 6th.

chrispy1 · 2017-02-02T14:23:50Z

@miq-bot add_label euwe/yes

enoodle · 2017-02-02T14:44:31Z

I refactored the solution to collect container image registries the moment that we identify them instead of later in a collection function.
Also added tests.

simon3z · 2017-02-02T14:53:06Z

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

@@ -692,6 +686,8 @@ def parse_container_image(image, imageID)
        if stored_container_image_registry.nil?
          @data_index.store_path(
            :container_image_registry, :by_host_and_port, host_port, container_image_registry)
+          @data[:container_image_registries] ||= []
+          @data[:container_image_registries] << container_image_registry


@Fryguy @blomquisg this LGTM but I don't know if there's any implication on not using process_collection for this data (should be OK for as much as I know).

@simon3z an alternative can be using:

process_collection_item(container_image_registry, :container_image_registries) { |r| r }

@enoodle it depends on where you'll put that line. Can you update the PR so we'll see how it looks like.

@simon3z I palced that line and move the initialization to the initialize function. PTAL

simon3z · 2017-02-02T14:53:50Z

LGTM 👍
cc @Fryguy @blomquisg

@miq-bot assign Fryguy

simon3z · 2017-02-02T14:54:52Z

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

@@ -692,6 +686,8 @@ def parse_container_image(image, imageID)
        if stored_container_image_registry.nil?
          @data_index.store_path(
            :container_image_registry, :by_host_and_port, host_port, container_image_registry)
+          @data[:container_image_registries] ||= []


@enoodle maybe we can move this initialization somewhere else?

There's an edge-case where all the images have no registry where this may remain nil, right? (As we'll never hit this code).

we can move this to initialize

simon3z · 2017-02-02T16:11:20Z

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

@@ -9,6 +9,7 @@ def self.ems_inv_to_hashes(inventory)

    def initialize
      @data = {}
+      @data[:container_image_registries] ||= []


@enoodle for sure no need for the || you just defined it above. Maybe you can even do:

@data = { # >>> Here you need a meaningful comment that explains this exception <<< :container_image_registries => [] }

Maybe you can even add a test... a refresh with no registries would it fail if it is nil instead of [] (if not we don't even need to initialize).

@simon3z I tested this and there is no need to initialize this in case no registries are found.
In case no registries are found then @data[:container_image_registries] will return nil, and then hashes[:container_image_registries] in [1] will be nil and in [2] we will return without a failure.

[1]https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_container.rb#L13
[2]https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_container.rb#L325

simon3z · 2017-02-02T23:06:50Z

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

+        expect(parser.instance_variable_get('@data')[:container_image_registries].size).to eq(1)
+      end
+
+      it "collects image registries from openshift images" do


@enoodle what is this testing? Maybe I am missing something or the description can be improved.

@simon3z it uses the :after clause to test, this makes sure that if an image registry is defined only from an image found from Openshift, this image registry will be collected.

@enoodle then it should probably be something more explanatory than "collects image registries from openshift images" which is not fully reflecting what you're testing here.

simon3z · 2017-02-02T23:07:14Z

@enoodle much better now! Really nice!

enoodle · 2017-02-05T10:03:50Z

I changed the spec name to "collects image registries from openshift images that are not also running pods images", also squashed the commits. @simon3z PTAL

enoodle · 2017-02-06T10:57:32Z

ping @simon3z @Fryguy

simon3z · 2017-02-06T14:45:40Z

@enoodle have you verified that this works also without any registry? (container_image_registries with values nil or '[]`)

enoodle · 2017-02-06T14:55:38Z

@simon3z Yes, I run it while manually disabling image/pod collection disabled and saw that in save_inventory_container we get to save_container_image_registries_inventory with empty hash ( hashes == nil and continues silently without failing.

simon3z · 2017-02-06T14:56:41Z

@enoodle OK thanks.

Then waiting for (review/)merge from @Fryguy @chessbyte 👍

Fryguy · 2017-02-06T15:30:50Z

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

@@ -166,6 +166,32 @@
        parser.instance_variable_get('@data_index')[:container_image][:by_ref_and_registry_host_port].values[0])
      expect(parser.instance_variable_get('@data')[:container_images][0][:architecture]).to eq('amd64')
    end
+
+    context "image registries from openshift images" do
+      after :each do


:each here is redundant and not needed.

Fryguy · 2017-02-06T15:31:34Z

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

+
+    context "image registries from openshift images" do
+      after :each do
+        inventory = {"image" => [image_from_openshift,]}


Is that trailing comma expected? It doesn't do anything, so I think it should be removed

Fryguy · 2017-02-06T15:33:06Z

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

+
+        parser.get_openshift_images(inventory)
+        expect(parser.instance_variable_get('@data_index')[:container_image_registry][:by_host_and_port].size).to eq(1)
+        expect(parser.instance_variable_get('@data')[:container_image_registries].size).to eq(1)


Doing expectations in an after is very strange, as after is meant for cleanup/teardown, and it especially makes the it on line 179 very strange as it's empty. If you want to bunch these together to DRY it up, prefer a regular Ruby method that is called from each it, or perhaps even better, use RSpec shared_examples

I changed to a single function parse_single_openshift_image_with_registry as shared_examples seems like something to share between different objects and less between different functions.

This will allow image registries collected from both running pods and openshift images to be collected.

miq-bot · 2017-02-06T16:02:20Z

Checked commit enoodle@dc816de with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

enoodle · 2017-02-06T16:53:19Z

@Fryguy I made the changes, PTAL

…age_registries Container Provider Refresh Refactor Image Registries Collection (cherry picked from commit 82b0db1) https://bugzilla.redhat.com/show_bug.cgi?id=1419680

simaishi · 2017-02-06T18:15:27Z

Euwe backport details:

$ git log -1
commit 6e5ce3176ccb9c5eba7bd0aac0fb43b202866935
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Feb 6 13:04:34 2017 -0500

    Merge pull request #13724 from enoodle/openshift_refresh_recollect_image_registries
    
    Container Provider Refresh Refactor Image Registries Collection
    (cherry picked from commit 82b0db17984edf9e019fb0d644c91d0150bfe34a)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1419680

miq-bot added bug providers/containers labels Feb 1, 2017

chessbyte assigned simon3z Feb 1, 2017

enoodle closed this Feb 1, 2017

enoodle reopened this Feb 1, 2017

miq-bot added the euwe/yes label Feb 2, 2017

enoodle force-pushed the openshift_refresh_recollect_image_registries branch from a15dc70 to cdfe211 Compare February 2, 2017 14:36

simon3z reviewed Feb 2, 2017

View reviewed changes

miq-bot assigned Fryguy and unassigned simon3z Feb 2, 2017

simon3z reviewed Feb 2, 2017

View reviewed changes

enoodle force-pushed the openshift_refresh_recollect_image_registries branch 2 times, most recently from 9eb015c to 45d842f Compare February 2, 2017 15:36

enoodle changed the title ~~Openshift Refresh re collect image registries~~ Container Provider Refresh Refactor Image Registries Collection Feb 2, 2017

simon3z reviewed Feb 2, 2017

View reviewed changes

enoodle force-pushed the openshift_refresh_recollect_image_registries branch from 45d842f to 2f223e3 Compare February 2, 2017 19:30

simon3z reviewed Feb 2, 2017

View reviewed changes

Fryguy assigned blomquisg and unassigned Fryguy Feb 3, 2017

enoodle force-pushed the openshift_refresh_recollect_image_registries branch from 2f223e3 to fcd248b Compare February 5, 2017 10:03

miq-bot assigned Fryguy and unassigned blomquisg Feb 5, 2017

Fryguy reviewed Feb 6, 2017

View reviewed changes

enoodle force-pushed the openshift_refresh_recollect_image_registries branch from fcd248b to 8e2e70c Compare February 6, 2017 15:39

Openshift Refresh to collect image registries on the go

dc816de

This will allow image registries collected from both running pods and openshift images to be collected.

enoodle force-pushed the openshift_refresh_recollect_image_registries branch from 8e2e70c to dc816de Compare February 6, 2017 15:58

simaishi added the blocker label Feb 6, 2017

Fryguy merged commit 82b0db1 into ManageIQ:master Feb 6, 2017

Fryguy added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 6, 2017

simaishi added euwe/backported and removed euwe/yes labels Feb 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Provider Refresh Refactor Image Registries Collection #13724

Container Provider Refresh Refactor Image Registries Collection #13724

enoodle commented Feb 1, 2017 •

edited by miq-bot

enoodle commented Feb 1, 2017

enoodle commented Feb 1, 2017

enoodle commented Feb 1, 2017

simon3z commented Feb 1, 2017

enoodle commented Feb 2, 2017

simon3z commented Feb 2, 2017

chrispy1 commented Feb 2, 2017

enoodle commented Feb 2, 2017

simon3z Feb 2, 2017

enoodle Feb 2, 2017

simon3z Feb 2, 2017

enoodle Feb 2, 2017

simon3z commented Feb 2, 2017

simon3z Feb 2, 2017

enoodle Feb 2, 2017

simon3z Feb 2, 2017

enoodle Feb 2, 2017

simon3z Feb 2, 2017

enoodle Feb 3, 2017

simon3z Feb 3, 2017 •

edited

simon3z commented Feb 2, 2017

enoodle commented Feb 5, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

Fryguy Feb 6, 2017

Fryguy Feb 6, 2017

enoodle Feb 6, 2017

Fryguy Feb 6, 2017

enoodle Feb 6, 2017

miq-bot commented Feb 6, 2017

enoodle commented Feb 6, 2017

simaishi commented Feb 6, 2017

Container Provider Refresh Refactor Image Registries Collection #13724

Container Provider Refresh Refactor Image Registries Collection #13724

Conversation

enoodle commented Feb 1, 2017 • edited by miq-bot

enoodle commented Feb 1, 2017

enoodle commented Feb 1, 2017

enoodle commented Feb 1, 2017

simon3z commented Feb 1, 2017

enoodle commented Feb 2, 2017

simon3z commented Feb 2, 2017

chrispy1 commented Feb 2, 2017

enoodle commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z Feb 3, 2017 • edited

Choose a reason for hiding this comment

simon3z commented Feb 2, 2017

enoodle commented Feb 5, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 6, 2017

enoodle commented Feb 6, 2017

simaishi commented Feb 6, 2017

enoodle commented Feb 1, 2017 •

edited by miq-bot

simon3z Feb 3, 2017 •

edited