Backport

ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

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

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on fine,
adjusted for different test method signature :connected vs :archived)

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

@@ -667,7 +667,8 @@ def parse_container_state(state_hash)
       res
     end
 
-    def parse_container_image(image, imageID)
+    # may return nil if store_new_images = false
+    def parse_container_image(image, imageID, store_new_images: true)
       container_image, container_image_registry = parse_image_name(image, imageID)
       host_port = nil
 
@@ -691,6 +692,7 @@ def parse_container_image(image, imageID)
         :container_image, :by_digest, container_image_identity)
 
       if stored_container_image.nil?
+        return nil unless store_new_images
         @data_index.store_path(
           :container_image, :by_digest,
           container_image_identity, container_image

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

@@ -8,13 +8,13 @@ def ems_inv_to_hashes(inventory, options = Config::Options.new)
         get_builds(inventory)
         get_build_pods(inventory)
         get_templates(inventory)
-        get_openshift_images(inventory) if options.get_container_images
+        get_openshift_images(inventory, options) if options.get_container_images
         EmsRefresh.log_inv_debug_trace(@data, "data:")
         @data
       end
 
-      def get_openshift_images(inventory)
-        inventory["image"].each { |img| parse_openshift_image(img) }
+      def get_openshift_images(inventory, options)
+        inventory["image"].each { |img| parse_openshift_image(img, options) }
       end
 
       def get_builds(inventory)
@@ -169,10 +169,11 @@ def parse_env_variables(env_variables)
         end
       end
 
-      def parse_openshift_image(openshift_image)
+      def parse_openshift_image(openshift_image, options)
         id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name]
         ref = "#{ContainerImage::DOCKER_PULLABLE_PREFIX}#{id}"
-        new_result = parse_container_image(id, ref)
+        new_result = parse_container_image(id, ref, :store_new_images => options.store_unused_images)
+        return if new_result.nil? # store_unused_images = false and wasn't mentioned by any pod
 
         if openshift_image[:dockerImageManifest].present?
           begin

config/settings.yml

@@ -144,6 +144,7 @@
   :openshift:
     :refresh_interval: 15.minutes
     :get_container_images: true
+    :store_unused_images: true
   :raise_vm_snapshot_complete_if_created_within: 15.minutes
   :refresh_interval: 24.hours
   :scvmm:

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

@@ -676,6 +676,15 @@
         expect(first_obj).to be(second_obj)
       end
     end
+
+    it "returns existing image or nil with store_new_images=false" do
+      obj1 = parser.parse_container_image(shared_image_without_host, shared_ref)
+      obj2 = parser.parse_container_image(shared_image_without_host, shared_ref, :store_new_images => false)
+      obj3 = parser.parse_container_image(shared_image_without_host, unique_ref, :store_new_images => false)
+      expect(obj1).not_to be nil
+      expect(obj2).to be obj2
+      expect(obj3).to be nil
+    end
   end
 
   describe "cross_link_node" do

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

@@ -1,7 +1,34 @@
 require 'recursive-open-struct'
 
 describe ManageIQ::Providers::Openshift::ContainerManager::RefreshParser do
+  let(:store_unused_images) { true }
+  let(:options) do
+    # using Struct not OpenStruct ensures we specify all options the code actually accesses
+    Struct.new(:store_unused_images).new(store_unused_images)
+  end
   let(:parser) { described_class.new }
+  let(:parser_data) { parser.instance_variable_get('@data') }
+  let(:parser_data_index) { parser.instance_variable_get('@data_index') }
+
+  def given_image(image)
+    parser_data[:container_images] ||= []
+    parser_data[:container_images] << image
+    parser_data_index.store_path(:container_image, :by_digest, image[:digest], image)
+  end
+
+  def check_data_index_images
+    expect(parser_data[:container_images].size).to eq(parser_data_index[:container_image][:by_digest].size)
+    parser_data[:container_images].each do |image|
+      expect(parser_data_index[:container_image][:by_digest][image[:digest]]).to be(image)
+    end
+  end
+
+  def given_image_registry(registry)
+    parser_data[:container_image_registries] ||= []
+    parser_data[:container_image_registries] << registry
+    parser_data_index.store_path(:container_image_registry, :by_host_and_port,
+                                 "#{image_registry}:#{image_registry_port}", registry)
+  end
 
   describe "get_openshift_images" do
     let(:image_name) { "image_name" }
