From 010a099a3b59e5203794b79aee5bb387afa8a7a4 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Oct 16 2017 08:44:16 +0000 Subject: [PATCH 1/2] Split _find_and_record_images_to_rebuild Purpose of this patch is to make this method easier to be reused and tested. With this patch, any time to test it or call it to find images that contains RPMs included in a specific Errata advisory, do not need care about database, and it is clear for caller to know it just finds images and has no side effect. As a result, it is named to _find_images_to_rebuild and becomes a generator to yield found images for each NVR added to advisory, those images are yielded in proper rebuild order from base image to leaf image through the dependency build chain. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index a8563d4..663f0f6 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -81,7 +81,10 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): # Get and record all images to rebuild based on the current # ErrataAdvisoryRPMsSignedEvent event. - builds = self._find_and_record_images_to_rebuild(db_event, event) + builds = {} + for batches in self._find_images_to_rebuild(db_event.search_key): + builds = self._record_batches(batches, event, builds) + if not builds: log.info('No container images to rebuild for advisory %r', event.errata_name) @@ -115,8 +118,8 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): continue seen_extra_events.append(ev) db_event.add_event_dependency(db.session, ev) - builds = self._find_and_record_images_to_rebuild( - ev, event, builds) + for batches in self._find_images_to_rebuild(ev.search_key): + builds = self._record_batches(batches, event, builds) repo_urls.append(self._prepare_yum_repo(ev)) db.session.commit() @@ -327,7 +330,7 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): Checks the images to rebuild and logs them using log.info(...). :param Event db_event: Database Event associated with images. :param builds dict: list of docker images to build as returned by - _find_and_record_images_to_rebuild(...). + _find_images_to_rebuild(...). """ log.info('Found docker images to rebuild in following order:') batch = 0 @@ -379,7 +382,7 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): :param db_event Event: Database representation of ErrataAdvisoryRPMsSignedEvent. :param builds dict: list of docker images to build as returned by - _find_and_record_images_to_rebuild(...). + _find_images_to_rebuild(...). """ events_to_include = [] for ev in Event.get_unreleased(db.session): @@ -478,7 +481,7 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): def _filter_out_not_allowed_builds(self, image): """ - Helper method for _find_and_record_images_to_rebuild(...) to filter + Helper method for _find_images_to_rebuild(...) to filter out all images which are not allowed to build by configuration. :param ContainerImage image: Image to be checked. @@ -495,24 +498,18 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): return True return False - def _find_and_record_images_to_rebuild(self, db_event, event, builds=None): + def _find_images_to_rebuild(self, errata_id): """ - Finds docker images to rebuild based on the particular - ErrataAdvisoryRPMsSignedEvent and records them into database. + Finds docker rebuild images from each build added to specific Errata + advisory. - :param db_event Event: Database representation of - ErrataAdvisoryRPMsSignedEvent. - :param event ErrataAdvisoryRPMsSignedEvent: The main event this handler - is currently handling. Used to store found docker images to - database. - :param builds dict: list of docker images to build as returned by - previous calls of _find_and_record_images_to_rebuild(...). - :return: mappings extended by and returned from ``_record_batches``. - :rtype: dict - """ + Found images are yielded in proper rebuild order from base images to + leaf images through the docker build dependnecy chain. + :param int errata_id: Errata ID. + """ errata = Errata(conf.errata_tool_server_url) - errata_id = int(db_event.search_key) + errata_id = int(errata_id) # Use the errata_id to find out Pulp repository IDs from Errata Tool # and furthermore get content_sets from Pulp where signed RPM will end @@ -534,7 +531,6 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): # For each RPM package in Errata advisory, find Docker images # containing this package and record those images into database. - builds = builds or {} nvrs = errata.get_builds(errata_id) for nvr in nvrs: # Container images builds end with ".tar.gz", so do not treat @@ -544,10 +540,9 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): batches = lb.find_images_to_rebuild( srpm_name, content_sets, filter_fnc=self._filter_out_not_allowed_builds) - builds = self._record_batches(batches, event, builds) + yield batches else: log.info("Skipping unsupported Errata build type: %s.", nvr) - return builds def _find_build_srpm_name(self, build_nvr): """Find srpm name from a build""" diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index 802260b..03e52d8 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -101,7 +101,7 @@ class TestAllowBuild(unittest.TestCase): db.session.commit() @patch("freshmaker.handlers.errata.ErrataAdvisoryRPMsSignedHandler." - "_find_and_record_images_to_rebuild", return_value=[]) + "_find_images_to_rebuild", return_value=[]) @patch("freshmaker.config.Config.handler_build_whitelist", new_callable=PropertyMock, return_value={ "ErrataAdvisoryRPMsSignedHandler": {"image": [{"advisory_name": "RHSA-.*"}]}}) @@ -116,7 +116,7 @@ class TestAllowBuild(unittest.TestCase): record_images.assert_not_called() @patch("freshmaker.handlers.errata.ErrataAdvisoryRPMsSignedHandler." - "_find_and_record_images_to_rebuild", return_value=[]) + "_find_images_to_rebuild", return_value=[]) @patch("freshmaker.config.Config.handler_build_whitelist", new_callable=PropertyMock, return_value={ "ErrataAdvisoryRPMsSignedHandler": {"image": [{"advisory_name": "RHSA-.*"}]}}) @@ -132,7 +132,7 @@ class TestAllowBuild(unittest.TestCase): record_images.assert_called_once() @patch("freshmaker.handlers.errata.ErrataAdvisoryRPMsSignedHandler." - "_find_and_record_images_to_rebuild", return_value=[]) + "_find_images_to_rebuild", return_value=[]) @patch( "freshmaker.config.Config.handler_build_whitelist", new_callable=PropertyMock, @@ -160,7 +160,7 @@ class TestAllowBuild(unittest.TestCase): record_images.assert_called_once() @patch("freshmaker.handlers.errata.ErrataAdvisoryRPMsSignedHandler." - "_find_and_record_images_to_rebuild", return_value=[]) + "_find_images_to_rebuild", return_value=[]) @patch( "freshmaker.config.Config.handler_build_whitelist", new_callable=PropertyMock, @@ -699,36 +699,23 @@ class TestPrepareYumRepo(unittest.TestCase): "of advisory 123 is the latest build in its candidate tag.")) -class TestFindAndRecordImagesToRebuild(unittest.TestCase): - def setup(self): - db.session.remove() - db.drop_all() - db.create_all() - db.session.commit() - - def tearDown(self): - db.session.remove() - db.drop_all() - db.session.commit() +class TestFindImagesToRebuild(unittest.TestCase): @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Pulp') @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.LightBlue') - def test_find_and_record_images_to_rebuild_non_rpm_content( + def test_find_images_to_rebuild_non_rpm_content( self, lb, pulp, errata): """ - Tests that _find_and_record_images_to_rebuild is not called for + Tests that _find_images_to_rebuild is not called for non-rpm content. """ errata.return_value.get_builds.return_value = set(["httpd-2.4.15-1.f27.tar.gz"]) - db_event = Mock(message_id='msg-id', search_key=12345) - event = Mock() - handler = ErrataAdvisoryRPMsSignedHandler() - ret = handler._find_and_record_images_to_rebuild(db_event, event) + ret = list(handler._find_images_to_rebuild(12345)) lb.find_images_to_rebuild.assert_not_called() - self.assertEqual(ret, {}) + self.assertEqual([], ret) class TestFindEventsToInclude(unittest.TestCase): @@ -882,3 +869,190 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): handler = ErrataAdvisoryStateChangedHandler() handler.handle(ev) + + +class TestRecordBatchesImages(unittest.TestCase): + """Test ErrataAdvisoryRPMsSignedHandler._record_batches""" + + def setUp(self): + db.session.remove() + db.drop_all() + db.create_all() + db.session.commit() + + self.mock_event = Mock(msg_id='msg-id', search_key=12345) + + self.event_types_patcher = patch.dict('freshmaker.models.EVENT_TYPES', + {self.mock_event.__class__: -1}) + self.event_types_patcher.start() + + self.prepare_pulp_repo_patcher = patch( + 'freshmaker.handlers.errata.' + 'ErrataAdvisoryRPMsSignedHandler._prepare_pulp_repo', + side_effect=[{'id': 1}, {'id': 2}]) + self.mock_prepare_pulp_repo = self.prepare_pulp_repo_patcher.start() + + def tearDown(self): + self.prepare_pulp_repo_patcher.stop() + self.event_types_patcher.stop() + + db.session.remove() + db.drop_all() + db.session.commit() + + def test_record_batches(self): + batches = [ + [{ + "brew": { + "completion_date": "20170420T17:05:37.000-0400", + "build": "rhel-server-docker-7.3-82", + "package": "rhel-server-docker" + }, + "parent": None, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "123456789", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": None + }], + [{ + "brew": { + "build": "rh-dotnetcore10-docker-1.0-16", + "package": "rh-dotnetcore10-docker", + "completion_date": "20170511T10:06:09.000-0400" + }, + "parent": { + "brew": { + "completion_date": "20170420T17:05:37.000-0400", + "build": "rhel-server-docker-7.3-82", + "package": "rhel-server-docker" + }, + "parent": None, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "123456789", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": None + }, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "987654321", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": None + }] + ] + + handler = ErrataAdvisoryRPMsSignedHandler() + handler._record_batches(batches, self.mock_event) + + # Check parent image + query = db.session.query(ArtifactBuild) + parent_image = query.filter( + ArtifactBuild.original_nvr == 'rhel-server-docker-7.3-82' + ).first() + self.assertNotEqual(None, parent_image) + self.assertEqual(ArtifactBuildState.PLANNED.value, parent_image.state) + + build_args = json.loads(parent_image.build_args) + self.assertEqual(1, build_args['odcs_pulp_compose_id']) + + # Check child image + child_image = query.filter( + ArtifactBuild.original_nvr == 'rh-dotnetcore10-docker-1.0-16' + ).first() + self.assertNotEqual(None, child_image) + self.assertEqual(parent_image, child_image.dep_on) + self.assertEqual(ArtifactBuildState.PLANNED.value, child_image.state) + + build_args = json.loads(child_image.build_args) + self.assertEqual(2, build_args['odcs_pulp_compose_id']) + + def test_mark_failed_state_if_image_has_error(self): + batches = [ + [{ + "brew": { + "completion_date": "20170420T17:05:37.000-0400", + "build": "rhel-server-docker-7.3-82", + "package": "rhel-server-docker" + }, + "parent": None, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "123456789", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": "Some error occurs while getting this image." + }] + ] + + handler = ErrataAdvisoryRPMsSignedHandler() + handler._record_batches(batches, self.mock_event) + + query = db.session.query(ArtifactBuild) + build = query.filter( + ArtifactBuild.original_nvr == 'rhel-server-docker-7.3-82' + ).first() + + self.assertEqual(ArtifactBuildState.FAILED.value, build.state) + + def test_mark_state_failed_if_depended_image_is_failed(self): + batches = [ + [{ + "brew": { + "completion_date": "20170420T17:05:37.000-0400", + "build": "rhel-server-docker-7.3-82", + "package": "rhel-server-docker" + }, + "parent": None, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "123456789", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": "Some error occured." + }], + [{ + "brew": { + "build": "rh-dotnetcore10-docker-1.0-16", + "package": "rh-dotnetcore10-docker", + "completion_date": "20170511T10:06:09.000-0400" + }, + "parent": { + "brew": { + "completion_date": "20170420T17:05:37.000-0400", + "build": "rhel-server-docker-7.3-82", + "package": "rhel-server-docker" + }, + "parent": None, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "123456789", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": None + }, + "content_sets": ["content-set-1"], + "repository": "repo-1", + "commit": "987654321", + "target": "target-candidate", + "git_branch": "rhel-7", + "error": "Some error occured too." + }] + ] + + handler = ErrataAdvisoryRPMsSignedHandler() + handler._record_batches(batches, self.mock_event) + + query = db.session.query(ArtifactBuild) + build = query.filter( + ArtifactBuild.original_nvr == 'rhel-server-docker-7.3-82' + ).first() + self.assertEqual(ArtifactBuildState.FAILED.value, build.state) + + build = query.filter( + ArtifactBuild.original_nvr == 'rh-dotnetcore10-docker-1.0-16' + ).first() + self.assertEqual(ArtifactBuildState.FAILED.value, build.state) From 29c08d8854a60b4f1c6edd8fe1bf56889ba7b3ae Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Oct 16 2017 08:50:53 +0000 Subject: [PATCH 2/2] Simplify code that converts found images to batches for rebuild Test of find_images_to_rebuild is also changed for this patch. Test data is redesigned. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 9a3f576..a6c8894 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -801,44 +801,18 @@ class LightBlue(object): # parent image). # Therefore, group the same parent images from the same inheritance # level to not build them multiple times for each image, but just once. - batches = [] - for i in reversed(range(-max_len, 0)): - batch = [] - seen = [] # Used to remove possible duplicates in single batch. - for imgs in to_rebuild: - if len(imgs) < abs(i): - continue - image = imgs[i] - - # Duplicate build means that it is built from the same - # repository and commit hash. We don't want duplicate builds, - # so in case we find some, do not add it to batch. - seen_dict = {} - if "repository" not in image or "commit" not in image: - log.error("Cannot obtain repository and commit of image %r", - image) - return [] - seen_dict["repository"] = image["repository"] - seen_dict["commit"] = image["commit"] - if seen_dict not in seen: - batch.append(image) - seen.append(seen_dict) - batches.append(batch) - - # In previous step, we have removed only duplicate builds within - # single batch, but we want to remove duplicates between batches too. - # In this step, check all the images in batch N and if we find the - # duplicate image in the batch N + 1, N + 2, ..., remove it from that - # batch. - for i, batch in enumerate(batches): - for image in batch: - for next_batch in batches[i + 1:]: - to_remove = [] - for next_image in next_batch: - if (next_image["repository"] == image["repository"] and - next_image["commit"] == image["commit"]): - to_remove.append(next_image) - for image_to_remove in to_remove: - next_batch.remove(image_to_remove) + # Using dict for each batch to remove duplicate images + 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 = '{0}_{1}'.format(image['repository'], + image['commit']) + 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 diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 2b46006..2817b3b 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -783,56 +783,136 @@ class TestQueryEntityFromLightBlue(unittest.TestCase): @patch('freshmaker.lightblue.LightBlue.find_parent_images_with_package') @patch('freshmaker.lightblue.LightBlue.find_unpublished_image_for_build') @patch('os.path.exists') - def test_images_to_rebuild(self, exists, unpublished_image, - parent_images, cont_images): - + def test_images_to_rebuild(self, + exists, + find_unpublished_image_for_build, + find_parent_images_with_package, + find_images_with_package_from_content_set): exists.return_value = True - child1 = ContainerImage.create({'brew': {'package': 'child1', 'build': 'child1'}, - "parsed_data": {"layers": None}}) - child2 = ContainerImage.create({'brew': {'package': 'child2', 'build': 'child2'}, - "parsed_data": {"layers": None}}) - cont_images.return_value = [child1, child2] - unpublished_image.side_effect = [child1, child2] - - child1_parent1 = ContainerImage.create( - {'brew': {'package': 'child1_parent1', 'build': 'child1_parent1'}}) - child1_parent2 = ContainerImage.create( - {'brew': {'package': 'child1_parent2', 'build': 'child1_parent2'}}) - child1_parent3 = ContainerImage.create( - {'brew': {'package': 'child1_parent3', 'build': 'child1_parent3'}}) - child1_parent4 = ContainerImage.create( - {'brew': {'package': 'shared_parent', 'build': 'shared_parent'}}) - # Include child1_parent2 twice to ensure find_images_to_rebuild - # removes duplicates - child1_parents = [child1_parent1, child1_parent2, child1_parent2, - child1_parent3, child1_parent4] - - child2_parent1 = ContainerImage.create( - {'brew': {'package': 'child2_parent1', 'build': 'child2_parent1'}}) - child2_parent2 = ContainerImage.create( - {'brew': {'package': 'child2_parent2', 'build': 'child2_parent2'}}) - child2_parent3 = ContainerImage.create( - {'brew': {'package': 'shared_parent', 'build': 'shared_parent'}}) - child2_parents = [child2_parent1, child2_parent2, child2_parent3] - - for image in child1_parents + child2_parents + [child1, child2]: - image["repository"] = "repo_" + image["brew"]["build"] - image["commit"] = "commit_" + image["brew"]["build"] - - parent_images.side_effect = [child1_parents, child2_parents] + image_a = ContainerImage.create({ + 'brew': {'package': 'image-a', 'build': 'image-a-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-a-commit' + }) + image_b = ContainerImage.create({ + 'brew': {'package': 'image-b', 'build': 'image-b-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-b-commit', + 'parent': image_a, + }) + image_c = ContainerImage.create({ + 'brew': {'package': 'image-c', 'build': 'image-c-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-c-commit', + 'parent': image_b, + }) + image_e = ContainerImage.create({ + 'brew': {'package': 'image-e', 'build': 'image-e-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-e-commit', + 'parent': image_a, + }) + image_d = ContainerImage.create({ + 'brew': {'package': 'image-d', 'build': 'image-d-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-d-commit', + 'parent': image_e, + }) + image_j = ContainerImage.create({ + 'brew': {'package': 'image-j', 'build': 'image-j-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-j-commit', + 'parent': image_e, + }) + image_k = ContainerImage.create({ + 'brew': {'package': 'image-k', 'build': 'image-k-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-k-commit', + 'parent': image_j, + }) + image_g = ContainerImage.create({ + 'brew': {'package': 'image-g', 'build': 'image-g-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-g-commit', + 'parent': None, + }) + image_f = ContainerImage.create({ + 'brew': {'package': 'image-f', 'build': 'image-f-v-r1'}, + 'repository': 'repo-1', + 'commit': 'image-f-commit', + 'parent': image_g, + }) + leaf_image1 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-1'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image1-commit', + }) + leaf_image2 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-2'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image2-commit', + }) + leaf_image3 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-3'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image3-commit', + }) + leaf_image4 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-4'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image4-commit', + }) + leaf_image5 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-5'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image5-commit', + }) + leaf_image6 = ContainerImage.create({ + 'brew': {'build': 'leaf-image-6'}, + 'parsed_data': {'layers': ['fake layer']}, + 'repository': 'repo-1', + 'commit': 'leaf-image6-commit', + }) + images = [ + leaf_image1, leaf_image2, leaf_image3, + leaf_image4, leaf_image5, leaf_image6 + ] + find_unpublished_image_for_build.side_effect = images + find_images_with_package_from_content_set.return_value = images + + find_parent_images_with_package.side_effect = [ + [image_b, image_a], # parents of leaf_image1 + [image_c, image_b, image_a], # parents of leaf_image2 + [image_k, image_j, image_e, image_a], # parents of leaf_image3 + [image_d, image_e, image_a], # parents of leaf_image4 + [image_a], # parents of leaf_image5 + [image_f, image_g] # parents of leaf_image6 + ] lb = LightBlue(server_url=self.fake_server_url, cert=self.fake_cert_file, private_key=self.fake_private_key) - ret = lb.find_images_to_rebuild("dummy", "dummy") - self.assertEqual([len(x) for x in ret], [1, 2, 2, 1, 1, 1]) - self.assertEqual(set(ret[0]), set([child1_parent4])) - self.assertEqual(set(ret[1]), set([child1_parent3, child2_parent2])) - self.assertEqual(set(ret[2]), set([child1_parent2, child2_parent1])) - self.assertEqual(set(ret[3]), set([child2])) - self.assertEqual(set(ret[4]), set([child1_parent1])) - self.assertEqual(set(ret[5]), set([child1])) + batches = lb.find_images_to_rebuild("dummy", "dummy") + + # Each of batch is sorted for assertion easily + expected_batches = [ + [image_a, image_g], + [image_b, image_e, image_f, leaf_image5], + [image_c, image_d, image_j, leaf_image1, leaf_image6], + [image_k, leaf_image2, leaf_image4], + [leaf_image3] + ] + + self.assertEqual( + expected_batches, + [sorted(images, key=lambda image: image['brew']['build']) + for images in batches]) @patch('freshmaker.lightblue.LightBlue.find_container_repositories') @patch('freshmaker.lightblue.LightBlue.find_container_images')