#371 Support for event canceling
Merged a month ago by fivaldi. Opened a month ago by fivaldi.
fivaldi/freshmaker fivaldi_canceling_events  into  master

file modified
+29

@@ -381,3 +381,32 @@ 

          super(FreshmakerManualRebuildEvent, self).__init__(

              msg_id, dry_run=dry_run)

          self.errata_id = errata_id

+ 

+ 

+ class FreshmakerManageEvent(BaseEvent):

+     """

+     Event triggered by an internal message for managing Freshmaker itself.

+     """

+     _max_tries = 3

+ 

+     def __init__(self, msg_body, **kwargs):

+         super(FreshmakerManageEvent, self).__init__(None, manual=True, **kwargs)

+         self.body = msg_body

+ 

+     def __new__(cls, msg_body, *args, **kwargs):

+         # The intention here is to balance control over retries. We want

+         # to allow handlers to implement their own logic depending on

+         # `last_try`, when they *SHALL* return an empty list. But, we also

+         # want to avoid endless loops and guarantee some higher control. If

+         # handler(s) don't stop their tries (by returning new events),

+         # then the unhandleable `None` is returned here as last resort,

+         # instead of `FreshmakerManageEvent`.

+         instance = super(FreshmakerManageEvent, cls).__new__(cls)

+         instance.action = msg_body['action']

+         instance.try_count = msg_body['try']

+         instance.try_count += 1

+         instance.last_try = instance.try_count == FreshmakerManageEvent._max_tries

+ 

+         if instance.try_count > FreshmakerManageEvent._max_tries:

+             return None

+         return instance

@@ -23,3 +23,4 @@ 

  from .update_db_on_module_build import UpdateDBOnModuleBuild  # noqa

  from .generate_advisory_signed_event_on_rpm_sign import GenerateAdvisorySignedEventOnRPMSign  # noqa

  from .update_db_on_odcs_compose_fail import UpdateDBOnODCSComposeFail  # noqa

+ from .cancel_event_on_freshmaker_manage_request import CancelEventOnFreshmakerManageRequest  # noqa

@@ -0,0 +1,74 @@ 

+ # -*- coding: utf-8 -*-

+ # Copyright (c) 2016  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ #

+ # Written by Filip Valder <fvalder@redhat.com>

+ 

+ from freshmaker import conf, log, db

+ from freshmaker.models import ArtifactBuild

+ from freshmaker.handlers import BaseHandler

+ from freshmaker.events import FreshmakerManageEvent

+ from freshmaker.kojiservice import koji_service

+ 

+ 

+ class CancelEventOnFreshmakerManageRequest(BaseHandler):

+     name = "CancelEventOnFreshmakerManageRequest"

+     order = 0

+ 

+     def can_handle(self, event):

+         if isinstance(event, FreshmakerManageEvent) and event.action == 'eventcancel':

+             return True

+ 

+         return False

+ 

+     def handle(self, event):

+         """

+         Handle Freshmaker manage request to cancel actions triggered by

+         event, given by event_id in the event.body. This especially

+         means to cancel running Koji builds. If some of the builds

+         couldn't be canceled for some reason, there's ongoing event

+         containing only those builds (by DB id).

+         """

+ 

+         failed_to_cancel_builds_id = []

+         log_fail = log.error if event.last_try else log.warning

+         with koji_service(

+                 conf.koji_profile, log, dry_run=event.dry_run) as session:

+             builds = db.session.query(ArtifactBuild).filter(

+                 ArtifactBuild.id.in_(event.body['builds_id'])).all()

+             for build in builds:

+                 if session.cancel_build(build.build_id):

+                     build.state_reason = 'Build canceled in external build system.'

+                     continue

+                 if event.last_try:

+                     build.state_reason = ('Build was NOT canceled in external build system.'

+                                           ' Max number of tries reached!')

+                 failed_to_cancel_builds_id.append(build.id)

+             db.session.commit()

+ 

+         if failed_to_cancel_builds_id:

+             log_fail("Builds which failed to cancel in external build system,"

+                      " by DB id: %s; try #%s",

+                      failed_to_cancel_builds_id, event.try_count)

+         if event.last_try or not failed_to_cancel_builds_id:

