PR#334: Do not rebuild container images with RPM which is already at least at the same version as the one in advisory. - freshmaker

freshmaker

#334 Do not rebuild container images with RPM which is already at least at the same version as the one in advisory.

Merged 5 years ago by jkaluza. Opened 5 years ago by jkaluza.

jkaluza/freshmaker compare-nvr into master

Do not rebuild container images with RPM which is already at least at the same version as the one in advisory.

Jan Kaluza • 5 years ago

8ab3619

freshmaker/handlers/errata/errata_advisory_rpms_signed.py

file modified

+5 -11

		`@@ -400,25 +400,19 @@`
		`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)`

freshmaker/lightblue.py

file modified

+98 -15

		`@@ -30,6 +30,7 @@`
		`import requests`
		`import six`
		`import dogpile.cache`
		`+ import kobo.rpmlib`
		`from itertools import groupby`

		`from six.moves import http_client`
		`@@ -750,15 +751,80 @@`
		`]`
		`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 @@`
		`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 @@`
		`"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 @@`
		`},`
		`]`
		`},`
		`- "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 @@`
		`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 @@`
		`: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 @@`
		`}`
		`)`

		`- 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 @@`
		`})`

		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		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.`

tests/test_errata_advisory_rpms_signed_handler.py

file modified

+5 -5

		`@@ -440,7 +440,7 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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"])`

tests/test_lightblue.py

file modified

+31 -8

		`@@ -657,6 +657,10 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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': {`

jkaluza commented 5 years ago

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.