Identifying container images by digest only #14185

enoodle · 2017-03-05T14:06:45Z

The image-ref is not a reliable source to identify images when it comes
from the docker daemon (in the docker://... form). This will identify
the images from the information that we can parse.

Doing so will also enable us to commit the images to the @DaTa hash when
we identify the images instead from a collection function (get_images),
which will simplify image collection from Openshift.

This will only identify images by the digest if they have the "docker-pullable://" prefix. Otherwise they will identified by their reference ("docker://..."). The full image name (that is also sent to image-inspection) is also determined from the docker-pullable value if it is present.

The only tricky point I identified is when the docker daemon sends "docker://" prefix but the images came from openshift (from the build process for example) and its name has the "docker-pullable://" prefix. In this case I reconstructed the reference from the available information about the image. [1]

[1]https://github.com/ManageIQ/manageiq/pull/14185/files#diff-0324981fdb3019ce6d98f9c86d97f2bbR769

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

enoodle · 2017-03-05T14:07:04Z

@cben, @zeari, @zakiva Please review.

simon3z · 2017-03-06T11:34:42Z

@enoodle what labels are relevant for this PR?

enoodle · 2017-03-06T12:31:01Z

@miq-bot add_label bug providers/containers

miq-bot · 2017-03-06T12:31:17Z

@enoodle Cannot apply the following label because they are not recognized: bug container

simon3z · 2017-03-06T17:02:36Z

@cben @moolitayer @zakiva please review

miq-bot · 2017-03-06T17:03:20Z

@enoodle Cannot apply the following label because they are not recognized: bug providers/containers

moolitayer · 2017-03-06T19:25:59Z

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

-
-        @data[:container_images].each do |ns|
-          @data_index.store_path(:container_images, :by_name, ns[:name], ns)
-          @data_index.store_path(:container_images, :by_image_ref, ns[:image_ref], ns)


these two where not used?

I couldn't find any use of these.

moolitayer · 2017-03-06T19:27:04Z

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

@@ -174,7 +168,7 @@ def parse_env_variables(env_variables)
      def parse_openshift_image(openshift_image)
        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).merge!({:image_ref => ref})


not that important but I think:
.merge(:image_ref => ref)
should do the same

I don't want to create a new object but to change the current one. (That I receive from @data[:container_images][...] )

moolitayer · 2017-03-06T19:29:43Z

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

