From 4d04b991c9de9e260d6b5d777dfbfc538ea93d5e Mon Sep 17 00:00:00 2001 From: Giulia Naponiello Date: Feb 03 2020 14:15:05 +0000 Subject: Use 'parent_brew_build' to find parent images In Lightblue there are currently 2 variants of same image. One is published and the other is not. Freshmaker uses both to find parent image tree. There is already new field 'parent_brew_build' which could be used instead. Old behavior kept for the moment for backwards compatibility. JIRA: FACTORY-4953 Signed-off-by: Giulia Naponiello --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 73c6f38..b51a91b 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -182,6 +182,7 @@ class ContainerImage(dict): "arches": None, "odcs_compose_ids": None, "published": None, + "parent_image_builds": None, } @region.cache_on_arguments() @@ -214,12 +215,11 @@ class ContainerImage(dict): "in the Koji build %r" % build) # Get the list of ODCS composes used to build the image. - if ("extra" in build and - "image" in build["extra"] and - "odcs" in build["extra"]["image"] and - "compose_ids" in build["extra"]["image"]["odcs"]): - data["odcs_compose_ids"] = \ - build["extra"]["image"]["odcs"]["compose_ids"] + extra_image = build.get("extra", {}).get("image", {}) + if extra_image.get("odcs", {}).get("compose_ids"): + data["odcs_compose_ids"] = extra_image["odcs"]["compose_ids"] + + data["parent_image_builds"] = extra_image.get("parent_image_builds") brew_task = session.get_task_request( build['task_id']) @@ -710,6 +710,7 @@ class LightBlue(object): {"field": "repositories.*.repository", "include": True, "recursive": True}, {"field": "repositories.*.tags.*.name", "include": True, "recursive": True}, {"field": "content_sets", "include": True, "recursive": True}, + {"field": "parent_brew_build", "include": True, "recursive": False}, ] if include_rpms: if srpm_names: @@ -945,7 +946,7 @@ class LightBlue(object): return images def get_images_by_nvrs(self, nvrs, published=True, content_sets=None, - srpm_nvrs=None, include_rpms=True): + srpm_nvrs=None, include_rpms=True, srpm_names=None): """Query lightblue and returns containerImages defined by list of `nvrs`. @@ -956,6 +957,7 @@ class LightBlue(object): :param list srpm_nvrs: list of SRPM NVRs to look for :param bool include_rpms: When True, the rpm_manifest is included in the returned ContainerImages. + :param list srpm_names: list of SRPM names to look for. :return: List of containerImages. :rtype: list of ContainerImages. """ @@ -1018,6 +1020,17 @@ class LightBlue(object): "rvalue": published }) + if srpm_names: + image_request["query"]["$and"].append( + { + "$or": [{ + "field": "rpm_manifest.*.rpms.*.srpm_name", + "op": "=", + "rvalue": srpm_name + } for srpm_name in srpm_names] + } + ) + images = self.find_container_images(image_request) if srpm_nvrs is not None: images = self.filter_out_images_with_higher_srpm_nvr(images, srpm_name_to_nvrs) @@ -1064,6 +1077,8 @@ class LightBlue(object): return None return images[0] + # TODO: this should be removed in the future. There's a field in lightblue and koji + # that allows us to get the parent directly, without checking the layers. @region.cache_on_arguments() def get_image_by_layer(self, top_layer, build_layers_count, srpm_name): @@ -1202,82 +1217,68 @@ class LightBlue(object): return latest_parent - def find_parent_images_with_package(self, child_image, srpm_name, layers): + def find_parent_images_with_package(self, child_image, srpm_name, images=None): """ - Returns the chain of all parent images of the image with - parsed_data.layers `layers` which contain the package `srpm_name` - in their RPM manifest. + Returns the chain of all parent images of the image which contain the + package `srpm_name` in their RPM manifest. - The first item in the list is direct parent of the image in question. + The first item in the list is the direct parent of the image in question. The last item in the list is the top level parent of the image in question. - Docker images are layered and those layers are identified by its - checksum in the ContainerImage["parsed_data"]["layers"] list. - The first layer defined there is the layer defining the image - itself, the second layer is the layer defining its parent, and so on. - - To find the parent image P of image X, we therefore have to search for - an image which has P.parsed_data.layers[0] equal to - X.parsed_data.layers[1]. However, query like this is not possible, so - we search for any image containing the layer X.parsed_data.layers[1], - but further limit the query to return only image which have the count - of the layers equal to `build_layers_count`. For example, layers of an - image - - [ - "sha256:3341bdf...b8e36168", <- layer of this image - "sha256:5fc16d0...0e4e587e", <- probably the first parent image A - "sha256:5d181d2...e6ad6992", - "sha256:274f5cd...ff8fd6e7", <- parent image of parent image A - "sha256:3ca89ba...b0ecae0e", - "sha256:77ed333...a44a147a", - "sha256:e2ec004...4c1fc873" - ] - - Parent images will be retrieved though these layers from top to bottom. + This method is recursive. """ - images = [] - - for idx, parent_top_layer in enumerate(layers[1:]): - # `len(layers) - 1 - idx`. We decrement 1, because we skip the - # first layer in for loop. - parent_build_layers_count = len(layers) - 1 - idx - image = self.get_image_by_layer(parent_top_layer, - parent_build_layers_count, - srpm_name) - children = images if images else [child_image] - if image: - image.resolve(self, children) + if not images: + images = [] + parent_image = None + + children = images if images else [child_image] + # We first try to find the parent from the `parent_brew_build` field in Lightblue. + parent_brew_build = child_image.get("parent_brew_build") + # We need to resolve the image in here because "parent_image_builds" needs to be there + # and it gets populated when the image gets resolved. + child_image.resolve(self, children) + # If the parent is not in `parent_brew_build` we can try to look for the parent in Brew, + # using the field `parent_image_builds` (searching for the nvr), which should always be there. + # In case parent_brew_build is None and child_image["parent_image_builds"] == {}, + # it means we found a base image, so we'll just continue and return the children. + if not parent_brew_build and child_image["parent_image_builds"]: + parent_brew_build = [ + i["nvr"] for i in child_image["parent_image_builds"].values() + if i["id"] == child_image["parent_build_id"]][0] + # We've reached the base image, stop recursion + if not parent_brew_build: + return children + parent_image = self.get_images_by_nvrs([parent_brew_build], srpm_names=[srpm_name]) + + if parent_image: + parent_image = parent_image[0] + parent_image.resolve(self, children) - if images: - if image: - images[-1]['parent'] = image + if images: + if parent_image: + images[-1]['parent'] = parent_image + else: + # If we did not find the parent image with the package, + # we still want to set the parent of the last image with + # the package so we know against which image it has been + # built. + # Let's try first with the "parent_brew_build" field. + parent = self.get_images_by_nvrs([parent_brew_build]) + if parent: + parent = parent[0] + parent.resolve(self, images) else: - # If we did not find the parent image with the package, - # We still want to set the parent of the last image with - # the package so we know against which image it has been - # built. - parent = self.find_latest_parent_image( - parent_top_layer, parent_build_layers_count) - - children_image_layers_count = parent_build_layers_count + 1 - if parent is None and children_image_layers_count != 2: - err = "Cannot find parent of image %s with layer %s " \ - "and layer count %d in Lightblue, Lightblue data " \ - "is probably incomplete" % ( - children[-1]['brew']['build'], parent_top_layer, - parent_build_layers_count) - log.error(err) - if not images[-1]['error']: - images[-1]['error'] = err + err = "Couldn't find parent image. Lightblue data is probably incomplete" + log.error(err) + if not images[-1]['error']: + images[-1]['error'] = err + images[-1]['parent'] = parent - if parent: - parent.resolve(self, images) - images[-1]['parent'] = parent - if not image: - return images - images.append(image) + if not parent_image: + return images + images.append(parent_image) + return self.find_parent_images_with_package(parent_image, srpm_name, images) def find_images_with_packages_from_content_set( self, srpm_nvrs, content_sets, filter_fnc=None, published=True, diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 502c798..c9eed04 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -636,6 +636,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): }, "content_sets": ["dummy-content-set-1", "dummy-content-set-2"], + 'parent_brew_build': 'some-original-nvr-7.6-252.1561619826', 'repositories': [ {'repository': 'product1/repo1', 'published': True, 'tags': [{"name": "latest"}]} @@ -776,6 +777,44 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): }, ] + self.fake_images_with_parent_brew_build = [ + { + 'brew': { + 'completion_date': '20170421T04:27:51.000-0400', + 'build': 'package-name-1-4-12.10', + 'package': 'package-name-1' + }, + 'content_sets': ['dummy-content-set-1', + 'dummy-content-set-2'], + 'parent_brew_build': 'some-original-nvr-7.6-252.1561619826', + 'repositories': [ + {'repository': 'product1/repo1', 'published': True, + 'tags': [{'name': 'latest'}]} + ], + 'parsed_data': { + 'files': [ + { + 'key': 'buildfile', + 'content_url': 'http://git.repo.com/cgit/rpms/repo-1/plain/Dockerfile?id=commit_hash1', + 'filename': 'Dockerfile' + } + ], + }, + 'rpm_manifest': [{ + 'rpms': [ + { + 'srpm_name': 'openssl', + 'srpm_nevra': 'openssl-0:1.2.3-1.src' + }, + { + 'srpm_name': 'tespackage', + 'srpm_nevra': 'testpackage-10:1.2.3-1.src' + } + ] + }] + } + ] + self.fake_container_images = [ ContainerImage.create(data) for data in self.fake_images_with_parsed_data] @@ -788,6 +827,10 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ContainerImage.create(data) for data in self.fake_images_with_modules] + self.fake_container_images_with_parent_brew_build = [ + ContainerImage.create(data) + for data in self.fake_images_with_parent_brew_build] + self.fake_koji_builds = [{"task_id": 654321}, {"task_id": 123456}] self.fake_koji_task_requests = [ ["git://pkgs.devel.redhat.com/rpms/repo-2#commit_hash2", @@ -1218,6 +1261,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): "error": None, "arches": None, "odcs_compose_ids": None, + "parent_image_builds": None, "published": True, "brew": { "completion_date": u"20170421T04:27:51.000-0400", @@ -1317,58 +1361,13 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) ret = lb.find_parent_images_with_package( - self.fake_container_images[0], "openssl", - ["layer0", "layer1", "layer2", "layer3"]) + self.fake_container_images[0], "openssl") self.assertEqual(1, len(ret)) self.assertEqual(ret[0]["brew"]["package"], "package-name-1") self.assertEqual(set(ret[0]["content_sets"]), set(["dummy-content-set-1", "dummy-content-set-2"])) - @patch('freshmaker.lightblue.LightBlue.find_container_images') - @patch('os.path.exists') - def test_parent_images_no_rpm_manifest(self, exists, cont_images): - exists.return_value = True - images_without_rpm_manifest = [] - for data in self.fake_images_with_parsed_data: - img = ContainerImage.create(data) - del img["rpm_manifest"] - images_without_rpm_manifest.append(img) - - cont_images.side_effect = [images_without_rpm_manifest, [], - images_without_rpm_manifest] - - lb = LightBlue(server_url=self.fake_server_url, - cert=self.fake_cert_file, - private_key=self.fake_private_key) - ret = lb.find_parent_images_with_package( - self.fake_container_images[0], "openssl", - ["layer0", "layer1", "layer2", "layer3"]) - - self.assertEqual(0, len(ret)) - - @patch('freshmaker.lightblue.LightBlue.find_container_images') - @patch('os.path.exists') - def test_parent_images_empty_rpm_manifest(self, exists, cont_images): - exists.return_value = True - images_without_rpm_manifest = [] - for data in self.fake_images_with_parsed_data: - img = ContainerImage.create(data) - img["rpm_manifest"] = [] - images_without_rpm_manifest.append(img) - - cont_images.side_effect = [images_without_rpm_manifest, [], - images_without_rpm_manifest] - - lb = LightBlue(server_url=self.fake_server_url, - cert=self.fake_cert_file, - private_key=self.fake_private_key) - ret = lb.find_parent_images_with_package( - self.fake_container_images[0], "openssl", - ["layer0", "layer1", "layer2", "layer3"]) - - self.assertEqual(0, len(ret)) - @patch('freshmaker.lightblue.ContainerImage.resolve_published') @patch('freshmaker.lightblue.LightBlue.find_container_images') @patch('os.path.exists') @@ -1399,8 +1398,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) ret = lb.find_parent_images_with_package( - self.fake_container_images[0], "openssl", - ["layer0", "layer1", "layer2", "layer3", "layer4"]) + self.fake_container_images[0], "openssl", []) self.assertEqual(3, len(ret)) self.assertEqual(ret[0]["brew"]["package"], "package-name-1") @@ -1596,6 +1594,67 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): self.assertTrue(leaf_image6_as_parent["directly_affected"]) break + @patch("freshmaker.lightblue.ContainerImage.resolve_published") + @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs") + @patch("os.path.exists") + @patch("freshmaker.kojiservice.KojiService.get_build") + @patch("freshmaker.kojiservice.KojiService.get_task_request") + def test_parent_images_with_package_using_field_parent_brew_build( + self, get_task_request, get_build, exists, cont_images, + resolve_published): + get_build.return_value = {"task_id": 123456} + get_task_request.return_value = [ + "git://example.com/rpms/repo-1#commit_hash1", "target1", {}] + exists.return_value = True + + cont_images.side_effect = [self.fake_container_images_with_parent_brew_build, [], []] + + lb = LightBlue(server_url=self.fake_server_url, + cert=self.fake_cert_file, + private_key=self.fake_private_key) + ret = lb.find_parent_images_with_package( + self.fake_container_images_with_parent_brew_build[0], "openssl", []) + + self.assertEqual(1, len(ret)) + self.assertEqual(ret[0]["brew"]["package"], "package-name-1") + self.assertEqual(set(ret[0]["content_sets"]), + set(["dummy-content-set-1", "dummy-content-set-2"])) + self.assertEqual(ret[-1]['error'], "Couldn't find parent image. Lightblue data is probably incomplete") + + @patch("freshmaker.lightblue.ContainerImage.resolve_published") + @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs") + @patch("os.path.exists") + @patch("freshmaker.kojiservice.KojiService.get_build") + @patch("freshmaker.kojiservice.KojiService.get_task_request") + def test_parent_images_with_package_using_field_parent_image_builds( + self, get_task_request, get_build, exists, cont_images, + resolve_published): + get_build.return_value = { + "task_id": 123456, + "parent_build_id": 1074147, + "parent_image_builds": { + "rh-osbs/openshift-golang-builder:1.11": { + "id": 969696, "nvr": "openshift-golang-builder-container-v1.11.13-3.1"}, + "rh-osbs/openshift-ose-base:v4.1.34.20200131.033116": { + "id": 1074147, "nvr": "openshift-enterprise-base-container-v4.1.34-202001310309"}}} + get_task_request.return_value = [ + "git://example.com/rpms/repo-1#commit_hash1", "target1", {}] + exists.return_value = True + + self.fake_container_images[0].pop('parent_brew_build') + cont_images.side_effect = [self.fake_container_images, [], []] + + lb = LightBlue(server_url=self.fake_server_url, + cert=self.fake_cert_file, + private_key=self.fake_private_key) + ret = lb.find_parent_images_with_package( + self.fake_container_images[0], "openssl", []) + + self.assertEqual(1, len(ret)) + self.assertEqual(ret[0]["brew"]["package"], "package-name-1") + self.assertEqual(set(ret[0]["content_sets"]), + set(["dummy-content-set-1", "dummy-content-set-2"])) + @patch('freshmaker.lightblue.LightBlue.find_images_with_packages_from_content_set') @patch('freshmaker.lightblue.LightBlue.find_unpublished_image_for_build') @patch('os.path.exists') @@ -1693,6 +1752,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): {'field': 'repositories.*.repository', 'include': True, 'recursive': True}, {'field': 'repositories.*.tags.*.name', 'include': True, 'recursive': True}, {'field': 'content_sets', 'include': True, 'recursive': True}, + {'field': 'parent_brew_build', 'include': True, 'recursive': False}, {'field': 'rpm_manifest.*.rpms', 'include': True, 'recursive': True}, {'field': 'rpm_manifest.*.rpms.*.srpm_name', 'include': True, 'recursive': True}], 'objectType': 'containerImage'})