Add config option to skip container_images #14606

agrare · 2017-04-01T14:56:49Z

In some OpenShift environments the number of container images can result
in extremely large API responses to the point where the connection will
timeout, as well as save_inventory times can be dramatically longer.

This adds a config option that if set to false will not request
container images from the /ImageList endpoint and will not parse them in
the RefreshParser.

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

cben · 2017-04-03T17:43:51Z

config/settings.yml

@@ -133,6 +133,7 @@
    :refresh_interval: 15.minutes
  :openshift:
    :refresh_interval: 15.minutes
+    :get_container_images: true


note that if you backport / make a hotfix, i believe euwe had both :openshift: & :openshift_enterprise: types, both will need this true default.

(or could change the logic to double negative unless refresher_options.get_container_images == false, which should preserve normal behavior if setting is missing)

Thanks @cben I didn't realize that!

cben

cc @enoodle. I think we want to merge this in any case, and you may want to base your following work on this.

cben · 2017-04-04T08:17:08Z

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

@@ -25,16 +25,19 @@ def fetch_hawk_inv(ems)
      end

      def parse_legacy_inventory(ems)
+        request_entities = OPENSHIFT_ENTITIES.dup
+        request_entities.delete(:name => 'images') if refresher_options.skip_container_images


Consider turning OPENSHIFT_ENTITIES into a method.
I don't like there being a const that's not always the correct value. Though now that you flipped it to include images unless skip, it's pretty OK as is.

cben · 2017-04-04T08:18:39Z

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

@@ -1,14 +1,14 @@
 module ManageIQ::Providers
  module Openshift
    class ContainerManager::RefreshParser < ManageIQ::Providers::Kubernetes::ContainerManager::RefreshParser
-      def ems_inv_to_hashes(inventory)
+      def ems_inv_to_hashes(inventory, options = Config::Options.new)


Forgot one thing: since you added options args to Kubernetes parser too, let's super(inventory, options) below.

Good catch I missed that one

enoodle

LGTM

Fryguy · 2017-04-04T14:10:17Z

config/settings.yml

@@ -133,6 +133,7 @@
    :refresh_interval: 15.minutes
  :openshift:
    :refresh_interval: 15.minutes
+    :skip_container_images: false


I prefer to use positive-oriented option names. This way, you don't get into the unless !no_ping type of code unreadability. skip: false is already a double-negative.

In amazon we used the option named get_public_images... See https://github.com/ManageIQ/manageiq-providers-amazon/blob/master/config/settings.yml#L32 . I recommend get_images: true or something similar here.

In some OpenShift environments the number of container images can result in extremely large API responses to the point where the connection will timeout, as well as save_inventory times can be dramatically longer. This adds a config option that if set to false will not request container images from the /ImageList endpoint and will not parse them in the RefreshParser.

miq-bot · 2017-04-04T15:04:11Z

Checked commit agrare@9ec23fb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 👍

agrare · 2017-04-04T19:56:50Z

@Fryguy changed it over to :get_container_images: true

Checked what happens to previous image data when image collection is disabled. => The unused images are disconected, the used image metadata is not deleted. stub_settings is risky, as it omits all other settings, and code may rely on default settings for sane behavior. stub_settings_merge is safer.

…ipped Add config option to skip container_images (cherry picked from commit d3ebee7) https://bugzilla.redhat.com/show_bug.cgi?id=1484548

simaishi · 2017-08-23T20:30:21Z

Fine backport details:

$ git log -1
commit c1f88dddd9ebb1fefdfd99126b09b5a419980b28
Author: Greg Blomquist <[email protected]>
Date:   Wed Apr 5 12:10:03 2017 -0400

    Merge pull request #14606 from agrare/allow_container_images_to_be_skipped
    
    Add config option to skip container_images
    (cherry picked from commit d3ebee7c480c8e88ca6066f505fc922daceb4943)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1484548

simaishi · 2017-08-23T20:40:21Z

@agrare For Euwe branch, does :get_container_images: true in config/settings.yml need to go to both :openshift: and :openshift_enterprise: ?

agrare · 2017-08-23T20:46:16Z

@simaishi yes I'll make an euwe PR for this

agrare · 2017-08-23T20:55:51Z

@simaishi #15885

simaishi · 2017-08-23T23:58:57Z

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

…elete-spec Test improvements for #14606 (cherry picked from commit 6354a4e) https://bugzilla.redhat.com/show_bug.cgi?id=1484548

…to_be_skipped Add config option to skip container_images (cherry picked from commit d3ebee7)

simaishi · 2017-08-25T21:51:23Z

Backported to Euwe via #15885

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

…-false-delete-spec Test improvements for ManageIQ#14606 https://bugzilla.redhat.com/show_bug.cgi?id=1484548 (cherry picked from master commit 6354a4e via [FINE] cherry pick commit 0c1fb68 which adjusted for stub_settings_merge not existing yet, then adjusted for different [EUWE] vcr cassette)

…age-metadata [EUWE] Test improvements for #14606

…to_be_skipped Add config option to skip container_images (cherry picked from commit d3ebee7) https://bugzilla.redhat.com/show_bug.cgi?id=1484548

