handling docker-pullable image ids #12711

enoodle · 2016-11-17T13:35:32Z

Recently Kubernetes / Docker started having a different prefix to the docker imageID: docker-pullable. This PR handles the changes needed to handle this change to save the image digest, name and docker_id and use them in a similar way to what was intended before.

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

enoodle · 2016-11-17T13:38:14Z

cc @bazulay @simon3z

simon3z · 2016-11-17T13:40:39Z

@zeari can you review? (Probably you already reviewed part/all of it in the other PR but I want to make sure)

simon3z · 2016-11-17T13:40:50Z

cc @moolitayer (review if you have time)

simon3z · 2016-11-17T13:41:13Z

@miq-bot add_label providers/containers, bug

moolitayer · 2016-11-17T15:04:46Z

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

          (?:(?:
            (?<host>([^\.:/]+\.)+[^\.:/]+)|
            (?:(?<host2>[^:/]+)(?::(?<port>\d+)))|
            (?<localhost>localhost)
          )/)?
          (?<name>(?:[^:/@]+/)*[^/:@]+)
-          (?:(?::(?<tag>.+))|(?:\@(?<digest>.+)))?
+          (?::(?<tag>[^:/@]+))?
+          (?:\@(?<digest>.+))?


Can we have both a tag and a digest? (asking about the change from | to both parts being optional)

Yes, Now we can have them both, but before is was one or the other (The tag would have included the digest..)

moolitayer · 2016-11-17T15:14:58Z

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

-          :digest        => parts[:digest],
+          :name          => image_parts[:name],
+          :tag           => image_parts[:tag],
+          :digest        => image_parts[:digest] || image_ref_parts[:digest],


can you give an example of an image ref with a digest?

docker-pullable://registry.access.redhat.com/openshift3/metrics-deployer@sha256:45f2f81608edcdf53f8f94b4893fc33c824a5ca79d467191e5db1a267f46bb3a

It seems that the new docker added the registry and name to the imageID field too after the docker-pullable:// prefix.

simon3z · 2016-11-21T11:00:48Z

@enoodle can you try to find reviews and LGTM for this? Thanks.

moolitayer · 2016-11-21T11:51:47Z

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

+                       :image      => {:name => "name1", :tag => "tagos", :digest => "sha256:123abcdef",
+                                       :image_ref => example_ref},
+                       :registry   => {:name => "reg.example.com", :host => "reg.example.com", :port => "1234"}},
+


Can you add a test for the new definitions?

@moolitayer I added this test below:

enoodle · 2016-11-21T12:41:36Z

@moolitayer I added more tests

moolitayer · 2016-11-21T13:03:05Z

Thanks @enoodle ✨
LGTM 👍

simon3z · 2016-11-21T16:16:06Z

LGTM 👍
@chessbyte ready for merge

@miq-bot assign chessbyte

simon3z · 2016-11-21T16:17:53Z

@enoodle we need this to be backported right?

Is this all the ManageIQ side needed to address the relevant bug?

enoodle · 2016-11-21T16:19:24Z

@simon3z Yes, backport to euwe
No, more is needed to handle the difference in the ID's between docker and the RepoDigests that we now get from kubernetes.

simon3z · 2016-11-21T16:24:06Z

No, more is needed to handle the difference in the ID's between docker and the RepoDigests that we now get from kubernetes.

@miq-bot assign enoodle

@enoodle please add that as well here so that we don't need any other BZ to merge the extra PRs.

simon3z · 2016-11-23T15:16:08Z

app/models/manageiq/providers/kubernetes/container_manager/scanning/job.rb

+    end
+
+    "cannot analyze image %s with id %s: detected id was %s" % [
+      options[:image_full_name], options[:docker_image_id][0..11], actual[0..11]]


@enoodle don't you have to update this being more specific now? (Maybe the check that failed was about the RepoDigests) No need for anything fancy... I just feel that this message now is misleading.

@simon3z We wouldn't know which one "should" have not failed as all the checks have already failed. I could also add the detected RepoDigests
Some thing like this:

