From 6bfd9463493d4893f88282b90ad1a72c418eab88 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Sep 29 2017 09:15:08 +0000 Subject: Set the build(s) to FAILED state in case of error. --- diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index 1f614a6..fc45be8 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -232,20 +232,23 @@ class ContainerBuildHandler(BaseHandler): :return: Koji build id. """ if build.state != ArtifactBuildState.PLANNED.value: - log.error("Trying to build %r container image, " - "but build it is not in PLANNED state", build) + build.transition( + ArtifactBuildState.FAILED.value, + "Container image build is not in PLANNED state.") return if not build.build_args: - log.error("Cannot rebuild container image %r, build_args not " - "defined", build) + build.transition( + ArtifactBuildState.FAILED.value, + "Container image does not have 'build_args' filled in.") return args = json.loads(build.build_args) if not args["parent"]: # TODO: Rebuild base image. - log.error("Base image %r should be rebuild, but this is not " - "supported yet", build) + build.transition( + ArtifactBuildState.FAILED.value, + "Rebuild of container base image is not supported yet.") return scm_url = "%s/%s#%s" % (conf.git_base_url, args["repository"], @@ -298,6 +301,13 @@ class ContainerBuildHandler(BaseHandler): for build in builds: build.build_id = self.build_image_artifact_build(build, repo_urls) - build.state = ArtifactBuildState.BUILD.value + if build.build_id: + build.transition( + ArtifactBuildState.BUILD.value, + "Building container image in Koji.") + else: + build.transition( + ArtifactBuildState.FAILED.value, + "Error while building container image in Koji.") db.session.add(build) db.session.commit() diff --git a/freshmaker/handlers/brew/container_task_state_change.py b/freshmaker/handlers/brew/container_task_state_change.py index 999bfce..4eae46e 100644 --- a/freshmaker/handlers/brew/container_task_state_change.py +++ b/freshmaker/handlers/brew/container_task_state_change.py @@ -49,9 +49,13 @@ class BrewContainerTaskStateChangeHandler(ContainerBuildHandler): if found_build is not None: # update build state in db if event.new_state == 'CLOSED': - found_build.state = ArtifactBuildState.DONE.value + found_build.transition( + ArtifactBuildState.DONE.value, + "Built successfully.") if event.new_state == 'FAILED': - found_build.state = ArtifactBuildState.FAILED.value + found_build.transition( + ArtifactBuildState.FAILED.value, + "Failed to build in Koji.") db.session.commit() if found_build.state == ArtifactBuildState.DONE.value: diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index ad19a0a..8204df5 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -178,15 +178,19 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): source = self._get_compose_source(nvr) if compose_source and compose_source != source: # TODO: Handle this by generating two ODCS composes - log.error("Packages for errata advisory %d found in multiple " - "different tags", errata_id) + db_event.builds_transition( + ArtifactBuildState.FAILED.value, "Packages for errata " + "advisory %d found in multiple different tags." + % (errata_id)) return else: compose_source = source if compose_source is None: - log.error('None of builds %s of advisory %d is the latest build in' - ' its candidate tag.', builds, errata_id) + db_event.builds_transition( + ArtifactBuildState.FAILED.value, 'None of builds %s of ' + 'advisory %d is the latest build in its candidate tag.' + % (builds, errata_id)) return log.info('Generate new compose for rebuild: ' diff --git a/freshmaker/migrations/versions/3f56425964cf_.py b/freshmaker/migrations/versions/3f56425964cf_.py new file mode 100644 index 0000000..a13299f --- /dev/null +++ b/freshmaker/migrations/versions/3f56425964cf_.py @@ -0,0 +1,22 @@ +"""Add state_reason to artifact_builds table. + +Revision ID: 3f56425964cf +Revises: 300b86758bb1 +Create Date: 2017-09-20 11:38:18.176512 + +""" + +# revision identifiers, used by Alembic. +revision = '3f56425964cf' +down_revision = '300b86758bb1' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('artifact_builds', sa.Column('state_reason', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('artifact_builds', 'state_reason') diff --git a/freshmaker/models.py b/freshmaker/models.py index f4de93b..7d6290d 100644 --- a/freshmaker/models.py +++ b/freshmaker/models.py @@ -27,7 +27,7 @@ from datetime import datetime from sqlalchemy.orm import (validates, relationship) -from freshmaker import db +from freshmaker import db, log from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.events import ( MBSModuleStateChangeEvent, GitModuleMetadataChangeEvent, @@ -125,6 +125,21 @@ class Event(FreshmakerBase): id=dep.event_dependency_id).first()) return events + def has_all_builds_in_state(self, state): + """ + Returns True when all builds are in the given `state`. + """ + return db.session.query(ArtifactBuild).filter_by( + event_id=self.id).filter(state != state).count() == 0 + + def builds_transition(self, state, reason): + """ + Calls transition(state, reason) for all builds associated whit this + event. + """ + for build in self.builds: + build.transition(state, reason) + def __repr__(self): return "" % (self.message_id, self.event_type, self.search_key) @@ -151,6 +166,7 @@ class ArtifactBuild(FreshmakerBase): name = db.Column(db.String, nullable=False) type = db.Column(db.Integer) state = db.Column(db.Integer, nullable=False) + state_reason = db.Column(db.String, nullable=True) time_submitted = db.Column(db.DateTime, nullable=False) time_completed = db.Column(db.DateTime) @@ -204,6 +220,48 @@ class ArtifactBuild(FreshmakerBase): return ArtifactType[field.upper()].value raise ValueError("%s: %s, not in %r" % (key, field, list(ArtifactType))) + def depending_artifact_builds(self): + """ + Returns list of artifact builds depending on this one. + """ + return ArtifactBuild.query.filter_by(dep_on_id=self.id).all() + + def transition(self, state, state_reason): + """ + Sets the state and state_reason of this ArtifactBuild. + + :param state: ArtifactBuildState value + :param state_reason: Reason why this state has been set. + """ + + # Log the state and state_reason + if state == ArtifactBuildState.FAILED.value: + log_fnc = log.error + else: + log_fnc = log.info + log_fnc("Artifact build %r moved to state %s, %r" % ( + self, ArtifactBuildState(state).name, state_reason)) + + if self.state == state: + return + + self.state = state + self.state_reason = state_reason + if self.state in [ArtifactBuildState.DONE.value, + ArtifactBuildState.FAILED.value, + ArtifactBuildState.CANCELED.value]: + self.time_completed = datetime.utcnow() + + # For FAILED/CANCELED states, move also all the artifacts depending + # on this one to FAILED/CANCELED state, because there is no way we + # can rebuild them. + if self.state in [ArtifactBuildState.FAILED.value, + ArtifactBuildState.CANCELED.value]: + for build in self.depending_artifact_builds(): + build.transition( + self.state, "Cannot build artifact, because its " + "dependency cannot be built.") + def __repr__(self): return "" % ( self.name, ArtifactType(self.type).name, @@ -217,6 +275,7 @@ class ArtifactBuild(FreshmakerBase): "type_name": ArtifactType(self.type).name, "state": self.state, "state_name": ArtifactBuildState(self.state).name, + "state_reason": self.state_reason, "dep_on": self.dep_on.name if self.dep_on else None, "time_submitted": self.time_submitted, "time_completed": self.time_completed, diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index b9c1cea..f595f8f 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, Mock +from mock import patch, MagicMock, PropertyMock from freshmaker.handlers.errata import ErrataAdvisoryRPMsSignedHandler from freshmaker.handlers.errata import ErrataAdvisoryStateChangedHandler @@ -367,7 +367,11 @@ class TestPrepareYumRepo(unittest.TestCase): db.create_all() db.session.commit() - Event.create(db.session, 'msg-id', 'nvr', 100) + self.ev = Event.create(db.session, 'msg-id', '123', 100) + ArtifactBuild.create( + db.session, self.ev, "parent", "image", + state=ArtifactBuildState.PLANNED.value) + db.session.commit() def tearDown(self): db.session.remove() @@ -400,13 +404,11 @@ class TestPrepareYumRepo(unittest.TestCase): errata.return_value.get_builds.return_value = set(["httpd-2.4.15-1.f27"]) - event = Mock(message_id='msg-id', search_key=12345) handler = ErrataAdvisoryRPMsSignedHandler() - repo_url = handler._prepare_yum_repo(event) + repo_url = handler._prepare_yum_repo(self.ev) - rebuild_event = db.session.query(Event).filter( - Event.message_id == event.message_id).first() - self.assertEqual(3, rebuild_event.compose_id) + db.session.refresh(self.ev) + self.assertEqual(3, self.ev.compose_id) _get_compose_source.assert_called_once_with("httpd-2.4.15-1.f27") _get_packages_for_compose.assert_called_once_with("httpd-2.4.15-1.f27") @@ -420,6 +422,67 @@ class TestPrepareYumRepo(unittest.TestCase): "http://localhost/composes/latest-odcs-3-1/compose/Temporary/odcs-3.repo", repo_url) + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.ODCS') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.' + 'ErrataAdvisoryRPMsSignedHandler._get_packages_for_compose') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.' + 'ErrataAdvisoryRPMsSignedHandler._get_compose_source') + @patch('time.sleep') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') + @patch('freshmaker.handlers.BaseHandler.krb_context', + new_callable=PropertyMock) + def test_get_repo_url_packages_in_multiple_tags( + self, krb_context, errata, sleep, _get_compose_source, + _get_packages_for_compose, ODCS): + _get_packages_for_compose.return_value = ['httpd', 'httpd-debuginfo'] + _get_compose_source.side_effect = [ + 'rhel-7.2-candidate', 'rhel-7.7-candidate'] + + errata.return_value.get_builds.return_value = [ + set(["httpd-2.4.15-1.f27"]), set(["foo-2.4.15-1.f27"])] + + handler = ErrataAdvisoryRPMsSignedHandler() + repo_url = handler._prepare_yum_repo(self.ev) + + ODCS.return_value.new_compose.assert_not_called() + self.assertEqual(repo_url, None) + + db.session.refresh(self.ev) + for build in self.ev.builds: + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + self.assertEqual(build.state_reason, "Packages for errata " + "advisory 123 found in multiple different tags.") + + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.ODCS') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.' + 'ErrataAdvisoryRPMsSignedHandler._get_packages_for_compose') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.' + 'ErrataAdvisoryRPMsSignedHandler._get_compose_source') + @patch('time.sleep') + @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') + @patch('freshmaker.handlers.BaseHandler.krb_context', + new_callable=PropertyMock) + def test_get_repo_url_packages_not_found_in_tag( + self, krb_context, errata, sleep, _get_compose_source, + _get_packages_for_compose, ODCS): + _get_packages_for_compose.return_value = ['httpd', 'httpd-debuginfo'] + _get_compose_source.return_value = None + + errata.return_value.get_builds.return_value = [ + set(["httpd-2.4.15-1.f27"]), set(["foo-2.4.15-1.f27"])] + + handler = ErrataAdvisoryRPMsSignedHandler() + repo_url = handler._prepare_yum_repo(self.ev) + + ODCS.return_value.new_compose.assert_not_called() + self.assertEqual(repo_url, None) + + db.session.refresh(self.ev) + for build in self.ev.builds: + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + self.assertTrue(build.state_reason.endswith( + "of advisory 123 is the latest build in its candidate tag.")) + class TestFindEventsToInclude(unittest.TestCase): """Test ErrataAdvisoryRPMsSignedHandler._find_events_to_include""" diff --git a/tests/test_handler.py b/tests/test_handler.py index 8aaaf6d..3bf0837 100644 --- a/tests/test_handler.py +++ b/tests/test_handler.py @@ -126,11 +126,23 @@ class TestBuildFirstBatch(TestCase): state=ArtifactBuildState.PLANNED.value, dep_on=p1) b.build_args = build_args + + # Not in PLANNED state. b = ArtifactBuild.create(db.session, self.db_event, "parent3", "image", state=ArtifactBuildState.BUILD.value) b.build_args = build_args + + # No build args + b = ArtifactBuild.create(db.session, self.db_event, "parent4", "image", + state=ArtifactBuildState.PLANNED.value) db.session.commit() + # No parent - base image + b = ArtifactBuild.create(db.session, self.db_event, "parent5", "image", + state=ArtifactBuildState.PLANNED.value) + b.build_args = build_args + b.build_args = b.build_args.replace("nvr", "") + def tearDown(self): db.session.remove() db.drop_all() @@ -171,60 +183,18 @@ class TestBuildFirstBatch(TestCase): for build in self.db_event.builds: if build.name == "parent1-1-4": self.assertEqual(build.build_id, 123) + elif build.name == "parent3": + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + self.assertEqual(build.state_reason, "Container image build " + "is not in PLANNED state.") + elif build.name == "parent4": + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + self.assertEqual(build.state_reason, "Container image does " + "not have 'build_args' filled in.") + elif build.name == "parent5": + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) + self.assertEqual(build.state_reason, "Rebuild of container " + "base image is not supported yet.") else: self.assertEqual(build.build_id, None) - - @patch('freshmaker.handlers.ODCS') - @patch('koji.ClientSession') - @patch('freshmaker.handlers.krbContext') - def test_build_first_batch_extra_events(self, krb, ClientSession, ODCS): - """ - Tests that only PLANNED images without a parent are submitted to - build system. - """ - ODCS.return_value.get_compose.side_effect = [{ - "id": 3, - "result_repo": "http://localhost/composes/latest-odcs-3-1/compose/Temporary", - "result_repofile": "http://localhost/composes/latest-odcs-3-1/compose/Temporary/odcs-3.repo", - "source": "f26", - "source_type": 1, - "state": 2, - "state_name": "done", - }, { - "id": 4, - "result_repo": "http://localhost/composes/latest-odcs-4-1/compose/Temporary", - "result_repofile": "http://localhost/composes/latest-odcs-4-1/compose/Temporary/odcs-4.repo", - "source": "f26", - "source_type": 1, - "state": 2, - "state_name": "done", - }] - mock_session = ClientSession.return_value - mock_session.buildContainer.return_value = 123 - - db_event2 = Event.get_or_create( - db.session, "msg2", "current_event", ErrataAdvisoryRPMsSignedEvent, - released=False) - db_event2.compose_id = 4 - db.session.commit() - self.db_event.add_event_dependency(db.session, db_event2) - db.session.commit() - - handler = MyHandler() - handler._build_first_batch(self.db_event) - - mock_session.buildContainer.assert_called_once_with( - 'git://pkgs.fedoraproject.org/repo#hash', - 'target', - {'scratch': True, 'isolated': True, 'koji_parent_build': u'nvr', - 'git_branch': 'mybranch', 'release': AnyStringWith('4.'), - 'yum_repourls': [ - 'http://localhost/composes/latest-odcs-3-1/compose/Temporary/odcs-3.repo', - 'http://localhost/composes/latest-odcs-4-1/compose/Temporary/odcs-4.repo']}) - - db.session.refresh(self.db_event) - for build in self.db_event.builds: - if build.name == "parent1-1-4": - self.assertEqual(build.build_id, 123) - else: - self.assertEqual(build.build_id, None) + self.assertEqual(build.state, ArtifactBuildState.PLANNED.value) diff --git a/tests/test_models.py b/tests/test_models.py index 3d563cd..ef5c244 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -24,6 +24,7 @@ import unittest from freshmaker import db, events from freshmaker.models import Event, ArtifactBuild +from freshmaker.types import ArtifactBuildState class TestModels(unittest.TestCase): @@ -88,3 +89,55 @@ class TestModels(unittest.TestCase): self.assertEqual(event.event_dependencies, [event1]) self.assertEqual(event.event_dependencies[0].search_key, "test2") self.assertEqual(event1.event_dependencies, []) + + def test_depending_artifact_builds(self): + event = Event.create(db.session, "test_msg_id", "test", events.TestingEvent) + parent = ArtifactBuild.create(db.session, event, "parent", "module", 1234) + build2 = ArtifactBuild.create(db.session, event, "mksh", "module", 1235, parent) + build3 = ArtifactBuild.create(db.session, event, "runtime", "module", 1236, parent) + ArtifactBuild.create(db.session, event, "perl-runtime", "module", 1237) + db.session.commit() + + deps = set(parent.depending_artifact_builds()) + self.assertEqual(deps, set([build2, build3])) + + def test_build_transition_recursion(self): + for state in [ArtifactBuildState.FAILED.value, + ArtifactBuildState.CANCELED.value]: + event = Event.create(db.session, "test_msg_id", "test", events.TestingEvent) + build1 = ArtifactBuild.create(db.session, event, "ed", "module", 1234) + build2 = ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build1) + build3 = ArtifactBuild.create(db.session, event, "runtime", "module", 1236, build2) + build4 = ArtifactBuild.create(db.session, event, "perl-runtime", "module", 1237) + db.session.commit() + + build1.transition(state, "reason") + self.assertEqual(build1.state, state) + self.assertEqual(build1.state_reason, "reason") + + for build in [build2, build3]: + self.assertEqual(build.state, state) + self.assertEqual( + build.state_reason, "Cannot build artifact, because its " + "dependency cannot be built.") + + self.assertEqual(build4.state, ArtifactBuildState.BUILD.value) + self.assertEqual(build4.state_reason, None) + + def test_build_transition_recursion_not_done_for_ok_states(self): + for state in [ArtifactBuildState.DONE.value, + ArtifactBuildState.PLANNED.value]: + event = Event.create(db.session, "test_msg_id", "test", events.TestingEvent) + build1 = ArtifactBuild.create(db.session, event, "ed", "module", 1234) + build2 = ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build1) + build3 = ArtifactBuild.create(db.session, event, "runtime", "module", 1236, build2) + build4 = ArtifactBuild.create(db.session, event, "perl-runtime", "module", 1237) + db.session.commit() + + build1.transition(state, "reason") + self.assertEqual(build1.state, state) + self.assertEqual(build1.state_reason, "reason") + + for build in [build2, build3, build4]: + self.assertEqual(build4.state, ArtifactBuildState.BUILD.value) + self.assertEqual(build4.state_reason, None)