From 7ae39794e7bee88ba2ae75750a216ed6bc037e45 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Oct 02 2017 06:10:30 +0000 Subject: [PATCH 1/3] When LB class fails to find additional information about container image in the Koji, mark the container image build as failed. --- diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 0258e66..cd1628a 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -338,10 +338,19 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): parent_name = image["parent"]["brew"]["build"] \ if image["parent"] else None dep_on = builds[parent_name] if parent_name in builds else None + + if "error" in image and image["error"]: + #state_reason = image["error"] + state = ArtifactBuildState.FAILED.value + else: + #state_reason = "" + state = ArtifactBuildState.PLANNED.value + + # TODO: Set state_reason, waiting on PR#88 build = self.record_build( event, name, ArtifactType.IMAGE, dep_on=dep_on, - state=ArtifactBuildState.PLANNED.value) + state=state) build_args = {} build_args["repository"] = image["repository"] diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 51fe7fe..b7f0d83 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -26,6 +26,7 @@ import os import re import requests import six +import dogpile.cache from six.moves import http_client import concurrent.futures @@ -101,7 +102,7 @@ class ContainerRepository(dict): class ContainerImage(dict): """Represent a container image""" - KOJI_BUILDS_CACHE = {} + region = dogpile.cache.make_region().configure(conf.dogpile_cache_backend) @classmethod def create(cls, data): @@ -112,6 +113,57 @@ class ContainerImage(dict): def __hash__(self): return hash((self['brew']['build'])) + @region.cache_on_arguments() + def _get_additional_data_from_koji(self, nvr): + """ + Finds the build defined by `nvr` in Koji and returns dict with + additional information about this build including "repository", + "commit", "target" and "git_branch". + + In case of lookup error, the "error" will be set to error string. + """ + data = {"repository": None, "commit": None, "target": None, + "git_branch": None, "error": None} + + with koji_service(conf.koji_profile, log) as session: + build = session.get_build(nvr) + if not build: + err = "Cannot find Koji build with nvr %s in Koji." % nvr + log.error(err) + data["error"] = err + return data + + if 'task_id' not in build or not build['task_id']: + if ("extra" in build and + "container_koji_task_id" in build["extra"] and + build["extra"]["container_koji_task_id"]): + build['task_id'] = build["extra"]['container_koji_task_id'] + else: + err = "Cannot find task_id or container_koji_task_id " \ + "in the Koji build %r" % build + log.error(err) + data["error"] = err + return data + + brew_task = session.get_task_request( + build['task_id']) + source = brew_task[0] + data["target"] = brew_task[1] + extra_data = brew_task[2] + if "git_branch" in extra_data: + data["git_branch"] = extra_data["git_branch"] + else: + data["git_branch"] = "unknown" + + m = re.match(r".*/(?P.*)/(?P.*)#(?P.*)", source) + if m: + namespace = m.group("namespace") + container = m.group("container") + data["repository"] = namespace + "/" + container + data["commit"] = m.group("commit") + + return data + def resolve_commit(self, srpm_name): """ Uses the ContainerImage data to resolve the information about @@ -130,56 +182,10 @@ class ContainerImage(dict): srpm_nevra = rpm['srpm_nevra'] break - reponame = None - commit = None - target = None - git_branch = None - - # Find the repository name, commit id and koji target form the Koji - # build. + # Find the additional data for Container build in Koji. nvr = self["brew"]["build"] - if nvr in ContainerImage.KOJI_BUILDS_CACHE: - reponame, commit, target = ContainerImage.KOJI_BUILDS_CACHE[nvr] - else: - with koji_service(conf.koji_profile, log) as session: - build = session.get_build(nvr) - if not build: - err = "Cannot find Koji build with nvr %s in Koji." % nvr - log.error(err) - raise ValueError(err) - - if not build['task_id']: - if ("extra" in build and - "container_koji_task_id" in build["extra"] and - build["extra"]["container_koji_task_id"]): - build['task_id'] = \ - build["extra"]['container_koji_task_id'] - else: - err = "Cannot find task_id or container_koji_task_id " \ - "in the Koji build %r" % build - log.error(err) - raise ValueError(err) - - brew_task = session.get_task_request( - build['task_id']) - source = brew_task[0] - target = brew_task[1] - extra_data = brew_task[2] - if "git_branch" in extra_data: - git_branch = extra_data["git_branch"] - else: - git_branch = "unknown" - - m = re.match(r".*/(?P.*)/(?P.*)#(?P.*)", source) - if m: - namespace = m.group("namespace") - container = m.group("container") - reponame = namespace + "/" + container - commit = m.group("commit") - ContainerImage.KOJI_BUILDS_CACHE[nvr] = (reponame, commit, target) - - data = {"repository": reponame, "commit": commit, "target": target, - "srpm_nevra": srpm_nevra, "git_branch": git_branch} + data = self._get_additional_data_from_koji(nvr) + data["srpm_nevra"] = srpm_nevra self.update(data) diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index 83aec48..e31129a 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -199,12 +199,12 @@ class TestBatches(unittest.TestCase): db.drop_all() db.session.commit() - def _mock_build(self, build, parent=None): + def _mock_build(self, build, parent=None, error=None): if parent: parent = {"brew": {"build": parent}} return {'brew': {'build': build}, 'repository': build + '_repo', 'commit': build + '_123', 'parent': parent, "target": "t1", - 'git_branch': 'mybranch'} + 'git_branch': 'mybranch', "error": error} def test_batches_records(self): """ @@ -219,7 +219,7 @@ class TestBatches(unittest.TestCase): # |- child2_parent2 # |- child2_parent1 # |- child2 - batches = [[self._mock_build("shared_parent")], + batches = [[self._mock_build("shared_parent", error="Fail")], [self._mock_build("child1_parent3", "shared_parent"), self._mock_build("child2_parent2", "shared_parent")], [self._mock_build("child1_parent2", "child1_parent3"), @@ -242,7 +242,12 @@ class TestBatches(unittest.TestCase): # Check that the images have proper data in proper db columns. e = db.session.query(Event).filter(Event.id == 1).one() for build in e.builds: - self.assertEqual(build.state, ArtifactBuildState.PLANNED.value) + # shared-parent is in FAILED state, because LB failed to resolve + # it. + if build.name == "shared_parent": + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + else: + self.assertEqual(build.state, ArtifactBuildState.PLANNED.value) self.assertEqual(build.type, ArtifactType.IMAGE.value) image = images[build.name] diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 8bc8efa..d50a433 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -167,6 +167,77 @@ class TestContainerImageObject(unittest.TestCase): self.assertEqual(image["srpm_nevra"], "openssl-0:1.2.3-1.src") + @patch('freshmaker.kojiservice.KojiService.get_build') + @patch('freshmaker.kojiservice.KojiService.get_task_request') + def test_resolve_commit_no_koji_build(self, get_task_request, get_build): + image = ContainerImage.create({ + '_id': '1233829', + 'brew': { + 'completion_date': u'20170421T04:27:51.000-0400', + 'build': 'package-name-1-4-12.10', + 'package': 'package-name-1' + }, + 'parsed_data': { + 'rpm_manifest': [ + { + "srpm_name": "openssl", + "srpm_nevra": "openssl-0:1.2.3-1.src" + }, + { + "srpm_name": "tespackage", + "srpm_nevra": "testpackage-10:1.2.3-1.src" + } + ] + } + }) + + get_build.return_value = {} + + image.resolve_commit("openssl") + self.assertEqual(image["repository"], None) + self.assertEqual(image["commit"], None) + self.assertEqual(image["target"], None) + self.assertEqual(image["srpm_nevra"], "openssl-0:1.2.3-1.src") + self.assertEqual( + image["error"], + "Cannot find Koji build with nvr package-name-1-4-12.10 in Koji.") + + @patch('freshmaker.kojiservice.KojiService.get_build') + @patch('freshmaker.kojiservice.KojiService.get_task_request') + def test_resolve_commit_no_task_id(self, get_task_request, get_build): + image = ContainerImage.create({ + '_id': '1233829', + 'brew': { + 'completion_date': u'20170421T04:27:51.000-0400', + 'build': 'package-name-1-4-12.10', + 'package': 'package-name-1' + }, + 'parsed_data': { + 'rpm_manifest': [ + { + "srpm_name": "openssl", + "srpm_nevra": "openssl-0:1.2.3-1.src" + }, + { + "srpm_name": "tespackage", + "srpm_nevra": "testpackage-10:1.2.3-1.src" + } + ] + } + }) + + get_build.return_value = {"task_id": None} + + image.resolve_commit("openssl") + self.assertEqual(image["repository"], None) + self.assertEqual(image["commit"], None) + self.assertEqual(image["target"], None) + self.assertEqual(image["srpm_nevra"], "openssl-0:1.2.3-1.src") + self.assertEqual( + image["error"], + "Cannot find task_id or container_koji_task_id in the Koji build " + "{'task_id': None}") + class TestContainerRepository(unittest.TestCase): def test_create(self): @@ -569,6 +640,7 @@ class TestQueryEntityFromLightBlue(unittest.TestCase): "srpm_nevra": "openssl-0:1.2.3-1.src", "target": "target1", "git_branch": "mybranch", + "error": None, "brew": { "completion_date": u"20170421T04:27:51.000-0400", "build": "package-name-1-4-12.10", @@ -600,6 +672,7 @@ class TestQueryEntityFromLightBlue(unittest.TestCase): "srpm_nevra": "openssl-1:1.2.3-1.src", "target": "target2", "git_branch": "mybranch", + "error": None, "brew": { "completion_date": u"20170421T04:27:51.000-0400", "build": "package-name-2-4-12.10", From fa800a95beb952510e2f6839482790f303ac31ed Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Oct 02 2017 06:10:30 +0000 Subject: [PATCH 2/3] Raise an exception in the get_additional_data() instead of returning an error. Mark builds depending on failed builds as failed. --- diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index cd1628a..2dd523d 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -340,17 +340,23 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): dep_on = builds[parent_name] if parent_name in builds else None if "error" in image and image["error"]: - #state_reason = image["error"] + state_reason = image["error"] + state = ArtifactBuildState.FAILED.value + elif dep_on and dep_on.state == ArtifactBuildState.FAILED.value: + # If this artifact build depends on a build which cannot + # be built by Freshmaker, mark this one as failed too. + state_reason = "Cannot build artifact, because its " \ + "dependency cannot be built." state = ArtifactBuildState.FAILED.value else: - #state_reason = "" + state_reason = "" state = ArtifactBuildState.PLANNED.value - # TODO: Set state_reason, waiting on PR#88 build = self.record_build( event, name, ArtifactType.IMAGE, dep_on=dep_on, state=state) + build.state_reason = state_reason build_args = {} build_args["repository"] = image["repository"] diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index b7f0d83..a5045b8 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -88,6 +88,9 @@ class LightBlueRequestError(LightBlueError): for err in self.raw['errors'])) ) +class KojiLookupError(ValueError): + """ Koji lookup error """ + pass class ContainerRepository(dict): """Represent a container repository""" @@ -113,6 +116,10 @@ class ContainerImage(dict): def __hash__(self): return hash((self['brew']['build'])) + def _get_default_additional_data(self): + return {"repository": None, "commit": None, "target": None, + "git_branch": None, "error": None} + @region.cache_on_arguments() def _get_additional_data_from_koji(self, nvr): """ @@ -122,16 +129,13 @@ class ContainerImage(dict): In case of lookup error, the "error" will be set to error string. """ - data = {"repository": None, "commit": None, "target": None, - "git_branch": None, "error": None} + data = self._get_default_additional_data() with koji_service(conf.koji_profile, log) as session: build = session.get_build(nvr) if not build: - err = "Cannot find Koji build with nvr %s in Koji." % nvr - log.error(err) - data["error"] = err - return data + raise KojiLookupError( + "Cannot find Koji build with nvr %s in Koji" % nvr) if 'task_id' not in build or not build['task_id']: if ("extra" in build and @@ -139,11 +143,9 @@ class ContainerImage(dict): build["extra"]["container_koji_task_id"]): build['task_id'] = build["extra"]['container_koji_task_id'] else: - err = "Cannot find task_id or container_koji_task_id " \ - "in the Koji build %r" % build - log.error(err) - data["error"] = err - return data + raise KojiLookupError( + "Cannot find task_id or container_koji_task_id " + "in the Koji build %r" % build) brew_task = session.get_task_request( build['task_id']) @@ -184,7 +186,14 @@ class ContainerImage(dict): # Find the additional data for Container build in Koji. nvr = self["brew"]["build"] - data = self._get_additional_data_from_koji(nvr) + try: + data = self._get_additional_data_from_koji(nvr) + except KojiLookupError as e: + err = "Cannot get data from Koji for build %s: %s." % (nvr, e) + log.error(err) + data = self._get_default_additional_data() + data["error"] = err + data["srpm_nevra"] = srpm_nevra self.update(data) diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index e31129a..204c220 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -219,12 +219,12 @@ class TestBatches(unittest.TestCase): # |- child2_parent2 # |- child2_parent1 # |- child2 - batches = [[self._mock_build("shared_parent", error="Fail")], + batches = [[self._mock_build("shared_parent")], [self._mock_build("child1_parent3", "shared_parent"), self._mock_build("child2_parent2", "shared_parent")], [self._mock_build("child1_parent2", "child1_parent3"), self._mock_build("child2_parent1", "child2_parent2")], - [self._mock_build("child1_parent1", "child1_parent2"), + [self._mock_build("child1_parent1", "child1_parent2", error="Fail"), self._mock_build("child2", "child2_parent1")], [self._mock_build("child1", "child1_parent1")]] @@ -242,9 +242,10 @@ class TestBatches(unittest.TestCase): # Check that the images have proper data in proper db columns. e = db.session.query(Event).filter(Event.id == 1).one() for build in e.builds: - # shared-parent is in FAILED state, because LB failed to resolve - # it. - if build.name == "shared_parent": + # child1_parent1 and child1 are in FAILED states, because LB failed + # to resolve child1_parent1 and therefore also child1 cannot be + # build. + if build.name in ["child1_parent1", "child1"]: self.assertEqual(build.state, ArtifactBuildState.FAILED.value) else: self.assertEqual(build.state, ArtifactBuildState.PLANNED.value) diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index d50a433..207ec93 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -198,9 +198,9 @@ class TestContainerImageObject(unittest.TestCase): self.assertEqual(image["commit"], None) self.assertEqual(image["target"], None) self.assertEqual(image["srpm_nevra"], "openssl-0:1.2.3-1.src") - self.assertEqual( - image["error"], - "Cannot find Koji build with nvr package-name-1-4-12.10 in Koji.") + self.assertTrue(image["error"].find( + "Cannot find Koji build with nvr package-name-1-4-12.10 in " + "Koji.") != -1) @patch('freshmaker.kojiservice.KojiService.get_build') @patch('freshmaker.kojiservice.KojiService.get_task_request') @@ -233,10 +233,9 @@ class TestContainerImageObject(unittest.TestCase): self.assertEqual(image["commit"], None) self.assertEqual(image["target"], None) self.assertEqual(image["srpm_nevra"], "openssl-0:1.2.3-1.src") - self.assertEqual( - image["error"], + self.assertTrue(image["error"].find( "Cannot find task_id or container_koji_task_id in the Koji build " - "{'task_id': None}") + "{'task_id': None}") != -1) class TestContainerRepository(unittest.TestCase): From d3cdd5afee9f2ca54a82c483b00cc9505c518500 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Oct 02 2017 06:14:42 +0000 Subject: [PATCH 3/3] Use build.transition in ErrataAdvisoryRPMsSignedHandler --- diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 2dd523d..2c2095a 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -355,8 +355,9 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): build = self.record_build( event, name, ArtifactType.IMAGE, dep_on=dep_on, - state=state) - build.state_reason = state_reason + state=ArtifactBuildState.PLANNED.value) + + build.transition(state, state_reason) build_args = {} build_args["repository"] = image["repository"] diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index 204c220..cb66873 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -24,7 +24,7 @@ import unittest import json -from mock import patch, MagicMock, PropertyMock +from mock import patch, MagicMock, PropertyMock, Mock from freshmaker.handlers.errata import ErrataAdvisoryRPMsSignedHandler from freshmaker.handlers.errata import ErrataAdvisoryStateChangedHandler