msg = "cannot analyze image %s with id %s: detected ids were %s" % [ options[:image_full_name], options[:docker_image_id][0..11], actual[0..11]] metadata.RepoDigests.each do |digest| msg << digest.split('@').last[0..11] + ", " end return msg

I am splitting on '@' because RepoDigests include the image-repository and name but this will be less interesting in this message in my opinion.

@simon3z I made this change, PTAL at the code.

miq-bot · 2016-11-24T11:21:23Z

<github_pr_commenter_batch />Some comments on commits enoodle/manageiq@eebf813~...0ab53d9

spec/models/manageiq/providers/kubernetes/container_manager/scanning/job_spec.rb

⚠️ - 123 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 243 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 244 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2016-11-24T11:22:21Z

Checked commits enoodle/manageiq@eebf813~...0ab53d9 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 3 offenses detected

app/models/manageiq/providers/kubernetes/container_manager/scanning/job.rb

❕ - Line 116, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for verify_scanned_image_id is too high. [23.39/20]

spec/models/manageiq/providers/kubernetes/container_manager/scanning/job_spec.rb

❕ - Line 123, Col 121 - Metrics/LineLength - Line is too long. [134/120]
❕ - Line 243, Col 121 - Metrics/LineLength - Line is too long. [134/120]

enoodle · 2016-12-01T11:31:06Z

@moolitayer @zeari PTAL

zeari · 2016-12-01T11:41:22Z

LGTM

enoodle · 2016-12-01T13:00:49Z

ping @simon3z @chessbyte

simon3z · 2016-12-01T14:44:58Z

Ready for merge 👍
cc @chessbyte

@miq-bot assign chessbyte

simon3z · 2016-12-01T14:46:15Z

cc @roliveri

chessbyte · 2016-12-02T16:45:32Z

Euwe BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1348610 and https://bugzilla.redhat.com/show_bug.cgi?id=1400615

…es_ids handling docker-pullable image ids (cherry picked from commit 4a52be9) https://bugzilla.redhat.com/show_bug.cgi?id=1348610 https://bugzilla.redhat.com/show_bug.cgi?id=1400615

chessbyte · 2016-12-02T16:46:04Z

Euwe Backport details:

$ git log -1
commit 8c861a6fd622c29fbeca3cfd5bdfddd4e8fb9abd
Author: Richard Oliveri <oliveri.richard.github@gmail.com>
Date:   Thu Dec 1 12:44:58 2016 -0500

    Merge pull request #12711 from enoodle/docker_pullable_container_images_ids
    
    handling docker-pullable image ids
    (cherry picked from commit 4a52be99ceb25b66099b6dfce9200963ec978d30)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1348610
    https://bugzilla.redhat.com/show_bug.cgi?id=1400615

chessbyte · 2016-12-13T23:23:41Z

Backported to Darga via #13142

enoodle mentioned this pull request Nov 17, 2016

Container Images: Collect container images from Openshift #10692

Closed

miq-bot added bug providers/containers labels Nov 17, 2016

moolitayer reviewed Nov 17, 2016

View reviewed changes

chessbyte assigned simon3z Nov 17, 2016

moolitayer reviewed Nov 17, 2016

View reviewed changes

enoodle force-pushed the docker_pullable_container_images_ids branch 4 times, most recently from 9e49471 to d9a1611 Compare November 20, 2016 12:55

enoodle closed this Nov 20, 2016

enoodle reopened this Nov 20, 2016

enoodle force-pushed the docker_pullable_container_images_ids branch from d9a1611 to ef2ca66 Compare November 20, 2016 17:54

moolitayer reviewed Nov 21, 2016

View reviewed changes

enoodle force-pushed the docker_pullable_container_images_ids branch from ef2ca66 to d8cd039 Compare November 21, 2016 12:31

miq-bot assigned chessbyte and unassigned simon3z Nov 21, 2016

miq-bot assigned enoodle and unassigned chessbyte Nov 21, 2016

simon3z reviewed Nov 23, 2016

View reviewed changes

enoodle force-pushed the docker_pullable_container_images_ids branch 2 times, most recently from 958471d to 6906640 Compare November 24, 2016 11:05

Erez Freiberger added 2 commits November 24, 2016 13:20

bump image-inspector version to 2.1

d0cc4ad