+             return []

+ 

+         event.body['builds_id'] = failed_to_cancel_builds_id

+         return [event]

@@ -154,12 +154,12 @@ 

              db_event.transition(

                  EventState.COMPLETE,

                  'Advisory %s: %d of %d container image(s) failed to rebuild.' % (

-                     db_event.search_key, num_failed, len(db_event.builds),))

+                     db_event.search_key, num_failed, len(db_event.builds.all()),))

          else:

              db_event.transition(

                  EventState.COMPLETE,

                  'Advisory %s: All %s container images have been rebuilt.' % (

-                     db_event.search_key, len(db_event.builds),))

+                     db_event.search_key, len(db_event.builds.all()),))

  

      def _verify_advisory_rpms_in_container_build(self, errata_id, container_build_id):

          """

@@ -124,7 +124,7 @@ 

              db_event.get_image_builds_in_first_batch(db.session))

  

          msg = 'Advisory %s: Rebuilding %d container images.' % (

-             db_event.search_key, len(db_event.builds))

+             db_event.search_key, len(db_event.builds.all()))

just curious, why is this needed?

See below the change in models:Event.builds. After the change, it returns AppenderQuery. The for loops are not affected by the change, but e.g. here, the AppenderQuery needs to be processed first.

          db_event.transition(EventState.BUILDING, msg)

  

          return []

@@ -140,7 +140,7 @@ 

          batch = 0

          printed = []

          while (len(printed) != len(builds.values()) or

-                len(printed) != len(db_event.builds)):

+                len(printed) != len(db_event.builds.all())):

              self.log_info('   Batch %d:', batch)

              old_printed_count = len(printed)

              for build in builds.values():

@@ -178,6 +178,9 @@ 

  

          return task_id

  

+     def cancel_build(self, build_id):

+         return self.session.cancelBuild(build_id)

+ 

      def get_build_rpms(self, build_nvr, arches=None):

          build_info = self.session.getBuild(build_nvr)

          return self.session.listRPMs(buildID=build_info['id'],

file modified
+26 -8

@@ -145,8 +145,10 @@ 

      state = db.Column(db.Integer, nullable=False)

      state_reason = db.Column(db.String, nullable=True)

      time_created = db.Column(db.DateTime, nullable=True)

-     # List of builds associated with this Event.

-     builds = relationship("ArtifactBuild", back_populates="event")

+     # AppenderQuery for getting builds associated with this Event.

+     builds = relationship("ArtifactBuild", back_populates="event",

Why is this change needed?

This is here to be able to further filter the builds, see https://pagure.io/freshmaker/pull-request/371#_7__34

+                           lazy="dynamic", cascade="all, delete-orphan",

+                           passive_deletes=True)

      # True if the even should be handled in dry run mode.

      dry_run = db.Column(db.Boolean, default=False)

      # For manual rebuilds, set to user requesting the rebuild. Otherwise null.

@@ -288,13 +290,23 @@ 

          return db.session.query(ArtifactBuild).filter_by(

              event_id=self.id).filter(state != state).count() == 0

  

-     def builds_transition(self, state, reason):

+     def builds_transition(self, state, reason, filters=None):

          """

          Calls transition(state, reason) for all builds associated with this

          event.

+ 

+         :param dict filters: Filter only specific builds to transition.

+         :return: list of build ids which were transitioned

          """

-         for build in self.builds:

-             build.transition(state, reason)

+ 

+         if not self.builds:

+             return []

+ 

+         builds_to_transition = self.builds.filter_by(

+             **filters).all() if isinstance(filters, dict) else self.builds

+ 

+         return [build.id

+                 for build in builds_to_transition if build.transition(state, reason)]

  

      def transition(self, state, state_reason=None):

          """

@@ -302,6 +314,7 @@ 

  

          :param state: EventState value

          :param state_reason: Reason why this state has been set.