@@ -69,7 +96,7 @@
 
     it "collects data from openshift images correctly" do
       expect(parser.send(:parse_openshift_image,
-                         image_from_openshift)).to eq(
+                         image_from_openshift, options)).to eq(
                            :name                     => image_name,
                            :registered_on            => Time.parse('2015-08-17T09:16:46Z').utc,
                            :digest                   => image_digest,
@@ -100,7 +127,7 @@
 
     it "handles openshift images without dockerImageManifest and dockerImageMetadata" do
       expect(parser.send(:parse_openshift_image,
-                         image_without_dockerImage_fields).except(:registered_on)).to eq(
+                         image_without_dockerImage_fields, options).except(:registered_on)).to eq(
                            :container_image_registry => nil,
                            :digest                   => nil,
                            :image_ref                => "docker-pullable://sha256:abcdefg",
@@ -111,7 +138,7 @@
 
     it "handles openshift image without dockerConfig" do
       expect(parser.send(:parse_openshift_image,
-                         image_without_dockerConfig)).to eq(
+                         image_without_dockerConfig, options)).to eq(
                            :container_image_registry => nil,
                            :digest                   => nil,
                            :image_ref                => "docker-pullable://sha256:abcdefg",
@@ -129,7 +156,7 @@
     # check https://bugzilla.redhat.com/show_bug.cgi?id=1414508
     it "handles openshift image without environment variables" do
       expect(parser.send(:parse_openshift_image,
-                         image_without_environment_variables)).to eq(
+                         image_without_environment_variables, options)).to eq(
                            :container_image_registry => nil,
                            :digest                   => nil,
                            :image_ref                => "docker-pullable://sha256:abcdefg",
@@ -150,82 +177,99 @@
     end
 
     it "doesn't add duplicated images" do
-      parser.instance_variable_get('@data')[:container_images] = [{
+      given_image(
         :name      => image_name,
         :tag       => image_tag,
         :digest    => image_digest,
         :image_ref => image_ref
-      },]
-      parser.instance_variable_get('@data_index').store_path(
-        :container_image,
-        :by_digest,
-        image_digest,
-        parser.instance_variable_get('@data')[:container_images][0])
+      )
 
       inventory = {"image" => [image_from_openshift,]}
 
-      parser.get_openshift_images(inventory)
-      expect(parser.instance_variable_get('@data')[:container_images].size).to eq(1)
-      expect(parser.instance_variable_get('@data')[:container_images][0]).to eq(
-        parser.instance_variable_get('@data_index')[:container_image][:by_digest].values[0])
-      expect(parser.instance_variable_get('@data')[:container_images][0][:architecture]).to eq('amd64')
+      parser.get_openshift_images(inventory, options)
+      expect(parser_data[:container_images].size).to eq(1)
+      expect(parser_data[:container_images][0][:architecture]).to eq('amd64')
+      check_data_index_images
+    end
+
+    context "store_unused_images=false" do
+      let(:store_unused_images) { false }
+
+      it "adds metadata to existing image" do
+        given_image(
+          :name          => image_name,
+          :tag           => image_tag,
+          :digest        => image_digest,
+          :image_ref     => image_ref,
+          :registered_on => Time.now.utc - 2.minutes
+        )
+
+        inventory = {"image" => [image_from_openshift,]}
+
+        parser.get_openshift_images(inventory, options)
+        expect(parser_data[:container_images].size).to eq(1)
+        expect(parser_data[:container_images][0][:architecture]).to eq('amd64')
+        check_data_index_images
+      end
+
+      it "doesn't add new images" do
+        inventory = {"image" => [image_from_openshift,]}
+
+        parser.get_openshift_images(inventory, options)
+        expect(parser_data[:container_images].blank?).to be true
+      end
     end
 
     it "matches images by digest" do
       FIRST_NAME = "first_name".freeze
       FIRST_TAG = "first_tag".freeze
       FIRST_REF = "first_ref".freeze
-      parser.instance_variable_get('@data')[:container_images] = [{
-        :name      => FIRST_NAME,
-        :tag       => FIRST_TAG,
-        :digest    => image_digest,
-        :image_ref => FIRST_REF
-      },]
-      parser.instance_variable_get('@data_index').store_path(
-        :container_image,
-        :by_digest,
-        image_digest,
-        parser.instance_variable_get('@data')[:container_images][0]
+      given_image(
+        :name          => FIRST_NAME,
+        :tag           => FIRST_TAG,
+        :digest        => image_digest,
+        :image_ref     => FIRST_REF,
+        :registered_on => Time.now.utc - 2.minutes
       )
 
       inventory = {"image" => [image_from_openshift,]}
 
-      parser.get_openshift_images(inventory)
-      expect(parser.instance_variable_get('@data')[:container_images].size).to eq(1)
-      expect(parser.instance_variable_get('@data')[:container_images][0]).to eq(
-        parser.instance_variable_get('@data_index')[:container_image][:by_digest].values[0]
-      )
-      expect(parser.instance_variable_get('@data')[:container_images][0][:architecture]).to eq('amd64')
-      expect(parser.instance_variable_get('@data')[:container_images][0][:name]).to eq(FIRST_NAME)
+      parser.get_openshift_images(inventory, options)
+      expect(parser_data[:container_images].size).to eq(1)
+      expect(parser_data[:container_images][0][:architecture]).to eq('amd64')
+      expect(parser_data[:container_images][0][:name]).to eq(FIRST_NAME)
+      check_data_index_images
     end
 
     context "image registries from openshift images" do
       def parse_single_openshift_image_with_registry
         inventory = {"image" => [image_from_openshift]}
 
-        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)
+        parser.get_openshift_images(inventory, options)
+        expect(parser_data_index[:container_image_registry][:by_host_and_port].size).to eq(1)
+        expect(parser_data[:container_image_registries].size).to eq(1)
       end
 
       it "collects image registries from openshift images that are not also running pods images" do
         parse_single_openshift_image_with_registry
       end
 
       it "avoids duplicate image registries from both running pods and openshift images" do
-        parser.instance_variable_get('@data')[:container_image_registries] = [{
+        given_image_registry(
           :name => image_registry,
           :host => image_registry,
           :port => image_registry_port,
-        },]
-        parser.instance_variable_get('@data_index').store_path(
-          :container_image_registry,
-          :by_host_and_port,
-          "#{image_registry}:#{image_registry_port}",
-          parser.instance_variable_get('@data')[:container_image_registries][0]
         )
         parse_single_openshift_image_with_registry
       end
+
+      context "store_unused_images=false" do
+        let(:store_unused_images) { false }
+
+        it "still adds the registries" do
+          parse_single_openshift_image_with_registry
+        end
+      end
     end
   end
 
@@ -336,20 +380,20 @@ def parse_single_openshift_image_with_registry
       expect(parser.send(:parse_build_pod,
                          RecursiveOpenStruct.new(build_pod)
                          )).to eq(:name                          => 'ruby-sample-build-1',
-                                  :ems_ref                       => 'af3d1a10-44c0-11e5-b186-0aaeec44370e',
-                                  :namespace                     => 'test-namespace',
-                                  :ems_created_on                => '2015-08-17T09:16:46Z',
-                                  :resource_version              => '165339',
-                                  :message                       => 'we come in peace',
-                                  :phase                         => 'set to stun',
-                                  :reason                        => 'this is a reason',
-                                  :duration                      => '33',
-                                  :completion_timestamp          => '50',
-                                  :start_timestamp               => '17',
-                                  :labels                        => [],
+        :ems_ref                       => 'af3d1a10-44c0-11e5-b186-0aaeec44370e',
+        :namespace                     => 'test-namespace',
+        :ems_created_on                => '2015-08-17T09:16:46Z',
+        :resource_version              => '165339',
+        :message                       => 'we come in peace',
+        :phase                         => 'set to stun',
+        :reason                        => 'this is a reason',
+        :duration                      => '33',
+        :completion_timestamp          => '50',
+        :start_timestamp               => '17',
+        :labels                        => [],
                                   :build_config                  => nil,
-                                  :output_docker_image_reference => 'host:port/path/to/image'
-                                 )
+        :output_docker_image_reference => 'host:port/path/to/image'
+      )
     end
 
     context "build config and pods linking" do
@@ -365,14 +409,14 @@ def parse_entities(namespace_pod, namespace_config)
 
       it "links correct build pods to build configurations in same namespace" do
         parse_entities('namespace_1', 'namespace_1')
-        expect(parser.instance_variable_get('@data')[:container_build_pods].first[:build_config]).to eq(
-          parser.instance_variable_get('@data')[:container_builds].first
+        expect(parser_data[:container_build_pods].first[:build_config]).to eq(
+          parser_data[:container_builds].first
         )
       end
 
       it "doesn't link build pods to build configurations in other namespace" do
         parse_entities('namespace_1', 'namespace_2')
-        expect(parser.instance_variable_get('@data')[:container_build_pods].first[:build_config]).to eq(nil)
+        expect(parser_data[:container_build_pods].first[:build_config]).to eq(nil)
       end
     end
   end
@@ -446,10 +490,10 @@ def parse_entities(namespace_pod, namespace_config)
       expect(parser.send(:parse_project,
                          RecursiveOpenStruct.new(
                            :metadata   => {
-                             :annotations => {
-                               'openshift.io/display-name' => 'example'
-                             },
-                           },
+              :annotations => {
+                'openshift.io/display-name' => 'example'
+              },
+            },
                          ))).to eq(nil)
     end
   end