From 8ab3619810ac543299fed712cc94fe88656e7d2f Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Dec 11 2018 12:41:18 +0000 Subject: Do not rebuild container images with RPM which is already at least at the same version as the one in advisory. If all the SRPM NVRs in the container image are newer than matching input NVRs, the container image is filtered out from the `images` list. For example: The httpd-2.4-1 RPM is released together with httpd-container. In this case, Freshmaker would try to rebuild httpd-container, because it contains httpd package. But this is not needed, because latest httpd-container already contains that updated package. Therefore we filter it out in this method. --- diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 59f6f1f..2d827ff 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -400,25 +400,19 @@ class ErrataAdvisoryRPMsSignedHandler(ContainerBuildHandler): published = None release_category = None - # For each RPM package in Errata advisory, find the SRPM package name. - srpm_names = set() - nvrs = errata.get_builds(errata_id) - for nvr in nvrs: - srpm_name = koji.parse_NVR(nvr)['name'] - srpm_names.add(srpm_name) - # Limit the Lightblue query to particular leaf images if set in Event. leaf_container_images = None if isinstance(self.event, ManualRebuildWithAdvisoryEvent): leaf_container_images = self.event.container_images - # For each SRPM name, find out all the containers which include - # this SRPM name. + # For each SRPM NVR, find out all the containers which include + # this SRPM NVR. + srpm_nvrs = set(errata.get_builds(errata_id)) self.log_info( "Going to find all the container images to rebuild as " - "result of %r update.", srpm_names) + "result of %r update.", srpm_nvrs) batches = lb.find_images_to_rebuild( - srpm_names, content_sets, + srpm_nvrs, content_sets, filter_fnc=self._filter_out_not_allowed_builds, published=published, release_category=release_category, leaf_container_images=leaf_container_images) diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 0547dc6..262b65b 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -30,6 +30,7 @@ import re import requests import six import dogpile.cache +import kobo.rpmlib from itertools import groupby from six.moves import http_client @@ -750,15 +751,80 @@ class LightBlue(object): ] return projection + def filter_out_images_with_lower_srpm_nvr(self, images, srpm_name_to_nvr): + """ + Checks whether the input NVRs defined in `srpm_name_to_nvr` dict are + newer than the matching SRPM NVRs in the container image. + + If all the SRPM NVRs in the container image are newer than matching + input NVRs, the container image is filtered out from the `images` + list. + + For example: The httpd-2.4-1 RPM is released together with + httpd-container. In this case, Freshmaker would try to rebuild + httpd-container, because it contains httpd package. But this is not + needed, because latest httpd-container already contains that updated + package. Therefore we filter it out in this method. + + :param list images: List of ContainerImage instances. + :param dict srpm_name_to_nvr: Dict with SRPM name as a key and NVR + as a value. + :rtype: list + :return: List of ContainerImage instances without the filtered images. + """ + ret = [] + for image in images: + if "rpm_manifest" not in image or not image["rpm_manifest"]: + # Do not filter if we are not sure what RPMs are in the image. + ret.append(image) + continue + # There is always just single "rpm_manifest". Lightblue returns + # this as a list, because it is reference to + # containerImageRPMManifest. + rpm_manifest = image["rpm_manifest"][0] + if "rpms" not in rpm_manifest: + # Do not filter if we are not sure what RPMs are in the image. + ret.append(image) + continue + # Check whether all the input SRPMs in the container image are + # older or newer and filter the container images in case they are + # not older. + rpms = rpm_manifest["rpms"] + for rpm in rpms: + if "srpm_name" in rpm and rpm["srpm_name"] in srpm_name_to_nvr: + image_srpm_nvr = kobo.rpmlib.parse_nvr(rpm["srpm_nevra"]) + input_srpm_nvr = kobo.rpmlib.parse_nvr( + srpm_name_to_nvr[rpm["srpm_name"]]) + # compare_nvr return values: + # - nvr1 newer than nvr2: 1 + # - same nvrs: 0 + # - nvr1 older: -1 + # We want to rebuild only images with SRPM NVR lower than + # input SRPM NVR, therefore we check for -1. + if kobo.rpmlib.compare_nvr(image_srpm_nvr, input_srpm_nvr, + ignore_epoch=True) == -1: + ret.append(image) + break + else: + # Oh-no, the mighty for/else block! + # The else clause executes after the loop completes normally. + # This means that the loop did not encounter a break statement. + # In our case, this means that we filtered out the image. + image.log_error( + "Will not rebuild %s, because it does not contain " + "older version of any input package: %r" % ( + image["brew"]["build"], srpm_name_to_nvr.values())) + return ret + def find_images_with_included_srpms( - self, content_sets, srpm_names, repositories, published=True): + self, content_sets, srpm_nvrs, repositories, published=True): """Query lightblue and find containerImages in given containerRepositories. By default limit only to images which have been published to at least one repository and images which have latest tag. :param list content_sets: List of content_sets the image includes RPMs from. - :param list srpm_names: list of srpm_name (source rpm name) to look for + :param list srpm_nvrs: list of SRPM NVRs to look for :param list repositories: List of repository names to look for. :param bool published: whether to limit queries to published repositories @@ -767,6 +833,12 @@ class LightBlue(object): for repo in repositories.values(): auto_rebuild_tags |= set(repo["auto_rebuild_tags"]) + # Lightblue cannot compare NVRs, so just ask for all the container + # images with any version/release of SRPM we are interested in and + # compare it on client side. + srpm_name_to_nvr = {koji.parse_NVR(srpm_nvr)["name"]: srpm_nvr + for srpm_nvr in srpm_nvrs} + image_request = { "objectType": "containerImage", "query": { @@ -790,7 +862,7 @@ class LightBlue(object): "field": "rpm_manifest.*.rpms.*.srpm_name", "op": "=", "rvalue": srpm_name - } for srpm_name in srpm_names] + } for srpm_name in srpm_name_to_nvr.keys()] }, { "field": "parsed_data.files.*.key", @@ -799,7 +871,8 @@ class LightBlue(object): }, ] }, - "projection": self._get_default_projection(srpm_names=srpm_names) + "projection": self._get_default_projection( + srpm_names=srpm_name_to_nvr.keys()) } if published is not None: @@ -831,10 +904,11 @@ class LightBlue(object): new_images.append(image) break images = new_images + images = self.filter_out_images_with_lower_srpm_nvr(images, srpm_name_to_nvr) return images def get_images_by_nvrs(self, nvrs, published=True, content_sets=None, - srpm_names=None): + srpm_nvrs=None): """Query lightblue and returns containerImages defined by list of `nvrs`. @@ -842,7 +916,7 @@ class LightBlue(object): :param bool published: whether to limit queries to published images :param list content_sets: List of content_sets the image includes RPMs from. - :param list srpm_names: list of srpm_name (source rpm name) to look for + :param list srpm_nvrs: list of SRPM NVRs to look for :return: List of containerImages. :rtype: list of ContainerImages. """ @@ -878,14 +952,19 @@ class LightBlue(object): } ) - if srpm_names is not None: + if srpm_nvrs is not None: + # Lightblue cannot compare NVRs, so just ask for all the container + # images with any version/release of SRPM we are interested in and + # compare it on client side. + srpm_name_to_nvr = {koji.parse_NVR(srpm_nvr)["name"]: srpm_nvr + for srpm_nvr in srpm_nvrs} image_request["query"]["$and"].append( { "$or": [{ "field": "rpm_manifest.*.rpms.*.srpm_name", "op": "=", "rvalue": srpm_name - } for srpm_name in srpm_names] + } for srpm_name in srpm_name_to_nvr.keys()] } ) @@ -898,6 +977,8 @@ class LightBlue(object): }) images = self.find_container_images(image_request) + if srpm_nvrs is not None: + images = self.filter_out_images_with_lower_srpm_nvr(images, srpm_name_to_nvr) return images def find_unpublished_image_for_build(self, build): @@ -1093,14 +1174,14 @@ class LightBlue(object): images.append(image) def find_images_with_packages_from_content_set( - self, srpm_names, content_sets, filter_fnc=None, + self, srpm_nvrs, content_sets, filter_fnc=None, published=True, deprecated=False, release_category="Generally Available", leaf_container_images=None): """Query lightblue and find containers which contain given package from one of content sets - :param list srpm_names: list of srpm_name (source rpm name) to look for + :param list srpm_nvrs: list of SRPM NVRs to look for :param list content_sets: list of strings (content sets) to consider when looking for the packages :param function filter_fnc: Function called as @@ -1132,12 +1213,12 @@ class LightBlue(object): return [] if not leaf_container_images: images = self.find_images_with_included_srpms( - content_sets, srpm_names, repos, published) + content_sets, srpm_nvrs, repos, published) else: # The `leaf_container_images` can contain unpublished container image, # therefore set `published` to None. images = self.get_images_by_nvrs( - leaf_container_images, None, content_sets, srpm_names) + leaf_container_images, None, content_sets, srpm_nvrs) # There can be multi-arch images which share the same # image['brew']['build']. Freshmaker is not interested in the image @@ -1374,7 +1455,7 @@ class LightBlue(object): return batches def find_images_to_rebuild( - self, srpm_names, content_sets, published=True, deprecated=False, + self, srpm_nvrs, content_sets, published=True, deprecated=False, release_category="Generally Available", filter_fnc=None, leaf_container_images=None): """ @@ -1386,7 +1467,7 @@ class LightBlue(object): image from N+1 must happen *after* all of the images from sub-list N have been rebuilt. - :param list srpm_names: List of srpm_name (source rpm name) to look for + :param list srpm_nvrs: List of SRPM NVRs to look for :param list content_sets: list of strings (content sets) to consider when looking for the packages :param bool published: whether to limit queries to published @@ -1407,9 +1488,11 @@ class LightBlue(object): is not respected when `leaf_container_images` are used. """ images = self.find_images_with_packages_from_content_set( - srpm_names, content_sets, filter_fnc, published, deprecated, + srpm_nvrs, content_sets, filter_fnc, published, deprecated, release_category, leaf_container_images=leaf_container_images) + srpm_names = [koji.parse_NVR(srpm_nvr)["name"] for srpm_nvr in srpm_nvrs] + def _get_images_to_rebuild(image): """ Find out parent images to rebuild, helper called from threadpool. diff --git a/tests/test_errata_advisory_rpms_signed_handler.py b/tests/test_errata_advisory_rpms_signed_handler.py index ae69e68..6d64d36 100644 --- a/tests/test_errata_advisory_rpms_signed_handler.py +++ b/tests/test_errata_advisory_rpms_signed_handler.py @@ -440,7 +440,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - set(['httpd']), ['content-set-1'], + set(['httpd-2.4-11.el7']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available', leaf_container_images=None) @@ -457,7 +457,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - set(['httpd']), ['content-set-1'], + set(['httpd-2.4-11.el7', 'httpd-2.2-11.el6']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available', leaf_container_images=None) @@ -476,7 +476,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - set(['httpd']), ['content-set-1'], + set(['httpd-2.4-11.el7']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=None, release_category=None, leaf_container_images=None) @@ -493,7 +493,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - set(['httpd']), ['content-set-1'], + set(['httpd-2.4-11.el7']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available', leaf_container_images=None) @@ -511,7 +511,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - set(['httpd']), ['content-set-1'], + set(['httpd-2.4-11.el7']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available', leaf_container_images=["foo", "bar"]) diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 89ce9a1..a73303c 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -657,6 +657,10 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ContainerImage.create(data) for data in self.fake_images_with_parsed_data] + self.fake_container_images_floating_tag = [ + ContainerImage.create(data) + for data in self.fake_images_with_parsed_data_floating_tag] + 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", @@ -880,7 +884,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): self.fake_repositories_with_content_sets} cont_images.return_value = self.fake_images_with_parsed_data ret = lb.find_images_with_included_srpms( - ["content-set-1", "content-set-2"], ["openssl"], repositories) + ["content-set-1", "content-set-2"], ["openssl-1.2.3-2"], repositories) expected_image_request = { "objectType": "containerImage", @@ -972,15 +976,34 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): repo["repository"]: repo for repo in self.fake_repositories_with_content_sets} cont_images.return_value = ( - self.fake_images_with_parsed_data + - self.fake_images_with_parsed_data_floating_tag) + self.fake_container_images + + self.fake_container_images_floating_tag) ret = lb.find_images_with_included_srpms( - ["content-set-1", "content-set-2"], ["openssl"], repositories) + ["content-set-1", "content-set-2"], ["openssl-1.2.3-2"], repositories) self.assertEqual( [image["brew"]["build"] for image in ret], ['package-name-2-4-12.10', 'package-name-3-4-12.10']) + @patch('freshmaker.lightblue.LightBlue.find_container_images') + @patch('os.path.exists') + def test_images_with_included_newer_srpm( + self, exists, cont_images): + + exists.return_value = True + lb = LightBlue(server_url=self.fake_server_url, + cert=self.fake_cert_file, + private_key=self.fake_private_key) + repositories = { + repo["repository"]: repo for repo in + self.fake_repositories_with_content_sets} + cont_images.return_value = ( + self.fake_container_images + + self.fake_container_images_floating_tag) + ret = lb.find_images_with_included_srpms( + ["content-set-1", "content-set-2"], ["openssl-1.2.3-1"], repositories) + self.assertEqual(ret, []) + def _filter_fnc(self, image): return image["brew"]["build"].startswith("filtered_") @@ -1012,7 +1035,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) ret = lb.find_images_with_packages_from_content_set( - "openssl", ["dummy-content-set-1"], filter_fnc=self._filter_fnc) + set(["openssl-1.2.3-3"]), ["dummy-content-set-1"], filter_fnc=self._filter_fnc) # Only the first image should be returned, because the first one # is in repository "product1/repo1", but we have asked for images @@ -1321,7 +1344,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): lb = LightBlue(server_url=self.fake_server_url, cert=self.fake_cert_file, private_key=self.fake_private_key) - batches = lb.find_images_to_rebuild(["dummy"], ["dummy"]) + batches = lb.find_images_to_rebuild(["dummy-1-1"], ["dummy"]) # Each of batch is sorted for assertion easily expected_batches = [ @@ -1361,7 +1384,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): lb = LightBlue(server_url=self.fake_server_url, cert=self.fake_cert_file, private_key=self.fake_private_key) - batches = lb.find_images_to_rebuild(["dummy"], ["dummy"]) + batches = lb.find_images_to_rebuild(["dummy-1-1"], ["dummy"]) self.assertEqual(len(batches), 1) self.assertEqual(len(batches[0]), 1) @@ -1414,7 +1437,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) lb.find_images_with_packages_from_content_set( - ["openssl"], ["dummy-content-set"], + ["openssl-1.2.3-2"], ["dummy-content-set"], leaf_container_images=["foo", "bar"]) cont_images.assert_called_once_with( {'query': {