@@ -772,7 +768,7 @@ def parse_image_name(image, image_ref)
        {
          :name          => image_parts[:name],
          :tag           => image_parts[:tag],
-          :digest        => image_parts[:digest] || (image_ref_parts[:digest] if image_ref_parts),
+          :digest        => image_parts[:digest] || (image_ref_parts[:digest] if image_ref_parts) || "",


Just curious, why did you want to change the empty value from nil to empty string?
(given that #{nil} is an empty string)

I didn't know that. Also at the beginning I thought of saving the images in @data_index[:container_images][:by_repository][...][:by_name][..][:by_digest][..] so it made sense. I left this to allow easier iteration over the images in @data_index[:container_images]` . I will revert this change now as it seems confusing.

enoodle · 2017-03-07T13:14:27Z

@zeari @cben Please review

simon3z · 2017-03-09T10:21:54Z

@enoodle any repercussion on existing images? E.g. after we merge this, in existing deployments out there, what will happen? Many images will result as "deleted"?

enoodle · 2017-03-09T10:25:33Z

@simon3z With systems with docker prior to 1.12, without the docker-pullable:// prefix to the reference where duplicated images were collected, those duplicated image are expected to be deleted.

simon3z · 2017-03-09T10:29:58Z

@simon3z With systems with docker prior to 1.12, without the docker-pullable:// prefix to the reference where duplicated images were collected, those duplicated image are expected to be deleted.

@enoodle OK. Also there are still clusters out there with docker older that 1.12 what will happen to those ones when this code is merged?

simon3z · 2017-03-14T09:58:45Z

@enoodle OK. Also there are still clusters out there with docker older that 1.12 what will happen to those ones when this code is merged?

Ping @enoodle

enoodle · 2017-03-14T10:00:12Z

@simon3z They currently may have duplicated images which will be deleted.

simon3z · 2017-03-14T17:46:07Z

@enoodle did you update this with the things we discussed this afternoon? If so we can go over this together tomorrow.

simon3z · 2017-03-15T10:12:00Z

@enoodle did you update this with the things we discussed this afternoon?

@enoodle ping?

enoodle · 2017-03-15T10:17:37Z

@simon3z Yes, This will only identify images by the digest if they have the "docker-pullable://" prefix. Otherwise they will identified by their reference ("docker://..."). the full image name (that is also sent to image-inspection) is also determined from the docker-pullable value if it is present.

Edit:
The only tricky point I identified is when the docker daemon sends "docker://" prefix but the images came from openshift (from the build process for example) and its name has the "docker-pullable://" prefix. In this case I reconstructed the reference from the available information about the image. [1]

[1]https://github.com/ManageIQ/manageiq/pull/14185/files#diff-0324981fdb3019ce6d98f9c86d97f2bbR769

@DaTa

The image-ref is not a reliable source to identify images when it comes from the docker daemon (in the docker://... form). This will identify the images from the information that we can parse. Doing so will also enable us to commit the images to the @DaTa hash when we identify the images instead from a collection function (get_images), which will simplify image collection from Openshift.

enoodle · 2017-03-15T13:04:46Z

@cben @zeari Please Review

enoodle · 2017-03-15T13:05:52Z

cc @blomquisg We would like to review/merge this ASAP before the pending freeze.

miq-bot · 2017-03-15T13:07:05Z

Checked commit enoodle@f03a2fd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 2 offenses detected

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

❗ - Line 691, Col 63 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

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

❗ - Line 166, Col 92 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

simon3z · 2017-03-15T13:09:56Z

@enoodle @cben please review and test this thoroughly because we can't do mistakes on this one.

simon3z · 2017-03-15T13:11:19Z

@enoodle @cben please review and test this thoroughly because we can't do mistakes on this one.

@enoodle can you also verify that SSA is working with all the images? (docker://, docker-pullable://, docker://...@sha..., etc.)

enoodle · 2017-03-15T13:24:01Z

@simon3z I tested SSA with this PR on both docker-pullable:// an docker:// systems and also for a system where an image has a reconstructed image-ref, it worked well on all cases.

simon3z · 2017-03-15T14:43:16Z

@simon3z I tested SSA with this PR on both docker-pullable:// an docker:// systems and also for a system where an image has a reconstructed image-ref, it worked well on all cases.

@enoodle what about docker://...@sha...?

enoodle · 2017-03-15T14:49:43Z

@simon3z I am not sure what this means. The older docker daemon only returns the reference in one way docker://sha256:<DIGEST> . This is what I tested above.

simon3z · 2017-03-16T15:48:22Z

LGTM 👍
@miq-bot assign blomquisg

simaishi · 2017-03-17T19:56:57Z

@enoodle there is a conflict backporting this to Euwe. The assert_table_counts in spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb is all different between master and Euwe. I'm thinking this shouldn't go in as is:

expect(ContainerImage.count).to eq(40)

Please let me know if it can, or if 40 should be changed to something else.

simon3z · 2017-03-18T12:38:22Z

@enoodle please handle this ASAP on Sunday (Euwe backport conflict).

enoodle · 2017-03-19T09:48:51Z

@simaishi It should be 31 for the tests in the euwe branch. I made a backport PR for your convenience #14396

simaishi · 2017-03-20T19:55:28Z

Backported to Euwe via #14396

enoodle force-pushed the better_identify_container_images branch 2 times, most recently from 22a60e8 to be1c581 Compare March 5, 2017 17:31

chessbyte added bug providers/containers labels Mar 6, 2017

chessbyte assigned simon3z Mar 6, 2017

moolitayer reviewed Mar 6, 2017

View reviewed changes

enoodle force-pushed the better_identify_container_images branch from be1c581 to 1996084 Compare March 7, 2017 08:27

enoodle mentioned this pull request Mar 7, 2017

[WIP] kubernetes refresh: better parse old pod images references #13788

Closed

zeari approved these changes Mar 12, 2017

View reviewed changes

enoodle force-pushed the better_identify_container_images branch from 1996084 to 65d0568 Compare March 14, 2017 16:56

enoodle changed the title ~~Identifying container images by registry, name and digest~~ Identifying container images by digest only Mar 14, 2017

enoodle force-pushed the better_identify_container_images branch from 65d0568 to bdccfb9 Compare March 14, 2017 17:08

enoodle force-pushed the better_identify_container_images branch from bdccfb9 to 7c27144 Compare March 15, 2017 08:46

enoodle force-pushed the better_identify_container_images branch from 7c27144 to f03a2fd Compare March 15, 2017 13:02

zeari approved these changes Mar 16, 2017

View reviewed changes

miq-bot assigned blomquisg and unassigned simon3z Mar 16, 2017

blomquisg added the euwe/yes label Mar 16, 2017

blomquisg merged commit 401bf3b into ManageIQ:master Mar 16, 2017

blomquisg added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 16, 2017

simaishi added the euwe/conflict label Mar 17, 2017

enoodle mentioned this pull request Mar 19, 2017

[EUWE] Identifying container images by registry, name and digest #14396

Merged

simaishi removed the euwe/conflict label Mar 20, 2017

simaishi added euwe/backported and removed euwe/yes labels Mar 20, 2017

enoodle mentioned this pull request Apr 27, 2017

container images are identified by image_ref only #14919

Closed

cben mentioned this pull request Jun 25, 2017

Add unique indexes for Containers tables ManageIQ/manageiq-schema#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identifying container images by digest only #14185

Identifying container images by digest only #14185

enoodle commented Mar 5, 2017 •

edited

Loading

enoodle commented Mar 5, 2017

simon3z commented Mar 6, 2017

enoodle commented Mar 6, 2017 •

edited

Loading

miq-bot commented Mar 6, 2017

simon3z commented Mar 6, 2017

miq-bot commented Mar 6, 2017

moolitayer Mar 6, 2017

enoodle Mar 7, 2017

moolitayer Mar 6, 2017

enoodle Mar 7, 2017

moolitayer Mar 6, 2017

enoodle Mar 7, 2017

enoodle commented Mar 7, 2017

simon3z commented Mar 9, 2017

enoodle commented Mar 9, 2017

simon3z commented Mar 9, 2017

simon3z commented Mar 14, 2017

enoodle commented Mar 14, 2017

simon3z commented Mar 14, 2017

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017 •

edited

Loading

enoodle commented Mar 15, 2017

enoodle commented Mar 15, 2017

miq-bot commented Mar 15, 2017

simon3z commented Mar 15, 2017

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017 •

edited

Loading

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017

simon3z commented Mar 16, 2017

simaishi commented Mar 17, 2017

simon3z commented Mar 18, 2017

enoodle commented Mar 19, 2017

simaishi commented Mar 20, 2017

Identifying container images by digest only #14185

Identifying container images by digest only #14185

Conversation

enoodle commented Mar 5, 2017 • edited Loading

enoodle commented Mar 5, 2017

simon3z commented Mar 6, 2017

enoodle commented Mar 6, 2017 • edited Loading

miq-bot commented Mar 6, 2017

simon3z commented Mar 6, 2017

miq-bot commented Mar 6, 2017

moolitayer Mar 6, 2017

Choose a reason for hiding this comment

enoodle Mar 7, 2017

Choose a reason for hiding this comment

moolitayer Mar 6, 2017

Choose a reason for hiding this comment

enoodle Mar 7, 2017

Choose a reason for hiding this comment

moolitayer Mar 6, 2017

Choose a reason for hiding this comment

enoodle Mar 7, 2017

Choose a reason for hiding this comment

enoodle commented Mar 7, 2017

simon3z commented Mar 9, 2017

enoodle commented Mar 9, 2017

simon3z commented Mar 9, 2017

simon3z commented Mar 14, 2017

enoodle commented Mar 14, 2017

simon3z commented Mar 14, 2017

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017 • edited Loading

enoodle commented Mar 15, 2017

enoodle commented Mar 15, 2017

miq-bot commented Mar 15, 2017

simon3z commented Mar 15, 2017

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017 • edited Loading

simon3z commented Mar 15, 2017

enoodle commented Mar 15, 2017

simon3z commented Mar 16, 2017

simaishi commented Mar 17, 2017

simon3z commented Mar 18, 2017

enoodle commented Mar 19, 2017

simaishi commented Mar 20, 2017

enoodle commented Mar 5, 2017 •

edited

Loading

enoodle commented Mar 6, 2017 •

edited

Loading

enoodle commented Mar 15, 2017 •

edited

Loading

enoodle commented Mar 15, 2017 •

edited

Loading