diff --git a/module_build_service/models.py b/module_build_service/models.py index 80854bd9..8d91c331 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -265,6 +265,9 @@ class ModuleBuild(MBSBase): # If the rebuild_strategy isn't specified, use the default rebuild_strategy=rebuild_strategy or conf.rebuild_strategy ) + # Add a state transition to "init" + mbt = ModuleBuildTrace(state_time=now, state=module.state) + module.module_builds_trace.append(mbt) session.add(module) session.commit() module_build_service.messaging.publish( diff --git a/module_build_service/pdc.py b/module_build_service/pdc.py index 9e9d2de0..b0d69f71 100644 --- a/module_build_service/pdc.py +++ b/module_build_service/pdc.py @@ -132,7 +132,7 @@ def get_variant_dict(data): result['active'] = data['active'] if not result: - raise ValueError("Couldn't get variant_dict from %s" % data) + raise RuntimeError("Couldn't get variant_dict from %s" % data) return result diff --git a/module_build_service/scheduler/handlers/modules.py b/module_build_service/scheduler/handlers/modules.py index 37b6d8aa..c8473230 100644 --- a/module_build_service/scheduler/handlers/modules.py +++ b/module_build_service/scheduler/handlers/modules.py @@ -32,7 +32,7 @@ from module_build_service.utils import ( attempt_to_reuse_all_components, record_component_builds, get_rpm_release_from_mmd) -from module_build_service.errors import UnprocessableEntity +from module_build_service.errors import UnprocessableEntity, Forbidden, ValidationError from module_build_service.builder.KojiContentGenerator import KojiContentGenerator from requests.exceptions import ConnectionError @@ -139,8 +139,17 @@ def init(config, session, msg): record_component_builds(mmd, build, session=session) build.modulemd = mmd.dumps() build.transition(conf, models.BUILD_STATES["wait"]) - except UnprocessableEntity: - build.transition(conf, models.BUILD_STATES["failed"]) + # Catch custom exceptions that we can expose to the user + except (UnprocessableEntity, Forbidden, ValidationError, RuntimeError) as e: + # Rollback changes underway + session.rollback() + build.transition(conf, models.BUILD_STATES["failed"], state_reason=str(e)) + except Exception as e: + log.exception(str(e)) + # Rollback changes underway + session.rollback() + msg = "An unknown error occurred while validating the modulemd" + build.transition(conf, models.BUILD_STATES["failed"], state_reason=msg) finally: session.add(build) session.commit() diff --git a/module_build_service/utils.py b/module_build_service/utils.py index a90e67aa..c903bd3a 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -843,14 +843,7 @@ def record_component_builds(mmd, module, initial_batch=1, # Format the modulemd by putting in defaults and replacing streams that # are branches with commit hashes - try: - format_mmd(mmd, module.scmurl, session=session) - except Exception as e: - module.transition(conf, models.BUILD_STATES["failed"], - "Failed to validate modulemd file: %s" % str(e)) - session.add(module) - session.commit() - raise + format_mmd(mmd, module.scmurl, session=session) # When main_mmd is set, merge the metadata from this mmd to main_mmd, # otherwise our current mmd is main_mmd. @@ -865,10 +858,7 @@ def record_component_builds(mmd, module, initial_batch=1, 'conflicting components: {2}' .format(mmd.name, main_mmd.name, ', '.join(duplicate_components))) - module.transition(conf, models.BUILD_STATES["failed"], error_msg) - session.add(module) - session.commit() - raise RuntimeError(error_msg) + raise UnprocessableEntity(error_msg) merge_included_mmd(main_mmd, mmd) else: main_mmd = mmd @@ -979,9 +969,15 @@ def submit_module_build(username, url, mmd, scm, optional_params=None): component.state = None db.session.add(component) module.username = username - module.transition(conf, models.BUILD_STATES["wait"], - "Resubmitted by %s" % username) - module.batch = 0 + # The last transition in the trace will be "failed", but we want to determine what the + # state was previous to the failure. + prev_state = module.module_builds_trace[-2].state + if prev_state == models.BUILD_STATES['init']: + transition_to = models.BUILD_STATES['init'] + else: + transition_to = models.BUILD_STATES['wait'] + module.batch = 0 + module.transition(conf, transition_to, "Resubmitted by %s" % username) log.info("Resumed existing module build in previous state %s" % module.state) else: diff --git a/module_build_service/views.py b/module_build_service/views.py index 8b970609..01dae1d8 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -181,6 +181,9 @@ class ModuleBuildAPI(AbstractQueryableBuildAPI): log.error('Invalid JSON submitted') raise ValidationError('Invalid JSON submitted') + if module.state == models.BUILD_STATES['failed']: + raise Forbidden('You can\'t cancel a failed module') + if r['state'] == 'failed' \ or r['state'] == str(models.BUILD_STATES['failed']): module.transition(conf, models.BUILD_STATES["failed"], diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 9a0ecb63..f1bbcaa6 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -27,7 +27,7 @@ import os from os import path, mkdir from os.path import dirname from shutil import copyfile -from datetime import datetime +from datetime import datetime, timedelta from nose.tools import timed @@ -805,6 +805,8 @@ class TestBuild(unittest.TestCase): Tests that resuming the build works even when previous batches are already built. """ + now = datetime.utcnow() + submitted_time = now - timedelta(minutes=3) # Create a module in the failed state build_one = models.ModuleBuild() build_one.name = 'testmodule' @@ -820,10 +822,21 @@ class TestBuild(unittest.TestCase): build_one.scmurl = 'git://pkgs.stg.fedoraproject.org/modules/testmodule.git?#7fea453' build_one.batch = models.BUILD_STATES['failed'] build_one.owner = 'Homer J. Simpson' - build_one.time_submitted = datetime(2017, 2, 15, 16, 8, 18) - build_one.time_modified = datetime(2017, 2, 15, 16, 19, 35) - build_one.time_completed = datetime(2017, 2, 15, 16, 19, 35) + build_one.time_submitted = submitted_time + build_one.time_modified = now build_one.rebuild_strategy = 'changed-and-after' + # It went from init, to wait, to build, and then failed + mbt_one = models.ModuleBuildTrace( + state_time=submitted_time, state=models.BUILD_STATES['init']) + mbt_two = models.ModuleBuildTrace( + state_time=now - timedelta(minutes=2), state=models.BUILD_STATES['wait']) + mbt_three = models.ModuleBuildTrace( + state_time=now - timedelta(minutes=1), state=models.BUILD_STATES['build']) + mbt_four = models.ModuleBuildTrace(state_time=now, state=build_one.state) + build_one.module_builds_trace.append(mbt_one) + build_one.module_builds_trace.append(mbt_two) + build_one.module_builds_trace.append(mbt_three) + build_one.module_builds_trace.append(mbt_four) # Successful component component_one = models.ComponentBuild() component_one.package = 'perl-Tangerine' @@ -850,7 +863,7 @@ class TestBuild(unittest.TestCase): component_three.batch = 3 component_three.module_id = 1 - db.session.add(build_one), + db.session.add(build_one) db.session.add(component_one) db.session.add(component_two) db.session.add(component_three) @@ -866,14 +879,14 @@ class TestBuild(unittest.TestCase): data = json.loads(rv.data) module_build_id = data['id'] - FakeModuleBuilder.BUILD_STATE = "BUILDING" + FakeModuleBuilder.BUILD_STATE = 'BUILDING' FakeModuleBuilder.INSTANT_COMPLETE = True module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() components = models.ComponentBuild.query.filter_by( - module_id=module_build_id, batch=2).order_by(models.ComponentBuild.id) + module_id=module_build_id, batch=2).order_by(models.ComponentBuild.id).all() # Make sure the build went from failed to wait - self.assertEqual(module_build.state, models.BUILD_STATES["wait"]) + self.assertEqual(module_build.state, models.BUILD_STATES['wait']) self.assertEqual(module_build.state_reason, 'Resubmitted by Homer J. Simpson') # Make sure the state was reset on the failed component self.assertIsNone(components[1].state) @@ -888,8 +901,80 @@ class TestBuild(unittest.TestCase): # or "ready" state. for build in models.ComponentBuild.query.filter_by(module_id=module_build_id).all(): self.assertEqual(build.state, koji.BUILD_STATES['COMPLETE']) - self.assertTrue(build.module_build.state in [models.BUILD_STATES["done"], - models.BUILD_STATES["ready"]]) + self.assertTrue(build.module_build.state in [models.BUILD_STATES['done'], + models.BUILD_STATES['ready']]) + + @timed(60) + @patch('module_build_service.auth.get_user', return_value=user) + @patch('module_build_service.scm.SCM') + def test_submit_build_resume_failed_init(self, mocked_scm, mocked_get_user, conf_system, dbg): + """ + Tests that resuming the build works when the build failed during the init step + """ + now = datetime.utcnow() + submitted_time = now - timedelta(minutes=3) + # Create a module in the failed state + build_one = models.ModuleBuild() + build_one.name = 'testmodule' + build_one.stream = 'master' + build_one.version = 1 + build_one.state = models.BUILD_STATES['failed'] + current_dir = os.path.dirname(__file__) + formatted_testmodule_yml_path = os.path.join( + current_dir, '..', 'staged_data', 'formatted_testmodule.yaml') + with open(formatted_testmodule_yml_path, 'r') as f: + build_one.modulemd = f.read() + build_one.koji_tag = 'module-testmodule-master-1' + build_one.scmurl = 'git://pkgs.stg.fedoraproject.org/modules/testmodule.git?#7fea453' + build_one.batch = models.BUILD_STATES['failed'] + build_one.owner = 'Homer J. Simpson' + build_one.time_submitted = submitted_time + build_one.time_modified = now + build_one.rebuild_strategy = 'changed-and-after' + # This module failed during init + mbt_one = models.ModuleBuildTrace( + state_time=submitted_time, state=models.BUILD_STATES['init']) + mbt_two = models.ModuleBuildTrace(state_time=now, state=build_one.state) + build_one.module_builds_trace.append(mbt_one) + build_one.module_builds_trace.append(mbt_two) + + db.session.add(build_one) + db.session.commit() + db.session.expire_all() + + FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', '7fea453') + # Resubmit the failed module + rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( + {'branch': 'master', 'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/' + 'testmodule.git?#7fea453'})) + + data = json.loads(rv.data) + module_build_id = data['id'] + + FakeModuleBuilder.BUILD_STATE = 'BUILDING' + FakeModuleBuilder.INSTANT_COMPLETE = True + + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + components = models.ComponentBuild.query.filter_by( + module_id=module_build_id, batch=2).order_by(models.ComponentBuild.id).all() + # Make sure the build went from failed to init + self.assertEqual(module_build.state, models.BUILD_STATES['init']) + self.assertEqual(module_build.state_reason, 'Resubmitted by Homer J. Simpson') + # Make sure there are no components + self.assertEqual(components, []) + db.session.expire_all() + + # Run the backend + msgs = [] + stop = module_build_service.scheduler.make_simple_stop_condition(db.session) + module_build_service.scheduler.main(msgs, stop) + + # All components should be built and module itself should be in "done" + # or "ready" state. + for build in models.ComponentBuild.query.filter_by(module_id=module_build_id).all(): + self.assertEqual(build.state, koji.BUILD_STATES['COMPLETE']) + self.assertTrue(build.module_build.state in [models.BUILD_STATES['done'], + models.BUILD_STATES['ready']]) @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') diff --git a/tests/test_scheduler/test_module_init.py b/tests/test_scheduler/test_module_init.py index 56d41675..09c7be15 100644 --- a/tests/test_scheduler/test_module_init.py +++ b/tests/test_scheduler/test_module_init.py @@ -147,4 +147,4 @@ class TestModuleInit(unittest.TestCase): session.refresh(build) # Make sure the module entered the failed state assert build.state == 4, build.state - assert 'Failed to validate modulemd file' in build.state_reason + assert 'Failed to get the latest commit for' in build.state_reason diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index 3db26563..7093f960 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -21,13 +21,14 @@ import unittest from os import path, mkdir from shutil import copyfile +from datetime import datetime import vcr import modulemd from mock import patch import module_build_service.utils import module_build_service.scm from module_build_service import models, conf -from module_build_service.errors import ProgrammingError, ValidationError +from module_build_service.errors import ProgrammingError, ValidationError, UnprocessableEntity from tests import (test_reuse_component_init_data, init_data, db, test_reuse_shared_userspace_init_data) import mock @@ -588,6 +589,19 @@ class TestUtils(unittest.TestCase): module_build.state = models.BUILD_STATES['failed'] module_build.state_reason = "Cancelled" module_build.version = 1 + now = datetime.utcnow() + mbt_one = models.ModuleBuildTrace( + state_time=now, state=models.BUILD_STATES['init']) + mbt_two = models.ModuleBuildTrace( + state_time=now, state=models.BUILD_STATES['wait']) + mbt_three = models.ModuleBuildTrace( + state_time=now, state=models.BUILD_STATES['build']) + mbt_four = models.ModuleBuildTrace( + state_time=now, state=models.BUILD_STATES['failed']) + module_build.module_builds_trace.append(mbt_one) + module_build.module_builds_trace.append(mbt_two) + module_build.module_builds_trace.append(mbt_three) + module_build.module_builds_trace.append(mbt_four) # Mark the components as COMPLETE/FAILED/CANCELED components = module_build.component_builds @@ -646,13 +660,10 @@ class TestUtils(unittest.TestCase): try: module_build_service.utils.record_component_builds( testmodule_variant_mmd, module_build, main_mmd=mmd) - assert False, 'A RuntimeError was expected but was not raised' - except RuntimeError as e: + assert False, 'A UnprocessableEntity was expected but was not raised' + except UnprocessableEntity as e: self.assertEqual(e.message, error_msg) - self.assertEqual(module_build.state, models.BUILD_STATES['failed']) - self.assertEqual(module_build.state_reason, error_msg) - class DummyModuleBuilder(GenericBuilder): """ diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 80850c8e..1fb909e0 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -499,7 +499,8 @@ class TestViews(unittest.TestCase): self.assertEquals(data['rebuild_strategy'], 'changed-and-after') self.assertEquals(data['state_name'], 'init') self.assertEquals(data['state_url'], '/module-build-service/1/module-builds/31') - self.assertEquals(data['state_trace'], []) + self.assertEquals(len(data['state_trace']), 1) + self.assertEquals(data['state_trace'][0]['state'], 0) self.assertDictEqual(data['tasks'], {}) mmd = _modulemd.ModuleMetadata() mmd.loads(data["modulemd"]) @@ -662,6 +663,19 @@ class TestViews(unittest.TestCase): self.assertEquals(data['state'], 4) self.assertEquals(data['state_reason'], 'Canceled by some_other_user.') + @patch('module_build_service.auth.get_user', return_value=other_user) + def test_cancel_build_already_failed(self, mocked_get_user): + module = ModuleBuild.query.filter_by(id=30).one() + module.state = 4 + db.session.add(module) + db.session.commit() + rv = self.client.patch('/module-build-service/1/module-builds/30', + data=json.dumps({'state': 'failed'})) + data = json.loads(rv.data) + + self.assertEquals(data['status'], 403) + self.assertEquals(data['error'], 'Forbidden') + @patch('module_build_service.auth.get_user', return_value=('sammy', set())) def test_cancel_build_unauthorized_no_groups(self, mocked_get_user): rv = self.client.patch('/module-build-service/1/module-builds/30',