+         :return: True/False, whether state was changed

          """

  

          # Log the state and state_reason

@@ -313,7 +326,7 @@ 

              self, EventState(state).name, state_reason))

  

          if self.state == state:

-             return

+             return False

  

          self.state = state

          if EventState(state).counter:

@@ -325,6 +338,8 @@ 

          messaging.publish('event.state.changed', self.json())

          messaging.publish('event.state.changed.min', self.json_min())

  

+         return True

+ 

      def __repr__(self):

          return "<Event %s, %r, %s>" % (self.message_id, self.event_type, self.search_key)

  

@@ -348,7 +363,7 @@ 

  

      def json_min(self):

          builds_summary = defaultdict(int)

-         builds_summary['total'] = len(self.builds)

+         builds_summary['total'] = len(self.builds.all())

          for build in self.builds:

              state_name = ArtifactBuildState(build.state).name

              builds_summary[state_name] += 1

@@ -526,6 +541,7 @@ 

  

          :param state: ArtifactBuildState value

          :param state_reason: Reason why this state has been set.

+         :return: True/False, whether state was changed

          """

  

          # Log the state and state_reason

@@ -537,7 +553,7 @@ 

              self, ArtifactBuildState(state).name, state_reason))

  

          if self.state == state:

-             return

+             return False

  

          self.state = state

          if ArtifactBuildState(state).counter:

@@ -561,6 +577,8 @@ 

  

          messaging.publish('build.state.changed', self.json())

  

+         return True

+ 

      def __repr__(self):

          return "<ArtifactBuild %s, type %s, state %s, event %s>" % (

              self.name, ArtifactType(self.type).name,

file modified
+4

@@ -118,6 +118,10 @@ 

      'freshmaker_event_skipped',

      'Number of events, for which no action was taken',

      registry=registry)

+ freshmaker_event_canceled_counter = Counter(

+     'freshmaker_event_canceled',

+     'Number of events canceled during their handling',

+     registry=registry)

  

  freshmaker_build_api_latency = Histogram(

      'build_api_latency',

@@ -20,3 +20,4 @@ 

  # SOFTWARE.

  

  from .manual_rebuild import FreshmakerManualRebuildParser  # noqa

+ from .freshmaker_manage_request import FreshmakerManageRequestParser  # noqa

@@ -0,0 +1,72 @@ 

+ # -*- coding: utf-8 -*-

+ # Copyright (c) 2017  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ #

+ # Written by Filip Valder <fvalder@redhat.com>

+ 

+ from freshmaker.parsers import BaseParser

+ from freshmaker.events import FreshmakerManageEvent

+ 

+ 

+ class FreshmakerManageRequestParser(BaseParser):

+     """Parser parsing freshmaker.manage.* events"""

+ 

+     name = "FreshmakerManageRequestParser"

+     topic_suffixes = ["freshmaker.manage.eventcancel"]

+ 

+     def can_parse(self, topic, msg):

+         return any([topic.endswith(s) for s in self.topic_suffixes])

+ 

+     def parse(self, topic, msg):

+         """

+         Parse message and call specific method according to the action

+         defined within the message.

+         """

+         action_from_topic = topic.split('.')[-1]

+         inner_msg = msg.get('msg')

+ 

+         if 'action' not in inner_msg:

+             raise ValueError("Action is not defined within the message.")

+ 

+         if inner_msg['action'] != action_from_topic:

+             raise ValueError("Last part of 'Freshmaker manage' message topic"

+                              " must match the action defined within the message.")

+ 

+         if 'try' not in inner_msg:

+             inner_msg['try'] = 0

+ 

+         try:

+             getattr(self, action_from_topic)(inner_msg)

+         except AttributeError:

+             raise NotImplementedError("The message contains unsupported action.")

+ 

+         return FreshmakerManageEvent(inner_msg)

+ 

+     def eventcancel(self, inner_msg):

+         """

+         Parse message for event cancelation request

+         """

+         try:

+             inner_msg['event_id']

+             inner_msg['builds_id']

+         except KeyError:

+             raise ValueError("Message doesn't contain all required information.")

+ 

+         return True

file modified
+5 -2

@@ -25,7 +25,7 @@ 

      freshmaker_artifact_build_failed_counter,

      freshmaker_artifact_build_canceled_counter,

      freshmaker_event_complete_counter, freshmaker_event_failed_counter,

-     freshmaker_event_skipped_counter)