…-false-delete-spec Test improvements for ManageIQ#14606 (cherry picked from commit 6354a4e) https://bugzilla.redhat.com/show_bug.cgi?id=1484548

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

agrare requested a review from Fryguy April 1, 2017 14:56

miq-bot added the wip label Apr 1, 2017

agrare added the providers/containers label Apr 3, 2017

chessbyte assigned blomquisg Apr 3, 2017

chessbyte added the enhancement label Apr 3, 2017

cben reviewed Apr 3, 2017

View reviewed changes

agrare force-pushed the allow_container_images_to_be_skipped branch from 0628baf to 1f09545 Compare April 3, 2017 19:18

cben approved these changes Apr 4, 2017

View reviewed changes

cben reviewed Apr 4, 2017

View reviewed changes

enoodle approved these changes Apr 4, 2017

View reviewed changes

enoodle mentioned this pull request Apr 4, 2017

Container image post refresh from openshift #14628

Closed

agrare changed the title ~~[WIP] Add config option to skip container_images~~ Add config option to skip container_images Apr 4, 2017

agrare removed the wip label Apr 4, 2017

Fryguy reviewed Apr 4, 2017

View reviewed changes

agrare force-pushed the allow_container_images_to_be_skipped branch 2 times, most recently from 121759f to 1dd92a7 Compare April 4, 2017 14:49

agrare force-pushed the allow_container_images_to_be_skipped branch from 1dd92a7 to 9ec23fb Compare April 4, 2017 14:56

chessbyte requested a review from simon3z April 5, 2017 09:03

cben approved these changes Apr 5, 2017

View reviewed changes

blomquisg merged commit d3ebee7 into ManageIQ:master Apr 5, 2017

blomquisg added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017

agrare deleted the allow_container_images_to_be_skipped branch April 5, 2017 16:12

cben mentioned this pull request Apr 19, 2017

[WIP] Reconnect container images when seen again #14808

Closed

agrare added the fine/yes label Aug 22, 2017

cben mentioned this pull request Aug 23, 2017

Test improvements for #14606 #14661

Merged

simaishi added fine/backported and removed fine/yes labels Aug 23, 2017

agrare mentioned this pull request Aug 23, 2017

[EUWE] Add config option to skip container_images #15885

Merged

simaishi pushed a commit that referenced this pull request Aug 23, 2017

Merge pull request #14661 from cben/does-get_openshift_images-false-d…

0c1fb68

…elete-spec Test improvements for #14606 (cherry picked from commit 6354a4e) https://bugzilla.redhat.com/show_bug.cgi?id=1484548

agrare pushed a commit to agrare/manageiq that referenced this pull request Aug 24, 2017

Merge pull request ManageIQ#14606 from agrare/allow_container_images_…

d89ceea

…to_be_skipped Add config option to skip container_images (cherry picked from commit d3ebee7)

simaishi added euwe/backported and removed euwe/yes labels Aug 25, 2017

This was referenced Aug 28, 2017

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

Merged

Should re-connect container images that are used again ManageIQ/manageiq-providers-kubernetes#103

Closed

cben mentioned this pull request Aug 30, 2017

[EUWE] Test improvements for #14606 #15910

Merged

simaishi added a commit that referenced this pull request Aug 31, 2017

Merge pull request #15910 from cben/EUWE-openshift-specs-container-im…

f8491a7

…age-metadata [EUWE] Test improvements for #14606

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config option to skip container_images #14606

Add config option to skip container_images #14606

agrare commented Apr 1, 2017 •

edited

Loading

cben Apr 3, 2017 •

edited

Loading

agrare Apr 3, 2017

cben left a comment

cben Apr 4, 2017

cben Apr 4, 2017

agrare Apr 4, 2017

enoodle left a comment

Fryguy Apr 4, 2017

agrare Apr 4, 2017

miq-bot commented Apr 4, 2017

agrare commented Apr 4, 2017

simaishi commented Aug 23, 2017

simaishi commented Aug 23, 2017

agrare commented Aug 23, 2017

agrare commented Aug 23, 2017

simaishi commented Aug 23, 2017

simaishi commented Aug 25, 2017

Add config option to skip container_images #14606

Add config option to skip container_images #14606

Conversation

agrare commented Apr 1, 2017 • edited Loading

cben Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

agrare Apr 3, 2017

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

cben Apr 4, 2017

Choose a reason for hiding this comment

cben Apr 4, 2017

Choose a reason for hiding this comment

agrare Apr 4, 2017

Choose a reason for hiding this comment

enoodle left a comment

Choose a reason for hiding this comment

Fryguy Apr 4, 2017

Choose a reason for hiding this comment

agrare Apr 4, 2017

Choose a reason for hiding this comment

miq-bot commented Apr 4, 2017

agrare commented Apr 4, 2017

simaishi commented Aug 23, 2017

simaishi commented Aug 23, 2017

agrare commented Aug 23, 2017

agrare commented Aug 23, 2017

simaishi commented Aug 23, 2017

simaishi commented Aug 25, 2017

agrare commented Apr 1, 2017 •

edited

Loading

cben Apr 3, 2017 •

edited

Loading