From a86f3c7de9e12d22aa7a38c909cf251bedf678db Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Mar 28 2018 09:23:59 +0000 Subject: Fix issues and performance with generating batches in Lightblue with complex deps between images. --- diff --git a/conf/config.py b/conf/config.py index 7fc1c22..3bf505d 100644 --- a/conf/config.py +++ b/conf/config.py @@ -270,6 +270,7 @@ class TestConfiguration(BaseConfiguration): AUTH_BACKEND = 'noauth' AUTH_LDAP_SERVER = 'ldap://ldap.example.com' AUTH_LDAP_GROUP_BASE = 'ou=groups,dc=example,dc=com' + MAX_THREAD_WORKERS = 1 HANDLER_BUILD_WHITELIST = { 'BrewSignRPMHandler': { diff --git a/freshmaker/config.py b/freshmaker/config.py index 95a188d..511fca5 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -285,6 +285,10 @@ class Config(object): 'type': dict, 'default': {}, 'desc': 'Configuration for each supported messaging backend.'}, + 'max_thread_workers': { + 'type': int, + 'default': 10, + 'desc': 'Maximum number of thread workers used by Freshmaker.'}, } def __init__(self, conf_section_obj): diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 193c35a..a8d5c93 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -103,9 +103,8 @@ class ErrataAdvisoryRPMsSignedHandler(ContainerBuildHandler): # Get and record all images to rebuild based on the current # ErrataAdvisoryRPMsSignedEvent event. - builds = {} - for batches in self._find_images_to_rebuild(db_event.search_key): - builds = self._record_batches(batches, event, builds) + batches = self._find_images_to_rebuild(db_event.search_key) + builds = self._record_batches(batches, event) if not builds: msg = 'No container images to rebuild for advisory %r' % event.errata_name @@ -682,19 +681,23 @@ class ErrataAdvisoryRPMsSignedHandler(ContainerBuildHandler): published = None release_category = None - # For each RPM package in Errata advisory, find Docker images - # containing this package and record those images into database. + # 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: - self.log_info( - "Going to find all the container images to rebuild as " - "result of %s update.", nvr) - srpm_name = self._find_build_srpm_name(nvr) - batches = lb.find_images_to_rebuild( - srpm_name, content_sets, - filter_fnc=self._filter_out_not_allowed_builds, - published=published, release_category=release_category) - yield batches + srpm_name = koji.parse_NVR(nvr)['name'] + srpm_names.add(srpm_name) + + # For each SRPM name, find out all the containers which include + # this SRPM name. + self.log_info( + "Going to find all the container images to rebuild as " + "result of %r update.", srpm_names) + batches = lb.find_images_to_rebuild( + srpm_names, content_sets, + filter_fnc=self._filter_out_not_allowed_builds, + published=published, release_category=release_category) + return batches def _find_build_srpm_name(self, build_nvr): """Find srpm name from a build""" diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index fd2dc83..6b8731c 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -33,6 +33,7 @@ from six.moves import http_client import concurrent.futures from freshmaker import log, conf from freshmaker.kojiservice import koji_service +from freshmaker.utils import sorted_by_nvr import koji @@ -205,15 +206,12 @@ class ContainerImage(dict): return data - def resolve_commit(self, srpm_name): + def resolve_commit(self): """ Uses the ContainerImage data to resolve the information about commit from which the Docker image has been built. Sets the "repository and "commit" keys/values if available. - - :param str srpm_name: Name of the package because of which the Docker - image is rebuilt. """ # Find the additional data for Container build in Koji. nvr = self["brew"]["build"] @@ -571,8 +569,8 @@ class LightBlue(object): return request - def find_images_with_included_srpm(self, repositories, srpm_name, - published=True): + def find_images_with_included_srpms(self, repositories, srpm_names, + published=True): """Query lightblue and find containerImages in given containerRepositories. By default limit only to images which have been @@ -581,7 +579,7 @@ class LightBlue(object): :param bool published: whether to limit queries to published repositories :param dict repositories: dictionary with repository names to look inside - :param str srpm_name: srpm_name (source rpm name) to look for + :param list srpm_names: list of srpm_name (source rpm name) to look for """ image_request = { "objectType": "containerImage", @@ -600,9 +598,11 @@ class LightBlue(object): "rvalue": "latest" }, { - "field": "rpm_manifest.*.rpms.*.srpm_name", - "op": "=", - "rvalue": srpm_name + "$or": [{ + "field": "rpm_manifest.*.rpms.*.srpm_name", + "op": "=", + "rvalue": srpm_name + } for srpm_name in srpm_names] }, { "field": "parsed_data.files.*.key", @@ -799,7 +799,7 @@ class LightBlue(object): if image: image.resolve_content_sets(self, children, published, deprecated, release_category) - image.resolve_commit(srpm_name) + image.resolve_commit() if images: if image: @@ -827,20 +827,20 @@ class LightBlue(object): parent.resolve_content_sets( self, images, published, deprecated, release_category) - parent.resolve_commit(srpm_name) + parent.resolve_commit() images[-1]['parent'] = parent if not image: return images images.append(image) - def find_images_with_package_from_content_set( - self, srpm_name, content_sets, filter_fnc=None, + def find_images_with_packages_from_content_set( + self, srpm_names, content_sets, filter_fnc=None, published=True, deprecated=False, release_category="Generally Available"): """Query lightblue and find containers which contain given package from one of content sets - :param str srpm_name: srpm_name (source rpm name) to look for + :param list srpm_names: list of srpm_name (source rpm name) 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 @@ -867,16 +867,16 @@ class LightBlue(object): content_sets, published, deprecated, release_category) if not repos: return [] - images = self.find_images_with_included_srpm( - repos, srpm_name, published) + images = self.find_images_with_included_srpms( + repos, srpm_names, published) # There can be multi-arch images which share the same # image['brew']['build']. Freshmaker is not interested in the image # architecture, it is only interested in NVR, so group the images # by the same image['brew']['build'] and include just first one in the # image list. - sorted_images = sorted( - images, key=lambda image: image['brew']['build'], reverse=True) + sorted_images = sorted_by_nvr( + images, get_nvr=lambda image: image['brew']['build'], reverse=True) images = [] for k, v in groupby(sorted_images, key=lambda x: x['brew']['build']): images.append(v.next()) @@ -886,8 +886,8 @@ class LightBlue(object): # contain all the versions which ever containing the srpm_name. if not published: # Sort images by brew build NVR descending - sorted_images = sorted( - images, key=lambda image: image['brew']['build'], reverse=True) + sorted_images = sorted_by_nvr( + images, get_nvr=lambda image: image['brew']['build'], reverse=True) # Iterate over all the images and only keep the very first one # with the given name-version - this is the latest one. @@ -909,7 +909,7 @@ class LightBlue(object): # published images should have the content_set set. image.resolve_content_sets(self, None, published, deprecated, release_category) - image.resolve_commit(srpm_name) + image.resolve_commit() return images def _deduplicate_images_to_rebuild(self, to_rebuild): @@ -934,8 +934,8 @@ class LightBlue(object): """ # Temporary dict mapping the NVR of image to coordinates in the # `to_rebuild` list. For example - # nvr_to_coordinates["nvr"] = [0, 3] means that the image with - # nvr "nvr" is 4th image in the to_rebuild[0] list. + # nvr_to_coordinates["nvr"] = [[0, 3], ...] means that the image with + # nvr "nvr" is 4th image in the to_rebuild[0] list, ... nvr_to_coordinates = {} # Temporary dict mapping the NV to list of NVRs. The List of NVRs # is always sorted descending. @@ -951,13 +951,16 @@ class LightBlue(object): nv = "%s-%s" % (parsed_nvr["name"], parsed_nvr["version"]) if nv not in nv_to_nvrs: nv_to_nvrs[nv] = [] - nv_to_nvrs[nv].append(nvr) - nvr_to_coordinates[nvr] = [image_id, parent_id] + if nvr not in nv_to_nvrs[nv]: + nv_to_nvrs[nv].append(nvr) + if nvr not in nvr_to_coordinates: + nvr_to_coordinates[nvr] = [] + nvr_to_coordinates[nvr].append([image_id, parent_id]) nvr_to_image[nvr] = image # Sort the lists in nv_to_nvrs dict. for nv in nv_to_nvrs.keys(): - nv_to_nvrs[nv].sort(reverse=True) + nv_to_nvrs[nv] = sorted_by_nvr(nv_to_nvrs[nv], reverse=True) # Iterate through list of NVs. for nvrs in nv_to_nvrs.values(): @@ -966,21 +969,64 @@ class LightBlue(object): latest_nvr = nvrs[0] # Now replace all others NVR with the highest one. for nvr in nvrs[1:]: - # At first replace the image in to_rebuid based - # on the coordinates from temp dict. - image_id, parent_id = nvr_to_coordinates[nvr] - to_rebuild[image_id][parent_id] = nvr_to_image[latest_nvr] + for image_id, parent_id in nvr_to_coordinates[nvr]: + # At first replace the image in to_rebuid based + # on the coordinates from temp dict. + to_rebuild[image_id][parent_id] = nvr_to_image[latest_nvr] - # And in case this image is not the the leaf image, also replace - # the ["parent"] record for the child image to point to the image - # with highest NVR. - if parent_id != 0: - to_rebuild[image_id][parent_id - 1]["parent"] = nvr_to_image[latest_nvr] + # And in case this image is not the the leaf image, also replace + # the ["parent"] record for the child image to point to the image + # with highest NVR. + if parent_id != 0: + to_rebuild[image_id][parent_id - 1]["parent"] = nvr_to_image[latest_nvr] return to_rebuild + def _images_to_rebuild_to_batches(self, to_rebuild): + """ + Creates batches with images as defined by `find_images_to_rebuild` + output from the `to_rebuild` list in following format: + + [ + [child_image, parent_of_child_image, parent_of_parent, ...], + ... + ] + """ + # At first get the max length of list in to_rebuild list. + max_len = 0 + for rebuild_list in to_rebuild: + max_len = max(len(rebuild_list), max_len) + + # Now create the batches with images. We still might find duplicate + # images in to_rebuild lists in two cases: + # + # 1) A depends on X and also B depends on X. The X then would be + # added to first batch twice. This is simple to fix by just + # adding same image to batch once. + # 2) A depends on X and A is also standalone image to rebuild. In this + # case, A would be in the second batch, because A must be built + # before X, but it is also standalone image to be rebuilt, so it + # would appear also in the first batch. + # To fix this, we at first add images with the longest dependency + # chains, so A will be added to second batch. Once we try to add + # standalone version of A, we won't add it, because it already + # exists in some batch. + # + # Both of these cases are handled by adding the image to `seen` set + # and checking if it exists there already before adding it again. + batches = [[] for i in range(max_len)] + seen = set() + for image_rebuild_list in sorted(to_rebuild, key=lambda lst: len(lst), reverse=True): + for image, batch in zip(reversed(image_rebuild_list), batches): + image_key = image["brew"]["build"] + if image_key in seen: + continue + seen.add(image_key) + batch.append(image) + return batches + def find_images_to_rebuild( - self, srpm_name, content_sets, published=True, deprecated=False, + self, srpm_names, content_sets, published=True, deprecated=False, release_category="Generally Available", filter_fnc=None): """ Find images to rebuild through image build layers @@ -991,7 +1037,7 @@ class LightBlue(object): image from N+1 must happen *after* all of the images from sub-list N have been rebuilt. - :param str srpm_name: srpm_name (source rpm name) to look for + :param list srpm_names: List of srpm_name (source rpm name) 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 @@ -1007,79 +1053,72 @@ class LightBlue(object): This function is used to filter out images not allowed by Freshmaker configuration. """ - images = self.find_images_with_package_from_content_set( - srpm_name, content_sets, filter_fnc, published, deprecated, + images = self.find_images_with_packages_from_content_set( + srpm_names, content_sets, filter_fnc, published, deprecated, release_category) def _get_images_to_rebuild(image): """ Find out parent images to rebuild, helper called from threadpool. """ - unpublished = self.find_unpublished_image_for_build( - image['brew']['build']) - if not unpublished: - image.log_error( - "Cannot find unpublished version of image, Lightblue " - "data is probably incomplete") - return [image] - - layers = unpublished["parsed_data"]["layers"] - rebuild_list = self.find_parent_images_with_package( - image, srpm_name, layers) - if rebuild_list: - image['parent'] = rebuild_list[0] - else: - parent = self.get_image_by_layer(layers[1], len(layers) - 1) - if parent: - parent.resolve_content_sets( - self, [image], published, deprecated, - release_category) - parent.resolve_commit(srpm_name) - elif len(layers) != 2: + rebuild_list = {} # per srpm-name rebuild list. + for srpm_name in srpm_names: + for rpm in image["rpm_manifest"][0]["rpms"]: + if rpm["srpm_name"] == srpm_name: + break + else: + # This `srpm_name` is not in image. + continue + + unpublished = self.find_unpublished_image_for_build( + image['brew']['build']) + if not unpublished: image.log_error( - "Cannot find parent image with layer %s and layer " - "count %d in Lightblue, Lightblue data is probably " - "incomplete" % (layers[1], len(layers) - 1)) - image['parent'] = parent - rebuild_list.insert(0, image) + "Cannot find unpublished version of image, Lightblue " + "data is probably incomplete") + rebuild_list[srpm_name] = [image] + continue + + layers = unpublished["parsed_data"]["layers"] + rebuild_list[srpm_name] = self.find_parent_images_with_package( + image, srpm_name, layers) + if rebuild_list[srpm_name]: + image['parent'] = rebuild_list[srpm_name][0] + else: + parent = self.get_image_by_layer(layers[1], len(layers) - 1) + if parent: + parent.resolve_content_sets( + self, [image], published, deprecated, + release_category) + parent.resolve_commit() + elif len(layers) != 2: + image.log_error( + "Cannot find parent image with layer %s and layer " + "count %d in Lightblue, Lightblue data is probably " + "incomplete" % (layers[1], len(layers) - 1)) + image['parent'] = parent + rebuild_list[srpm_name].insert(0, image) return rebuild_list # For every image, find out all its parent images which contain the # srpm_name package and store these lists to to_rebuild. to_rebuild = [] - max_len = 0 - with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor: + with concurrent.futures.ThreadPoolExecutor( + max_workers=conf.max_thread_workers) as executor: futures = {executor.submit(_get_images_to_rebuild, i): i for i in images} concurrent.futures.wait(futures) for future in futures: - rebuild_list = future.result() - max_len = max(len(rebuild_list), max_len) - to_rebuild.append(rebuild_list) + rebuild_lists = future.result() + for rebuild_list in rebuild_lists.values(): + to_rebuild.append(rebuild_list) # The to_rebuild list now contains all the images which need to be # rebuilt, but there are lot of duplicates there. - # At first remove duplicated images which share the same namd and + # At first remove duplicated images which share the same name and # version, but different release. to_rebuild = self._deduplicate_images_to_rebuild(to_rebuild) - # Now create the batches with images. We still might find duplicate - # images in batch. For example if the image A depends on X and also - # image B depends on X, the X would be in the first batch twice. - # We remove duplicates like that using the dict for each batch with - # Brew build NVR as a key and add the image to the batch only when - # it is not in this dict. - batches = [{} for i in range(max_len)] - for image_rebuild_list in to_rebuild: - for image, batch in zip(reversed(image_rebuild_list), batches): - image_key = image["brew"]["build"] - - if image_key not in batch: - batch[image_key] = image - - # Final step to convert batches to list of sub-lists - # Each sublist contains images in this order - # [found image containing signed RPMs, parent, grandparent, ...] - batches = [batch.values() for batch in batches] - return batches + # Now generate batches from deduplicated list and return it. + return self._images_to_rebuild_to_batches(to_rebuild) diff --git a/freshmaker/utils.py b/freshmaker/utils.py index f3191b7..9390921 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -31,6 +31,7 @@ import sys import tempfile import time import koji +import kobo.rpmlib from freshmaker import conf, app, log from freshmaker.types import ArtifactType @@ -38,6 +39,42 @@ from krbcontext import krbContext from flask import has_app_context, url_for +def _cmp(a, b): + """ + Replacement for cmp() in Python 3. + """ + return (a > b) - (a < b) + + +def sorted_by_nvr(lst, get_nvr=None, reverse=False): + """ + Sorts the list `lst` containing NVR by the NVRs. + + :param list lst: List with NVRs to sort. + :param fnc get_nvr: Function taking the item from a list and returning + the NVR. If None, the item from `lst` is expected to be NVR string. + :param bool reverse: When True, the result of sorting is reversed. + :rtype: list + :return: Sorted `lst`. + """ + def _compare_items(item1, item2): + if get_nvr: + nvr1 = get_nvr(item1) + nvr2 = get_nvr(item2) + else: + nvr1 = item1 + nvr2 = item2 + + nvr1_dict = kobo.rpmlib.parse_nvr(nvr1) + nvr2_dict = kobo.rpmlib.parse_nvr(nvr2) + if nvr1_dict["name"] != nvr2_dict["name"]: + return _cmp(nvr1_dict["name"], nvr2_dict["name"]) + return kobo.rpmlib.compare_nvr(nvr1_dict, nvr2_dict) + + return sorted( + lst, key=functools.cmp_to_key(_compare_items), reverse=reverse) + + def get_url_for(*args, **kwargs): """ flask.url_for wrapper which creates the app_context on-the-fly. diff --git a/tests/test_errata_advisory_rpms_signed_handler.py b/tests/test_errata_advisory_rpms_signed_handler.py index 3013d13..b58fa49 100644 --- a/tests/test_errata_advisory_rpms_signed_handler.py +++ b/tests/test_errata_advisory_rpms_signed_handler.py @@ -188,13 +188,11 @@ class TestErrataAdvisoryRPMsSignedHandler(helpers.ModelsTestCase): # For simplicify, mocking _find_images_to_rebuild to just return one # batch, which contains images found for rebuild from parent to # childrens. - self.mock_find_images_to_rebuild.return_value = iter([ - [ - [self.image_a, self.image_b], - [self.image_c, self.image_d, self.image_e], - [self.image_f] - ] - ]) + self.mock_find_images_to_rebuild.return_value = [ + [self.image_a, self.image_b], + [self.image_c, self.image_d, self.image_e], + [self.image_f] + ] def tearDown(self): super(TestErrataAdvisoryRPMsSignedHandler, self).tearDown() @@ -211,7 +209,7 @@ class TestErrataAdvisoryRPMsSignedHandler(helpers.ModelsTestCase): db.session.add(compose_4) db.session.commit() - self.mock_find_images_to_rebuild.return_value = iter([[[]]]) + self.mock_find_images_to_rebuild.return_value = [[]] event = ErrataAdvisoryRPMsSignedEvent( "123", "RHBA-2017", 123, "", "REL_PREP", "product") handler = ErrataAdvisoryRPMsSignedHandler() @@ -231,7 +229,7 @@ class TestErrataAdvisoryRPMsSignedHandler(helpers.ModelsTestCase): db.session.add(compose_4) db.session.commit() - self.mock_find_images_to_rebuild.return_value = iter([[[]]]) + self.mock_find_images_to_rebuild.return_value = [[]] event = ErrataAdvisoryRPMsSignedEvent( "123", "RHBA-2017", 123, "", "REL_PREP", "product") handler = ErrataAdvisoryRPMsSignedHandler() @@ -245,7 +243,7 @@ class TestErrataAdvisoryRPMsSignedHandler(helpers.ModelsTestCase): } }) def test_event_state_updated_when_no_images_to_rebuild(self): - self.mock_find_images_to_rebuild.return_value = iter([[[]]]) + self.mock_find_images_to_rebuild.return_value = [[]] event = ErrataAdvisoryRPMsSignedEvent( "123", "RHBA-2017", 123, "", "REL_PREP", "product") handler = ErrataAdvisoryRPMsSignedHandler() @@ -264,11 +262,8 @@ class TestErrataAdvisoryRPMsSignedHandler(helpers.ModelsTestCase): }) def test_event_state_updated_when_all_images_failed(self): self.image_a['error'] = "foo" - self.mock_find_images_to_rebuild.return_value = iter([ - [ - [self.image_a] - ] - ]) + self.mock_find_images_to_rebuild.return_value = [ + [self.image_a]] event = ErrataAdvisoryRPMsSignedEvent( "123", "RHBA-2017", 123, "", "REL_PREP", "product") handler = ErrataAdvisoryRPMsSignedHandler() @@ -543,7 +538,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): self.find_images_to_rebuild = self.patcher.patch( 'freshmaker.lightblue.LightBlue.find_images_to_rebuild', - return_value=iter([[[]]])) + return_value=[[]]) self.event = ErrataAdvisoryRPMsSignedEvent( "123", "RHBA-2017", 123, "", "REL_PREP", "product") @@ -565,7 +560,23 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - 'httpd', ['content-set-1'], + set(['httpd']), ['content-set-1'], + filter_fnc=self.handler._filter_out_not_allowed_builds, + published=True, release_category='Generally Available') + + @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ + 'ErrataAdvisoryRPMsSignedHandler': { + 'image': [{'advisory_name': 'RHBA-*'}] + } + }) + @patch('os.path.exists', return_value=True) + def test_multiple_srpms(self, exists): + self.get_builds.return_value = ["httpd-2.4-11.el7", "httpd-2.2-11.el6"] + for x in self.handler._find_images_to_rebuild(123456): + pass + + self.find_images_to_rebuild.assert_called_once_with( + set(['httpd']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available') @@ -583,7 +594,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - 'httpd', ['content-set-1'], + set(['httpd']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=None, release_category=None) @@ -599,7 +610,7 @@ class TestFindImagesToRebuild(helpers.FreshmakerTestCase): pass self.find_images_to_rebuild.assert_called_once_with( - 'httpd', ['content-set-1'], + set(['httpd']), ['content-set-1'], filter_fnc=self.handler._filter_out_not_allowed_builds, published=True, release_category='Generally Available') diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 866fe88..0cfd007 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -31,6 +31,7 @@ from freshmaker.lightblue import ContainerRepository from freshmaker.lightblue import LightBlue from freshmaker.lightblue import LightBlueRequestError from freshmaker.lightblue import LightBlueSystemError +from freshmaker.utils import sorted_by_nvr from tests import helpers @@ -175,7 +176,7 @@ class TestContainerImageObject(helpers.FreshmakerTestCase): get_task_request.return_value = [ "git://example.com/rpms/repo-1?#commit_hash1", "target1", {}] - image.resolve_commit("openssl") + image.resolve_commit() self.assertEqual(image["repository"], "rpms/repo-1") self.assertEqual(image["commit"], "commit_hash1") self.assertEqual(image["target"], "target1") @@ -206,7 +207,7 @@ class TestContainerImageObject(helpers.FreshmakerTestCase): get_build.return_value = {} - image.resolve_commit("openssl") + image.resolve_commit() self.assertEqual(image["repository"], None) self.assertEqual(image["commit"], None) self.assertEqual(image["target"], None) @@ -240,7 +241,7 @@ class TestContainerImageObject(helpers.FreshmakerTestCase): get_build.return_value = {"task_id": None} - image.resolve_commit("openssl") + image.resolve_commit() self.assertEqual(image["repository"], None) self.assertEqual(image["commit"], None) self.assertEqual(image["target"], None) @@ -641,8 +642,8 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): private_key=self.fake_private_key) repositories = self.fake_repositories_with_content_sets cont_images.return_value = self.fake_images_with_parsed_data - ret = lb.find_images_with_included_srpm(repositories, - "openssl") + ret = lb.find_images_with_included_srpms(repositories, + ["openssl"]) expected_image_request = { "objectType": "containerImage", @@ -668,9 +669,13 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): "rvalue": "latest" }, { - "field": "rpm_manifest.*.rpms.*.srpm_name", - "op": "=", - "rvalue": "openssl" + "$or": [ + { + "field": "rpm_manifest.*.rpms.*.srpm_name", + "op": "=", + "rvalue": "openssl" + }, + ], }, { "field": "parsed_data.files.*.key", @@ -724,7 +729,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): lb = LightBlue(server_url=self.fake_server_url, cert=self.fake_cert_file, private_key=self.fake_private_key) - ret = lb.find_images_with_package_from_content_set( + ret = lb.find_images_with_packages_from_content_set( "openssl", ["dummy-content-set-1"], filter_fnc=self._filter_fnc) # Only the first image should be returned, because the first one @@ -898,7 +903,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): self.assertEqual(ret[1]["content_sets"], set(["content-set"])) self.assertEqual(ret[2]["content_sets"], set(["content-set"])) - @patch('freshmaker.lightblue.LightBlue.find_images_with_package_from_content_set') + @patch('freshmaker.lightblue.LightBlue.find_images_with_packages_from_content_set') @patch('freshmaker.lightblue.LightBlue.find_parent_images_with_package') @patch('freshmaker.lightblue.LightBlue.find_unpublished_image_for_build') @patch('os.path.exists') @@ -906,7 +911,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): exists, find_unpublished_image_for_build, find_parent_images_with_package, - find_images_with_package_from_content_set): + find_images_with_packages_from_content_set): exists.return_value = True image_a = ContainerImage.create({ @@ -1003,8 +1008,16 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): leaf_image1, leaf_image2, leaf_image3, leaf_image4, leaf_image5, leaf_image6 ] + + for image in images: + image["rpm_manifest"] = [{ + "rpms": [ + {"srpm_name": "dummy"} + ] + }] + find_unpublished_image_for_build.side_effect = images - find_images_with_package_from_content_set.return_value = images + find_images_with_packages_from_content_set.return_value = images find_parent_images_with_package.side_effect = [ [image_b, image_a], # parents of leaf_image1 @@ -1017,7 +1030,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"], ["dummy"]) # Each of batch is sorted for assertion easily expected_batches = [ @@ -1030,30 +1043,34 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): returned_batches = [sorted(images, key=lambda image: image['brew']['build']) for images in batches] - self.assertEqual(expected_batches, returned_batches) - @patch('freshmaker.lightblue.LightBlue.find_images_with_package_from_content_set') + @patch('freshmaker.lightblue.LightBlue.find_images_with_packages_from_content_set') @patch('freshmaker.lightblue.LightBlue.find_unpublished_image_for_build') @patch('os.path.exists') def test_images_to_rebuild_cannot_find_unpublished( self, exists, find_unpublished_image_for_build, - find_images_with_package_from_content_set): + find_images_with_packages_from_content_set): exists.return_value = True image_a = ContainerImage.create({ 'brew': {'package': 'image-a', 'build': 'image-a-v-r1'}, 'repository': 'repo-1', - 'commit': 'image-a-commit' + 'commit': 'image-a-commit', + 'rpm_manifest': [{ + "rpms": [ + {"srpm_name": "dummy"} + ] + }] }) find_unpublished_image_for_build.return_value = None - find_images_with_package_from_content_set.return_value = [image_a] + find_images_with_packages_from_content_set.return_value = [image_a] 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"], ["dummy"]) self.assertEqual(len(batches), 1) self.assertEqual(len(batches[0]), 1) @@ -1078,7 +1095,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) with self.assertRaises(LightBlueRequestError): - lb.find_images_with_package_from_content_set( + lb.find_images_with_packages_from_content_set( "openssl", ["dummy-content-set-1"]) @@ -1087,7 +1104,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): {"errors": [{"msg": "dummy error"}]}, http_client.REQUEST_TIMEOUT) with self.assertRaises(LightBlueRequestError): - lb.find_images_with_package_from_content_set( + lb.find_images_with_packages_from_content_set( "openssl", ["dummy-content-set-1"]) @@ -1183,32 +1200,123 @@ class TestDeduplicateImagesToRebuild(helpers.FreshmakerTestCase): def test_use_highest_nvr(self): httpd = self._create_imgs([ "httpd-2.4-12", - "s2i-base-1-3", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-125", + ]) + + perl = self._create_imgs([ + "perl-5.7-1", + "s2i-base-1-2", "s2i-core-1-2", + "rhel-server-docker-7.4-150", + ]) + + expected_images = [ + self._create_imgs([ + "httpd-2.4-12", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-150", + ]), + self._create_imgs([ + "perl-5.7-1", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-150", + ]) + ] + + ret = self.lb._deduplicate_images_to_rebuild([httpd, perl]) + self.assertEqual(ret, expected_images) + + def test_keep_multiple_nvs(self): + httpd = self._create_imgs([ + "httpd-2.4-12", + "s2i-base-1-10", + "s2i-core-1-11", "rhel-server-docker-7.4-125", ]) perl = self._create_imgs([ "perl-5.7-1", - "s2i-base-1-1", - "s2i-core-1-1", + "s2i-base-2-2", + "s2i-core-2-2", "rhel-server-docker-7.4-150", ]) expected_images = [ self._create_imgs([ "httpd-2.4-12", - "s2i-base-1-3", - "s2i-core-1-2", + "s2i-base-1-10", + "s2i-core-1-11", "rhel-server-docker-7.4-150", ]), self._create_imgs([ "perl-5.7-1", - "s2i-base-1-3", - "s2i-core-1-2", + "s2i-base-2-2", + "s2i-core-2-2", "rhel-server-docker-7.4-150", ]) ] ret = self.lb._deduplicate_images_to_rebuild([httpd, perl]) self.assertEqual(ret, expected_images) + + def test_batches_same_image_in_batch(self): + httpd = self._create_imgs([ + "httpd-2.4-12", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-150", + ]) + perl = self._create_imgs([ + "perl-5.7-1", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-150", + ]) + to_rebuild = [httpd, perl] + batches = self.lb._images_to_rebuild_to_batches(to_rebuild) + batches = [ + sorted_by_nvr(images, get_nvr=lambda image: image['brew']['build']) + for images in batches] + + # Both perl and httpd share the same parent images, so include + # just httpd's one in expected batches - they are the same as + # for perl one. But for the last batch, we expect both images. + expected = [ + [httpd[3]], + [httpd[2]], + [httpd[1]], + [httpd[0], perl[0]], + ] + + self.assertEqual(batches, expected) + + def test_batches_standalone_image_in_batch(self): + # Create to_rebuild list of following images: + # [ + # [httpd, s2i-base, s2i-core, rhel-server-docker], + # [s2i-base, s2i-core, rhel-server-docker], + # ..., + # [rhel-server-docker] + # ] + deps = self._create_imgs([ + "httpd-2.4-12", + "s2i-base-1-10", + "s2i-core-1-11", + "rhel-server-docker-7.4-150", + ]) + to_rebuild = [] + for i in range(len(deps)): + to_rebuild.append(deps[i:]) + + batches = self.lb._images_to_rebuild_to_batches(to_rebuild) + batches = [ + sorted_by_nvr(images, get_nvr=lambda image: image['brew']['build']) + for images in batches] + + # We expect each image to be rebuilt just once. + expected = [[deps[3]], [deps[2]], [deps[1]], [deps[0]]] + self.assertEqual(batches, expected)