+     freshmaker_event_skipped_counter, freshmaker_event_canceled_counter)

  

  

  class ArtifactType(Enum):

@@ -70,7 +70,8 @@ 

              None,

              freshmaker_event_complete_counter,

              freshmaker_event_failed_counter,

-             freshmaker_event_skipped_counter

+             freshmaker_event_skipped_counter,

+             freshmaker_event_canceled_counter

          ]

  

          if type(value) == int:

@@ -87,3 +88,5 @@ 

      FAILED = 3

      # no action to take upon the event

      SKIPPED = 4

+     # handling of the event has been canceled (also canceling builds etc.)

+     CANCELED = 5

file modified
+50 -1

@@ -34,6 +34,7 @@ 

  from freshmaker import db

  from freshmaker import conf

  from freshmaker import version

+ from freshmaker import log

  from freshmaker.api_utils import filter_artifact_builds

  from freshmaker.api_utils import filter_events

  from freshmaker.api_utils import json_error

@@ -43,6 +44,7 @@ 

  from freshmaker.monitor import (

      monitor_api, freshmaker_build_api_latency, freshmaker_event_api_latency)

  from freshmaker.image_verifier import ImageVerifier

+ from freshmaker.types import ArtifactBuildState, EventState

  

  api_v1 = {

      'event_types': {

@@ -101,7 +103,7 @@ 

          'event': {

              'url': '/api/1/events/<int:id>',

              'options': {

-                 'methods': ['GET'],

+                 'methods': ['GET', 'PATCH'],

              }

          },

      },

@@ -217,6 +219,9 @@ 

  

  

  class EventAPI(MethodView):

+ 

+     _freshmaker_manage_prefix = 'event'

+ 

      @freshmaker_event_api_latency.time()

      def get(self, id):

          if id is None:

@@ -236,6 +241,50 @@ 

              else:

                  return json_error(404, "Not Found", "No such event found.")

  

+     @login_required

+     @requires_role('admins')

+     def patch(self, id):

+         """

+         Manage event

+ 

+         Accepts JSON with following key/value pairs:

+             - "action" - one of currently supported actions: 'cancel'

+         """

+         data = request.get_json(force=True)

+         if 'action' not in data:

+             return json_error(

+                 400, "Bad Request", "Missing action in request."

+                 " Don't know what to do with the event.")

+ 

+         if data['action'] == 'cancel':

+             event = models.Event.query.filter_by(id=id).first()

+             if not event:

+                 return json_error(400, "Not Found", "No such event found.")

+ 

+             msg = "Event id %s requested for canceling by user %s" % \

+                 (event.id, g.user.username)

+             log.info(msg)

+ 

+             event.transition(EventState.CANCELED, msg)

+             event.builds_transition(

+                 ArtifactBuildState.CANCELED.value,

+                 "Build canceled before running on external build system.",

+                 filters={'state': ArtifactBuildState.PLANNED.value})

+             builds_id = event.builds_transition(

+                 ArtifactBuildState.CANCELED.value, None,

+                 filters={'state': ArtifactBuildState.BUILD.value})

+             db.session.commit()

+ 

+             data["action"] = self._freshmaker_manage_prefix + data["action"]

+             data["event_id"] = event.id

+             data["builds_id"] = builds_id

+             messaging.publish("manage.eventcancel", data)

+             # Return back the JSON representation of Event to client.

+             return jsonify(event.json()), 200

+         else:

+             return json_error(

+                 400, "Bad Request", "Unsupported action requested.")

+ 

  

  class BuildAPI(MethodView):

      @freshmaker_build_api_latency.time()

@@ -0,0 +1,18 @@ 

+ {

+   "i": 85,

+   "source_version": "0.1.1",

+   "username": "freshmaker",

+   "signature": "123",

+   "timestamp": 1552038054.3143399,

+   "topic": "org.fedoraproject.prod.freshmaker.manage.eventcancel",

+   "source_name": "unittest",

+   "msg": {

+     "action": "eventcancel",

+     "event_id": 3,

+     "builds_id": [

+       1,

+       2

+     ]

+   },

+   "msg_id": "2019-4d21d4c1-22b0-4182-a680-8de1b1374e0c"

+ }

@@ -0,0 +1,13 @@ 

+ {

+   "i": 86,

+   "source_version": "0.1.1",

+   "username": "freshmaker",

+   "signature": "124",

+   "timestamp": 1552038054.3143400,

+   "topic": "org.fedoraproject.prod.freshmaker.manage.eventcancel",

+   "source_name": "unittest",

+   "msg": {

+     "action": "unsupported"

+   },

+   "msg_id": "2019-4483a0ba-1234-46a1-be7e-d2bf4104ed46"

+ }

@@ -0,0 +1,11 @@ 

+ {

+   "i": 86,

+   "source_version": "0.1.1",

+   "username": "freshmaker",

+   "signature": "124",

+   "timestamp": 1552038054.3143400,

+   "topic": "org.fedoraproject.prod.freshmaker.manage.eventcancel",

+   "source_name": "unittest",

+   "msg": {},

+   "msg_id": "2019-4483a0ba-1234-46a1-be7e-d2bf4104ed46"

+ }

@@ -0,0 +1,133 @@ 

+ # Copyright (c) 2017  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ #

+ # Written by Filip Valder <fvalder@redhat.com>

+ 

+ import unittest

+ 

+ from mock import patch

+ 

+ from freshmaker import events, models, db

+ from freshmaker.handlers.internal import CancelEventOnFreshmakerManageRequest

+ from freshmaker.parsers.internal import FreshmakerManageRequestParser

+ from freshmaker.types import ArtifactBuildState

+ 

+ from tests import helpers, get_fedmsg

+ 

+ 

+ class ErroneousFreshmakerManageRequestsTest(helpers.ModelsTestCase):

+     def setUp(self):

+         super(ErroneousFreshmakerManageRequestsTest, self).setUp()

+         events.BaseEvent.register_parser(FreshmakerManageRequestParser)

+ 

+     def test_freshmaker_manage_mismatched_action(self):

+         msg = get_fedmsg('freshmaker_manage_mismatched_action')

+         with self.assertRaises(ValueError) as err:

+             self.get_event_from_msg(msg)

+         self.assertEqual(

+             err.exception.args[0], 'Last part of \'Freshmaker manage\' message'

+             ' topic must match the action defined within the message.')

+ 

+     def test_freshmaker_manage_missing_action(self):

+         msg = get_fedmsg('freshmaker_manage_missing_action')

+         with self.assertRaises(ValueError) as err:

+             self.get_event_from_msg(msg)

+         self.assertEqual(

+             err.exception.args[0], 'Action is not defined within the message.')

+ 

+     def test_more_than_max_tries_on_freshmaker_manage_request(self):

+         msg = get_fedmsg('freshmaker_manage_eventcancel')

+         msg['body']['msg']['try'] = events.FreshmakerManageEvent._max_tries

+         event = self.get_event_from_msg(msg)

+ 

+         handler = CancelEventOnFreshmakerManageRequest()

+         self.assertFalse(handler.can_handle(event))

+ 

+ 

+ class CancelEventOnFreshmakerManageRequestTest(helpers.ModelsTestCase):

+     def setUp(self):

+         super(CancelEventOnFreshmakerManageRequestTest, self).setUp()

+         events.BaseEvent.register_parser(FreshmakerManageRequestParser)

+ 

+         self.db_event = models.Event.create(

+             db.session, "2017-00000000-0000-0000-0000-000000000003", "RHSA-2018-103",

+             events.TestingEvent)

+         models.ArtifactBuild.create(

+             db.session, self.db_event, "mksh", "module", build_id=1237,

+             state=ArtifactBuildState.CANCELED.value)

+         models.ArtifactBuild.create(

+             db.session, self.db_event, "bash", "module", build_id=1238,

+             state=ArtifactBuildState.CANCELED.value)

+ 

+     @patch('freshmaker.kojiservice.KojiService.cancel_build')

+     def test_cancel_event_on_freshmaker_manage_request(self, mocked_cancel_build):

+         msg = get_fedmsg('freshmaker_manage_eventcancel')

+         event = self.get_event_from_msg(msg)

+ 

+         handler = CancelEventOnFreshmakerManageRequest()

+         self.assertTrue(handler.can_handle(event))

+         retval = handler.handle(event)

+         self.assertEqual(retval, [])

+ 

+         mocked_cancel_build.assert_any_call(1237)

+         mocked_cancel_build.assert_any_call(1238)

+         self.assertEqual([b.state_reason for b in self.db_event.builds.all()].count(

+             "Build canceled in external build system."), 2)

+ 

+     def test_can_not_handle_other_action_than_eventcancel(self):

+         msg = get_fedmsg('freshmaker_manage_eventcancel')

+         msg['body']['topic'] = 'freshmaker.manage.someotheraction'

+         msg['body']['msg']['action'] = 'someotheraction'

+         event = self.get_event_from_msg(msg)

+ 

+         handler = CancelEventOnFreshmakerManageRequest()

+         self.assertFalse(handler.can_handle(event))

+ 

+     @patch('freshmaker.kojiservice.KojiService.cancel_build', side_effect=[False, False])

+     def test_max_tries_reached_on_cancel_event(self, mocked_cancel_build):

+         msg = get_fedmsg('freshmaker_manage_eventcancel')

+         msg['body']['msg']['try'] = events.FreshmakerManageEvent._max_tries - 1

+         event = self.get_event_from_msg(msg)

+ 

+         handler = CancelEventOnFreshmakerManageRequest()

+         retval = handler.handle(event)

+         self.assertEqual(retval, [])

+ 

+         self.assertEqual([b.state_reason for b in self.db_event.builds.all()].count(

+             "Build was NOT canceled in external build system. Max number of tries reached!"), 2)

+ 

+     @patch('freshmaker.kojiservice.KojiService.cancel_build',

+            side_effect=[False, False, True, True])

+     def test_retry_failed_cancel_event_with_success(self, mocked_cancel_build):

+         msg = get_fedmsg('freshmaker_manage_eventcancel')

+         event = self.get_event_from_msg(msg)

+ 

+         handler = CancelEventOnFreshmakerManageRequest()

+         new_event = handler.handle(event)

+         self.assertTrue(isinstance(new_event, list) and len(new_event))

+         retval = handler.handle(new_event[0])

+         self.assertEqual(retval, [])

+ 

+         self.assertEqual([b.state_reason for b in self.db_event.builds.all()].count(

+             "Build canceled in external build system."), 2)

+ 

+ 

+ if __name__ == '__main__':

+     unittest.main()

file modified
+1 -1

@@ -50,7 +50,7 @@ 

          self.assertEqual(e.message_id, "test_msg_id")

          self.assertEqual(e.search_key, "RHSA-2017-284")

          self.assertEqual(e.event_type, events.TestingEvent)

-         self.assertEqual(len(e.builds), 2)

+         self.assertEqual(len(e.builds.all()), 2)

  

          self.assertEqual(e.builds[0].name, "ed")

          self.assertEqual(e.builds[0].type, 2)

file modified
+1 -1

@@ -29,7 +29,7 @@ 

  from freshmaker import app, db, events, models, login_manager

  from tests import helpers

  

- num_of_metrics = 44

+ num_of_metrics = 46

  

  

  @login_manager.user_loader

file modified
+42

@@ -350,6 +350,48 @@ 

          for id, build in zip([2, 1], evs):

              self.assertEqual(id, build['id'])

  

+     def test_patch_event_missing_action(self):

+         resp = self.client.patch(

+             '/api/1/events/1',

+             data=json.dumps({}))

+         data = json.loads(resp.get_data(as_text=True))

+         self.assertEqual(data['error'], 'Bad Request')

+         self.assertTrue(data['message'].startswith('Missing action in request.'))

+ 

+     def test_patch_event_unsupported_action(self):

+         resp = self.client.patch(

+             '/api/1/events/1',

+             data=json.dumps({'action': 'unsupported'}))

+         data = json.loads(resp.get_data(as_text=True))

+         self.assertEqual(data['error'], 'Bad Request')

+         self.assertTrue(data['message'].startswith('Unsupported action requested.'))

+ 

+     def test_patch_event_cancel(self):

+         event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003",

+                                     "RHSA-2018-103", events.TestingEvent)

