From 93d79e777e4b433b93aab50e03b01d58c37ea061 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 18 Oct 2017 16:03:38 -0400 Subject: [PATCH] Automatically disable build triggers with successive failures or internal errors We allow users to reenable them manually once disabled --- buildman/server.py | 7 ++- config.py | 8 ++++ data/database.py | 4 +- ...add_automatic_disable_of_build_triggers.py | 45 ++++++++++++++++++ data/model/build.py | 41 ++++++++++++++++- data/model/test/test_build.py | 46 +++++++++++++++++++ endpoints/api/test/test_security.py | 3 -- initdb.py | 2 + .../repo-view/repo-panel-builds.html | 21 +++++++-- 9 files changed, 166 insertions(+), 11 deletions(-) create mode 100644 data/migrations/versions/17aff2e1354e_add_automatic_disable_of_build_triggers.py create mode 100644 data/model/test/test_build.py diff --git a/buildman/server.py b/buildman/server.py index 13dd20444..466eee9b7 100644 --- a/buildman/server.py +++ b/buildman/server.py @@ -14,7 +14,7 @@ from flask import Flask from buildman.enums import BuildJobResult, BuildServerStatus, RESULT_PHASES from buildman.jobutil.buildstatus import StatusHandler from buildman.jobutil.buildjob import BuildJob, BuildJobLoadException -from data import database +from data import database, model from app import app, metric_queue logger = logging.getLogger(__name__) @@ -151,6 +151,11 @@ class BuilderServer(object): else: self._queue.complete(build_job.job_item) + # Update the trigger failure tracking (if applicable). + if build_job.repo_build.trigger is not None: + model.build.update_trigger_disable_status(build_job.repo_build.trigger, + RESULT_PHASES[job_status]) + if update_phase: status_handler = StatusHandler(self._build_logs, build_job.repo_build.uuid) yield From(status_handler.set_phase(RESULT_PHASES[job_status])) diff --git a/config.py b/config.py index 4fcb7f336..369c850c2 100644 --- a/config.py +++ b/config.py @@ -519,3 +519,11 @@ class DefaultConfig(ImmutableConfig): 'engine': 'memcached', 'endpoint': ('127.0.0.1', 18080), } + + # Defines the number of successive failures of a build trigger's build before the trigger is + # automatically disabled. + SUCCESSIVE_TRIGGER_FAILURE_DISABLE_THRESHOLD = 100 + + # Defines the number of successive internal errors of a build trigger's build before the + # trigger is automatically disabled. + SUCCESSIVE_TRIGGER_INTERNAL_ERROR_DISABLE_THRESHOLD = 5 diff --git a/data/database.py b/data/database.py index 6e41a9307..cc8def8b2 100644 --- a/data/database.py +++ b/data/database.py @@ -710,7 +710,9 @@ class RepositoryBuildTrigger(BaseModel): pull_robot = QuayUserField(allows_robots=True, null=True, related_name='triggerpullrobot', robot_null_delete=True) enabled = BooleanField(default=True) - disabled_reason = ForeignKeyField(DisableReason, null=True) + disabled_reason = EnumField(DisableReason, null=True) + successive_failure_count = IntegerField(default=0) + successive_internal_error_count = IntegerField(default=0) class EmailConfirmation(BaseModel): diff --git a/data/migrations/versions/17aff2e1354e_add_automatic_disable_of_build_triggers.py b/data/migrations/versions/17aff2e1354e_add_automatic_disable_of_build_triggers.py new file mode 100644 index 000000000..ca71dd73a --- /dev/null +++ b/data/migrations/versions/17aff2e1354e_add_automatic_disable_of_build_triggers.py @@ -0,0 +1,45 @@ +"""Add automatic disable of build triggers + +Revision ID: 17aff2e1354e +Revises: 61cadbacb9fc +Create Date: 2017-10-18 15:58:03.971526 + +""" + +# revision identifiers, used by Alembic. +revision = '17aff2e1354e' +down_revision = '61cadbacb9fc' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import mysql + +def upgrade(tables): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('repositorybuildtrigger', sa.Column('successive_failure_count', sa.Integer(), nullable=False)) + op.add_column('repositorybuildtrigger', sa.Column('successive_internal_error_count', sa.Integer(), nullable=False)) + # ### end Alembic commands ### + + op.bulk_insert( + tables.disablereason, + [ + {'id': 2, 'name': 'successive_build_failures'}, + {'id': 3, 'name': 'successive_build_internal_errors'}, + ], + ) + +def downgrade(tables): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('repositorybuildtrigger', 'successive_internal_error_count') + op.drop_column('repositorybuildtrigger', 'successive_failure_count') + # ### end Alembic commands ### + + op.execute(tables + .disablereason + .delete() + .where(tables.disablereason.c.name == op.inline_literal('successive_internal_error_count'))) + + op.execute(tables + .disablereason + .delete() + .where(tables.disablereason.c.name == op.inline_literal('successive_failure_count'))) diff --git a/data/model/build.py b/data/model/build.py index 5d491389a..9d844e665 100644 --- a/data/model/build.py +++ b/data/model/build.py @@ -263,6 +263,45 @@ def toggle_build_trigger(trigger, enabled, reason='user_toggled'): trigger.enabled = enabled if not enabled: - trigger.disabled_reason = DisableReason.get(name=reason) + trigger.disabled_reason = RepositoryBuildTrigger.disabled_reason.get_id(reason) trigger.save() + + +def update_trigger_disable_status(trigger, final_phase): + """ Updates the disable status of the given build trigger. If the build trigger had a + failure, then the counter is increased and, if we've reached the limit, the trigger is + automatically disabled. Otherwise, if the trigger succeeded, it's counter is reset. This + ensures that triggers that continue to error are eventually automatically disabled. + """ + with db_transaction(): + # If the build completed successfully, then reset the successive counters. + if final_phase == BUILD_PHASE.COMPLETE: + trigger.successive_failure_count = 0 + trigger.successive_internal_error_count = 0 + trigger.save() + return + + # Otherwise, increment the counters and check for trigger disable. + if final_phase == BUILD_PHASE.ERROR: + trigger.successive_failure_count = trigger.successive_failure_count + 1 + trigger.successive_internal_error_count = 0 + elif final_phase == BUILD_PHASE.INTERNAL_ERROR: + trigger.successive_failure_count = 0 + trigger.successive_internal_error_count = trigger.successive_internal_error_count + 1 + + # Check if we need to disable the trigger. + failure_threshold = config.app_config.get('SUCCESSIVE_TRIGGER_FAILURE_DISABLE_THRESHOLD') + error_threshold = config.app_config.get('SUCCESSIVE_TRIGGER_INTERNAL_ERROR_DISABLE_THRESHOLD') + + if failure_threshold and trigger.successive_failure_count >= failure_threshold: + trigger.enabled = False + trigger.disabled_reason = RepositoryBuildTrigger.disabled_reason.get_id('successive_build_failures') + elif (error_threshold and + trigger.successive_internal_error_count >= error_threshold): + trigger.enabled = False + trigger.disabled_reason = RepositoryBuildTrigger.disabled_reason.get_id('successive_build_internal_errors') + + # Save the trigger changes. + trigger.save() + \ No newline at end of file diff --git a/data/model/test/test_build.py b/data/model/test/test_build.py new file mode 100644 index 000000000..17b15c6b5 --- /dev/null +++ b/data/model/test/test_build.py @@ -0,0 +1,46 @@ +import pytest + +from mock import patch + +from data.database import BUILD_PHASE, RepositoryBuildTrigger +from data.model.build import update_trigger_disable_status +from test.fixtures import * + +TEST_FAIL_THRESHOLD = 5 +TEST_INTERNAL_ERROR_THRESHOLD = 2 + +@pytest.mark.parametrize('starting_failure_count, starting_error_count, status, expected_reason', [ + (0, 0, BUILD_PHASE.COMPLETE, None), + (10, 10, BUILD_PHASE.COMPLETE, None), + + (TEST_FAIL_THRESHOLD - 1, TEST_INTERNAL_ERROR_THRESHOLD - 1, BUILD_PHASE.COMPLETE, None), + (TEST_FAIL_THRESHOLD - 1, 0, BUILD_PHASE.ERROR, 'successive_build_failures'), + (0, TEST_INTERNAL_ERROR_THRESHOLD - 1, BUILD_PHASE.INTERNAL_ERROR, + 'successive_build_internal_errors'), +]) +def test_update_trigger_disable_status(starting_failure_count, starting_error_count, status, + expected_reason, initialized_db): + test_config = { + 'SUCCESSIVE_TRIGGER_FAILURE_DISABLE_THRESHOLD': TEST_FAIL_THRESHOLD, + 'SUCCESSIVE_TRIGGER_INTERNAL_ERROR_DISABLE_THRESHOLD': TEST_INTERNAL_ERROR_THRESHOLD, + } + + trigger = model.build.list_build_triggers('devtable', 'building')[0] + trigger.successive_failure_count = starting_failure_count + trigger.successive_internal_error_count = starting_error_count + trigger.enabled = True + trigger.save() + + with patch('data.model.config.app_config', test_config): + update_trigger_disable_status(trigger, status) + updated_trigger = RepositoryBuildTrigger.get(uuid=trigger.uuid) + + assert updated_trigger.enabled == (expected_reason is None) + + if expected_reason is not None: + assert updated_trigger.disabled_reason.name == expected_reason + else: + assert updated_trigger.disabled_reason is None + assert updated_trigger.successive_failure_count == 0 + assert updated_trigger.successive_internal_error_count == 0 + \ No newline at end of file diff --git a/endpoints/api/test/test_security.py b/endpoints/api/test/test_security.py index 9d24b0367..863a78f2e 100644 --- a/endpoints/api/test/test_security.py +++ b/endpoints/api/test/test_security.py @@ -13,11 +13,8 @@ from endpoints.api.signing import RepositorySignatures from endpoints.api.search import ConductRepositorySearch from endpoints.api.superuser import SuperUserRepositoryBuildLogs, SuperUserRepositoryBuildResource from endpoints.api.superuser import SuperUserRepositoryBuildStatus -<<<<<<< HEAD from endpoints.api.appspecifictokens import AppTokens, AppToken -======= from endpoints.api.trigger import BuildTrigger ->>>>>>> Add ability for triggers to be disabled from endpoints.test.shared import client_with_identity, toggle_feature from test.fixtures import * diff --git a/initdb.py b/initdb.py index e42d599e4..378a22e93 100644 --- a/initdb.py +++ b/initdb.py @@ -436,6 +436,8 @@ def initialize_database(): TagKind.create(name='channel') DisableReason.create(name='user_toggled') + DisableReason.create(name='successive_build_failures') + DisableReason.create(name='successive_build_internal_errors') def wipe_database(): diff --git a/static/directives/repo-view/repo-panel-builds.html b/static/directives/repo-view/repo-panel-builds.html index 1e7577ffc..91961fff7 100644 --- a/static/directives/repo-view/repo-panel-builds.html +++ b/static/directives/repo-view/repo-panel-builds.html @@ -169,16 +169,27 @@ - + - This build trigger is currently disabled and will not build: + This build trigger is user disabled and will not build: + Re-enable this trigger + + + + This build trigger was automatically disabled due to successive failures: + Re-enable this trigger + + + + This build trigger was automatically disabled due to successive internal errors: Re-enable this trigger - + - -