From 50273b4dbd78fa85245c2e6f28ef339b998ef98f Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Tue, 18 Oct 2016 14:43:08 +0200 Subject: [PATCH] Fix #85 - Add 'state_reason' to ComponentBuild and ModuleBuild models to have a user-friendly error message when build fails. --- migrations/versions/229f6f273a56_.py | 28 ++++++++++++++++++ rida/models.py | 17 ++++++++--- rida/scheduler/handlers/components.py | 11 ++++++-- rida/scheduler/handlers/modules.py | 4 ++- rida/scheduler/handlers/repos.py | 2 +- rida/utils.py | 13 +++++---- rida/views.py | 4 +++ tests/test_scheduler/test_repo_done.py | 39 ++++++++++++++++++++++++++ tests/test_views/test_views.py | 1 + 9 files changed, 106 insertions(+), 13 deletions(-) create mode 100644 migrations/versions/229f6f273a56_.py diff --git a/migrations/versions/229f6f273a56_.py b/migrations/versions/229f6f273a56_.py new file mode 100644 index 00000000..77a65f43 --- /dev/null +++ b/migrations/versions/229f6f273a56_.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 229f6f273a56 +Revises: 1a44272e8b4c +Create Date: 2016-10-20 10:18:03.019775 + +""" + +# revision identifiers, used by Alembic. +revision = '229f6f273a56' +down_revision = '1a44272e8b4c' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('component_builds', sa.Column('state_reason', sa.String(), nullable=True)) + op.add_column('module_builds', sa.Column('state_reason', sa.String(), nullable=True)) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('module_builds', 'state_reason') + op.drop_column('component_builds', 'state_reason') + ### end Alembic commands ### diff --git a/rida/models.py b/rida/models.py index c7f8d631..afd4ad9c 100644 --- a/rida/models.py +++ b/rida/models.py @@ -79,6 +79,7 @@ class ModuleBuild(RidaBase): version = db.Column(db.String, nullable=False) release = db.Column(db.String, nullable=False) state = db.Column(db.Integer, nullable=False) + state_reason = db.Column(db.String) modulemd = db.Column(db.String, nullable=False) koji_tag = db.Column(db.String) # This gets set after 'wait' scmurl = db.Column(db.String) @@ -153,16 +154,19 @@ class ModuleBuild(RidaBase): ) return module - def transition(self, conf, state): + def transition(self, conf, state, state_reason = None): """ Record that a build has transitioned state. """ now = datetime.utcnow() old_state = self.state self.state = state self.time_modified = now - if self.state in ['done', 'failed']: + if INVERSE_BUILD_STATES[self.state] in ['done', 'failed']: self.time_completed = now + if INVERSE_BUILD_STATES[self.state] == "failed" and state_reason: + self.state_reason = state_reason + log.debug("%r, state %r->%r" % (self, old_state, self.state)) rida.messaging.publish( modname='rida', @@ -201,6 +205,7 @@ class ModuleBuild(RidaBase): 'release': self.release, 'state': self.state, 'state_name': INVERSE_BUILD_STATES[self.state], + 'state_reason': self.state_reason, 'scmurl': self.scmurl, 'owner': self.owner, 'time_submitted': self.time_submitted, @@ -229,6 +234,7 @@ class ModuleBuild(RidaBase): return { "id": self.id, "state": self.state, + 'state_reason': self.state_reason, "owner": self.owner, "name": self.name, "time_submitted": self._utc_datetime_to_iso(self.time_submitted), @@ -265,6 +271,8 @@ class ComponentBuild(RidaBase): task_id = db.Column(db.Integer) # This is the id of the build in koji # XXX: Consider making this a proper ENUM (or an int) state = db.Column(db.Integer) + # Reason why the build failed + state_reason = db.Column(db.String) # This stays as None until the build completes. nvr = db.Column(db.String) @@ -292,6 +300,7 @@ class ComponentBuild(RidaBase): 'format': self.format, 'task_id': self.task_id, 'state': self.state, + 'state_reason': self.state_reason, 'module_build': self.module_id, } @@ -306,5 +315,5 @@ class ComponentBuild(RidaBase): return retval def __repr__(self): - return "" % ( - self.package, self.module_id, self.state, self.task_id, self.batch) + return "" % ( + self.package, self.module_id, self.state, self.task_id, self.batch, self.state_reason) diff --git a/rida/scheduler/handlers/components.py b/rida/scheduler/handlers/components.py index d391e584..c3410089 100644 --- a/rida/scheduler/handlers/components.py +++ b/rida/scheduler/handlers/components.py @@ -50,9 +50,15 @@ def _finalize(config, session, msg, state): log.debug("We have no record of %s" % nvr) return + if state != koji.BUILD_STATES['COMPLETE']: + state_reason = "Failed to build artifact %s in Koji" % (msg.build_name) + else: + state_reason = "" + # Mark the state in the db. component_build.state = state component_build.nvr = nvr + component_build.state_reason = state_reason session.commit() parent = component_build.module_build @@ -60,7 +66,8 @@ def _finalize(config, session, msg, state): if component_build.package == 'module-build-macros': if state != koji.BUILD_STATES['COMPLETE']: # If the macro build failed, then the module is doomed. - parent.transition(config, state=models.BUILD_STATES['failed']) + parent.transition(config, state=models.BUILD_STATES['failed'], + state_reason) session.commit() return @@ -86,7 +93,7 @@ def _finalize(config, session, msg, state): # to a next batch. This module build is doomed. if all([c.state != koji.BUILD_STATES['COMPLETE'] for c in current_batch]): # They didn't all succeed.. so mark this module build as a failure. - parent.transition(config, models.BUILD_STATES['failed']) + parent.transition(config, models.BUILD_STATES['failed'], state_reason) session.commit() return diff --git a/rida/scheduler/handlers/modules.py b/rida/scheduler/handlers/modules.py index 41e073a7..b9d948f1 100644 --- a/rida/scheduler/handlers/modules.py +++ b/rida/scheduler/handlers/modules.py @@ -130,13 +130,14 @@ def wait(config, session, msg): artifact_name = "module-build-macros" state = koji.BUILD_STATES['BUILDING'] # Default state + state_reason = "" task_id = builder.build(artifact_name=artifact_name, source=srpm) # Fail task if we failed to submit it to koji # This typically happens when koji auth failed if not task_id: state = koji.BUILD_STATES['FAILED'] - # TODO: set fail_reason to "Failed to submit build" + state_reason = "Failed to submit artifact %s to Koji" % (artifact_name) component_build = models.ComponentBuild( module_id=build.id, @@ -145,6 +146,7 @@ def wait(config, session, msg): scmurl=srpm, task_id=task_id, state=state, + state_reason = state_reason, batch=1, ) session.add(component_build) diff --git a/rida/scheduler/handlers/repos.py b/rida/scheduler/handlers/repos.py index da6f8e45..ed6f9810 100644 --- a/rida/scheduler/handlers/repos.py +++ b/rida/scheduler/handlers/repos.py @@ -99,7 +99,7 @@ def done(config, session, msg): if c.state != koji.BUILD_STATES['COMPLETE'] ] if leftover_components: - rida.utils.start_next_build_batch( + rida.utils.start_next_build_batch(config, module_build, session, builder, components=leftover_components) else: module_build.transition(config, state=models.BUILD_STATES['done']) diff --git a/rida/utils.py b/rida/utils.py index ac368a5d..2f8efea6 100644 --- a/rida/utils.py +++ b/rida/utils.py @@ -51,7 +51,7 @@ def retry(timeout=120, interval=30, wait_on=Exception): return wrapper -def start_next_build_batch(module, session, builder, components=None): +def start_next_build_batch(config, module, session, builder, components=None): """ Starts a next round of the build cycle for a module. """ import koji # Placed here to avoid py2/py3 conflicts... @@ -71,11 +71,14 @@ def start_next_build_batch(module, session, builder, components=None): for c in unbuilt_components: c.batch = module.batch c.task_id = builder.build(artifact_name=c.package, source=c.scmurl) - # Fail task if we failed to submit it to koji - # This typically happens when koji auth failed + if not c.task_id: - c.state = koji.BUILD_STATES['FAILED'] - # TODO: set c.fail_reason to "Failed to submit build" + c.state = koji.BUILD_STATES["FAILED"] + c.state_reason = "Failed to submit to Koji" + module.transition(config, models.BUILD_STATES["failed"], + "Failed to submit artifact %s to Koji" % (c.package)) + session.add(module) + break session.commit() diff --git a/rida/views.py b/rida/views.py index 54fcc0ab..0e962355 100644 --- a/rida/views.py +++ b/rida/views.py @@ -158,6 +158,10 @@ class ModuleBuildAPI(MethodView): # the availability of git URLs paralelly later. full_urls = [] + # List of (pkg_name, git_url) tuples to be used to check + # the availability of git URLs paralelly later. + full_urls = [] + for pkgname, pkg in mmd.components.rpms.packages.items(): try: if pkg.get("repository") and not conf.rpms_allow_repository: diff --git a/tests/test_scheduler/test_repo_done.py b/tests/test_scheduler/test_repo_done.py index 1de319b7..9ae2f2c5 100644 --- a/tests/test_scheduler/test_repo_done.py +++ b/tests/test_scheduler/test_repo_done.py @@ -25,6 +25,7 @@ import mock import rida.messaging import rida.scheduler.handlers.repos +import rida.models class TestRepoDone(unittest.TestCase): @@ -78,3 +79,41 @@ class TestRepoDone(unittest.TestCase): 'no matches for this...', '2016-some-guid') self.fn(config=self.config, session=self.session, msg=msg) build_fn.assert_called_once_with(artifact_name='foo', source='full_scm_url') + + + @mock.patch('rida.builder.KojiModuleBuilder.buildroot_ready') + @mock.patch('rida.builder.KojiModuleBuilder.get_session_from_config') + @mock.patch('rida.builder.KojiModuleBuilder.build') + @mock.patch('rida.builder.KojiModuleBuilder.buildroot_connect') + @mock.patch('rida.models.ModuleBuild.from_repo_done_event') + def test_a_single_match_build_fail(self, from_repo_done_event, connect, build_fn, config, ready): + """ Test that when a KojiModuleBuilder.build fails, the build is + marked as failed with proper state_reason. + """ + config.return_value = mock.Mock(), "development" + unbuilt_component_build = mock.Mock() + unbuilt_component_build.package = 'foo' + unbuilt_component_build.scmurl = 'full_scm_url' + unbuilt_component_build.state = None + built_component_build = mock.Mock() + built_component_build.package = 'foo2' + built_component_build.scmurl = 'full_scm_url' + built_component_build.state = 1 + module_build = mock.Mock() + module_build.batch = 1 + module_build.component_builds = [unbuilt_component_build, built_component_build] + module_build.current_batch.return_value = [built_component_build] + build_fn.return_value = None + + from_repo_done_event.return_value = module_build + + ready.return_value = True + + msg = rida.messaging.KojiRepoChange( + 'no matches for this...', '2016-some-guid') + self.fn(config=self.config, session=self.session, msg=msg) + build_fn.assert_called_once_with(artifact_name='foo', source='full_scm_url') + module_build.transition.assert_called_once_with(self.config, + rida.models.BUILD_STATES["failed"], + 'Failed to submit artifact foo to Koji') + self.assertEquals(unbuilt_component_build.state_reason, "Failed to submit to Koji") diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 8a0c2040..efe2e25c 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -43,6 +43,7 @@ class TestViews(unittest.TestCase): self.assertEquals(data['name'], 'nginx') self.assertEquals(data['owner'], 'Moe Szyslak') self.assertEquals(data['state'], 3) + self.assertEquals(data['state_reason'], None) self.assertEquals(data['tasks'], { 'rpms/module-build-macros': '12312321/1', 'rpms/nginx': '12312345/1'}