+         models.ArtifactBuild.create(db.session, event, "mksh", "module", build_id=1237,

+                                     state=ArtifactBuildState.PLANNED.value)

+         models.ArtifactBuild.create(db.session, event, "bash", "module", build_id=1238,

+                                     state=ArtifactBuildState.PLANNED.value)

+         models.ArtifactBuild.create(db.session, event, "dash", "module", build_id=1239,

+                                     state=ArtifactBuildState.BUILD.value)

+         models.ArtifactBuild.create(db.session, event, "tcsh", "module", build_id=1240,

+                                     state=ArtifactBuildState.DONE.value)

+         db.session.commit()

+ 

+         resp = self.client.patch(

+             '/api/1/events/{}'.format(event.id),

+             data=json.dumps({'action': 'cancel'}))

+         data = json.loads(resp.get_data(as_text=True))

+ 

+         self.assertEqual(data['id'], event.id)

+         self.assertEqual(len(data['builds']), 4)

+         self.assertEqual(data['state_name'], 'CANCELED')

+         self.assertTrue(data['state_reason'].startswith(

+             'Event id {} requested for canceling by user '.format(event.id)))

+         self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'CANCELED']), 3)

+         self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'DONE']), 1)

+ 

      def test_query_event_types(self):

          resp = self.client.get('/api/1/event-types/')

          event_types = json.loads(resp.get_data(as_text=True))['items']

  • Frontend checks the request, cancels the database event with the corresponding builds and generates a message. The message contains builds_id of builds affected by canceling. It also has reference to the canceled event: event_id.
  • Backend parses the message (using the FreshmakerManageRequestParser) and generates FreshmakerManageEvent. The event holds the body of the original message.
  • The event is further processed by CancelEventOnFreshmakerManageRequest handler, which finds build_id for each ArtifactBuild and tries to cancel them in external build system (Koji). For builds, which failed to cancel, it generates a list of builds_id, which is inject into a new event returned from the previous/already handled one. This way several retries are performed, until max_retries is reached.