Container SSA: comparing RepoDigest of images

0ab53d9

enoodle force-pushed the docker_pullable_container_images_ids branch from 6906640 to 0ab53d9 Compare November 24, 2016 11:20

enoodle mentioned this pull request Nov 28, 2016

Fix chargeback for container Images with rate assigning by docker label #12851

Merged

miq-bot assigned chessbyte and unassigned enoodle Dec 1, 2016

chessbyte assigned roliveri and unassigned chessbyte Dec 1, 2016

roliveri merged commit 4a52be9 into ManageIQ:master Dec 1, 2016

chessbyte added this to the Sprint 50 Ending Dec 5, 2016 milestone Dec 2, 2016

chessbyte added euwe/backported and removed euwe/yes labels Dec 2, 2016

simaishi added the darga/yes label Dec 9, 2016

enoodle mentioned this pull request Dec 13, 2016

[DARGA] Handle Docker pullable container images ids #13142

Merged

chessbyte added darga/backported and removed darga/yes labels Dec 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handling docker-pullable image ids #12711

handling docker-pullable image ids #12711

enoodle commented Nov 17, 2016 •

edited

enoodle commented Nov 17, 2016

simon3z commented Nov 17, 2016

simon3z commented Nov 17, 2016

simon3z commented Nov 17, 2016

moolitayer Nov 17, 2016

enoodle Nov 17, 2016

moolitayer Nov 17, 2016

enoodle Nov 18, 2016

simon3z commented Nov 21, 2016

moolitayer Nov 21, 2016

enoodle Dec 1, 2016 •

edited

enoodle commented Nov 21, 2016

moolitayer commented Nov 21, 2016

simon3z commented Nov 21, 2016

simon3z commented Nov 21, 2016

enoodle commented Nov 21, 2016

simon3z commented Nov 21, 2016

simon3z Nov 23, 2016

enoodle Nov 23, 2016 •

edited

enoodle Nov 23, 2016

miq-bot commented Nov 24, 2016

miq-bot commented Nov 24, 2016

enoodle commented Dec 1, 2016

zeari commented Dec 1, 2016

enoodle commented Dec 1, 2016

simon3z commented Dec 1, 2016

simon3z commented Dec 1, 2016

chessbyte commented Dec 2, 2016

chessbyte commented Dec 2, 2016

chessbyte commented Dec 13, 2016

handling docker-pullable image ids #12711

handling docker-pullable image ids #12711

Conversation

enoodle commented Nov 17, 2016 • edited

enoodle commented Nov 17, 2016

simon3z commented Nov 17, 2016

simon3z commented Nov 17, 2016

simon3z commented Nov 17, 2016

moolitayer Nov 17, 2016

Choose a reason for hiding this comment

enoodle Nov 17, 2016

Choose a reason for hiding this comment

moolitayer Nov 17, 2016

Choose a reason for hiding this comment

enoodle Nov 18, 2016

Choose a reason for hiding this comment

simon3z commented Nov 21, 2016

moolitayer Nov 21, 2016

Choose a reason for hiding this comment

enoodle Dec 1, 2016 • edited

Choose a reason for hiding this comment

enoodle commented Nov 21, 2016

moolitayer commented Nov 21, 2016

simon3z commented Nov 21, 2016

simon3z commented Nov 21, 2016

enoodle commented Nov 21, 2016

simon3z commented Nov 21, 2016

simon3z Nov 23, 2016

Choose a reason for hiding this comment

enoodle Nov 23, 2016 • edited

Choose a reason for hiding this comment

enoodle Nov 23, 2016

Choose a reason for hiding this comment

miq-bot commented Nov 24, 2016

miq-bot commented Nov 24, 2016

enoodle commented Dec 1, 2016

zeari commented Dec 1, 2016

enoodle commented Dec 1, 2016

simon3z commented Dec 1, 2016

simon3z commented Dec 1, 2016

chessbyte commented Dec 2, 2016

chessbyte commented Dec 2, 2016

chessbyte commented Dec 13, 2016

enoodle commented Nov 17, 2016 •

edited

enoodle Dec 1, 2016 •

edited

enoodle Nov 23, 2016 •

edited