The new approach of "Freshmaker manage" messages can be reused for other similar scenarios.

UPDATE: Core of the retry machinery has moved to FreshmakerManageEvent. There's a comment in the code explaining details.

rebased onto c88f673

a month ago

rebased onto 21109d5

a month ago

pretty please pagure-ci rebuild

a month ago

rebased onto 88eb923

a month ago

rebased onto 50d5e0c

a month ago

rebased onto 303d2a1

a month ago

rebased onto 6bef6a1

a month ago

rebased onto 307e9fb

a month ago

pretty please pagure-ci rebuild

a month ago

I know we only have single FreshmakerManageEvent, but can we force this handler to really handle only the 'cancelevent' case?

I'm thinking about adding topic to FreshmakerManageEventand checking here if the topic is reallycancelevent`.

This will prevent this handler from running in case we add another manage event.

Why is this change needed?

Looks good generally, I just have few questions and one change request ;).

This is here to be able to further filter the builds, see https://pagure.io/freshmaker/pull-request/371#_7__34

See below the change in models:Event.builds. After the change, it returns AppenderQuery. The for loops are not affected by the change, but e.g. here, the AppenderQuery needs to be processed first.

As I understand, messages from message bus always go through the parser, which does exactly, what you mean. (And I also want this behavior to be there - checking the topic/action.) I am only afraid of messages generated internally, which needn't(?) go through the parser. Then we maybe want to have some check in the __new__ method/constructor of FreshmakerManageEvent?

If we add new action in parser, we will need to remember to also update this handler to not handle that new action. Instead, I would like to write a code in a way that we can add new action in parser and do not care about the existing handlers. It would be clear that this handler really handles just the cancelevent action and will do that even if we implement some new action in the future.

rebased onto 1b1e309

a month ago

Please for merging https://pagure.io/freshmaker/pull-request/373 before this PR, so that we can check the unit tests are really OK in the CI for this PR.

The @jkaluza's change request was done in 1b1e309.

pretty please pagure-ci rebuild

a month ago

rebased onto 4f20383

a month ago

rebased onto 4dac014

a month ago

rebased onto 8bc180f

a month ago

Pull-Request has been merged by fivaldi

a month ago