#308 Simplify the manual rebuilds by removing FreshmakerManualRebuildEvent and handler. We call the particular manual rebuild event directly now.
Merged 10 months ago by jkaluza. Opened 10 months ago by jkaluza.
jkaluza/freshmaker rebuild-with-advisory  into  master

file modified
-1

@@ -57,7 +57,6 @@ 

  

      # List of enabled composing handlers.

      HANDLERS = [

-         "freshmaker.handlers.internal:FreshmakerManualRebuildHandler",

          "freshmaker.handlers.bodhi:BodhiUpdateCompleteStableHandler",

          "freshmaker.handlers.git:GitDockerfileChangeHandler",

          "freshmaker.handlers.git:GitModuleMetadataChangeHandler",

file modified
+24

@@ -309,6 +309,26 @@ 

      """

  

  

+ class ManualRebuildWithAdvisoryEvent(ErrataAdvisoryRPMsSignedEvent):

+     """

+     Event representing manual rebuild of particular container images with RPMs

+     from advisory.

+     """

+ 

+     def __init__(self, msg_id, advisory, container_images, **kwargs):

+         """

+         Creates new ManualRebuildWithAdvisoryEvent.

+ 

+         :param str msg_id: Message id.

+         :param ErrataAdvisory advisory: Errata advisory associated with event.

+         :param list container_images: List of NVRs of images to rebuild or

+             empty list to rebuild all images affected by the advisory.

+         """

+         super(ManualRebuildWithAdvisoryEvent, self).__init__(

+             msg_id, advisory, **kwargs)

+         self.container_images = container_images

+ 

+ 

  class BrewSignRPMEvent(BaseEvent):

      """

      Represents the message sent by Brew when RPM is signed.

@@ -350,6 +370,10 @@ 

  

  

  class FreshmakerManualRebuildEvent(BaseEvent):

+     """

+     NOTE: This event is deprecated and not used anymore, but we have to keep

+     it around, because we have instances of this event stored in database.

+     """

      def __init__(self, msg_id, errata_id=None, dry_run=False):

          super(FreshmakerManualRebuildEvent, self).__init__(

              msg_id, dry_run=dry_run)

@@ -610,7 +610,6 @@ 

          """

  

          parsed_nvr = koji.parse_NVR(image["brew"]["build"])

-         image_name = koji.parse_NVR(image["brew"]["build"])['name']

  

          if not self.event.is_allowed(

                  self, image_name=parsed_nvr["name"],

@@ -1,22 +0,0 @@ 

- # -*- 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.

- 

- from .manual_rebuild import FreshmakerManualRebuildHandler  # noqa

@@ -1,85 +0,0 @@ 

- # -*- 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 Jan Kaluza <jkaluza@redhat.com>

- 

- from freshmaker import db, log

- from freshmaker.models import Event

- from freshmaker.handlers import ContainerBuildHandler

- from freshmaker.events import (

-     FreshmakerManualRebuildEvent, ErrataAdvisoryStateChangedEvent)

- from freshmaker.errata import Errata

- from freshmaker.types import EventState

- 

- __all__ = ('FreshmakerManualRebuildHandler',)

- 

- 

- class FreshmakerManualRebuildHandler(ContainerBuildHandler):

-     """Start image rebuild with this compose containing included packages"""

- 

-     def can_handle(self, event):

-         if not isinstance(event, FreshmakerManualRebuildEvent):

-             return False

-         return True

- 

-     def generate_fake_event(self, manual_rebuild_event):

-         """

-         Returns fake ErrataAdvisoryStateChangedEvent which will trigger manual

-         rebuild of artifacts based on Errata advisory `errata_id`.

- 

-         :param manual_rebuild_event: FreshmakerManualRebuildEvent instance.

-         :rtype: ErrataAdvisoryStateChangedEvent

-         :return: Newly generated ErrataAdvisoryStateChangedEvent.

-         """

- 

-         # Get additional info from Errata to fill in the needed data.

-         errata = Errata()

-         advisories = errata.advisories_from_event(manual_rebuild_event)

-         if not advisories:

-             msg = "Unknown Errata advisory %d" % manual_rebuild_event.errata_id

-             self.current_db_event.transition(EventState.FAILED, msg)

-             db.session.commit()

-             return None

- 

-         log.info("Generating ErrataAdvisoryStateChangedEvent for Errata "

-                  "advisory %d - manually triggered rebuild.",

-                  manual_rebuild_event.errata_id)

-         advisory = advisories[0]

-         new_event = ErrataAdvisoryStateChangedEvent(

-             manual_rebuild_event.msg_id + "." + str(advisory.name), advisory)

-         new_event.manual = True

-         new_event.dry_run = manual_rebuild_event.dry_run

-         msg = ("Generated ErrataAdvisoryStateChangedEvent (%s) for errata: %s"

-                % (manual_rebuild_event.msg_id, manual_rebuild_event.errata_id))

-         self.current_db_event.transition(EventState.COMPLETE, msg)

-         db.session.commit()

-         return new_event

- 

-     def handle(self, event):

-         # We log every manual trigger event to DB.

-         db_event = Event.get_or_create_from_event(db.session, event)

-         db.session.commit()

-         self.set_context(db_event)

- 

-         fake_event = self.generate_fake_event(event)

-         if not fake_event:

-             return []

-         return [fake_event]

file modified
+2 -1

@@ -44,7 +44,7 @@ 

      BodhiUpdateCompleteStableEvent, KojiTaskStateChangeEvent, BrewSignRPMEvent,

      ErrataAdvisoryRPMsSignedEvent, BrewContainerTaskStateChangeEvent,

      ErrataAdvisoryStateChangedEvent, FreshmakerManualRebuildEvent,

-     ODCSComposeStateChangeEvent)

+     ODCSComposeStateChangeEvent, ManualRebuildWithAdvisoryEvent)

  

  EVENT_TYPES = {

      MBSModuleStateChangeEvent: 0,

@@ -60,6 +60,7 @@ 

      ErrataAdvisoryStateChangedEvent: 10,

      FreshmakerManualRebuildEvent: 11,

      ODCSComposeStateChangeEvent: 12,

+     ManualRebuildWithAdvisoryEvent: 13,

  }

  

  INVERSE_EVENT_TYPES = {v: k for k, v in EVENT_TYPES.items()}

@@ -19,24 +19,43 @@ 

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

  # SOFTWARE.

  

+ import time

  from freshmaker.parsers import BaseParser

- from freshmaker.events import FreshmakerManualRebuildEvent

+ from freshmaker.events import ManualRebuildWithAdvisoryEvent

+ from freshmaker.errata import Errata, ErrataAdvisory

  

  

  class FreshmakerManualRebuildParser(BaseParser):

-     """Parser parsing odcs.compose.state.change"""

+     """Parser parsing freshmaker.manual.rebuild"""

  

-     name = "FreshmakerManualRebuildEvent"

+     name = "FreshmakerManualRebuildParser"

      topic_suffixes = ["freshmaker.manual.rebuild"]

  

      def can_parse(self, topic, msg):

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

  

+     def parse_post_data(self, data):

+         """

+         Method shared between Frontend and Backend to parse the POST data

+         of manual rebuild JSON and generate the BaseEvent representation

+         of the rebuild request.

+ 

+         :param dict data: Dict generated from JSON from HTTP POST or parsed

+             from the UMB message sent from Frontend to Backend.

+         """

+         msg_id = data.get('msg_id', "manual_rebuild_%s" % (str(time.time())))

+         errata_id = data.get('errata_id')

+         dry_run = data.get('dry_run', False)

+ 

+         errata = Errata()

+         advisory = ErrataAdvisory.from_advisory_id(errata, errata_id)

+ 

+         event = ManualRebuildWithAdvisoryEvent(

+             msg_id, advisory, data.get("container_images", []), manual=True,

+             dry_run=dry_run)

+ 

+         return event

+ 

      def parse(self, topic, msg):

-         msg_id = msg.get('msg_id')

          inner_msg = msg.get('msg')

-         errata_id = inner_msg.get('errata_id')

-         dry_run = inner_msg.get('dry_run', False)

- 

-         return FreshmakerManualRebuildEvent(

-             msg_id, errata_id=errata_id, dry_run=dry_run)

+         return self.parse_post_data(inner_msg)

file modified
+31 -6

@@ -29,12 +29,13 @@ 

  from freshmaker import messaging

  from freshmaker import models

  from freshmaker import types

+ from freshmaker import db

  from freshmaker.api_utils import filter_artifact_builds

  from freshmaker.api_utils import filter_events

  from freshmaker.api_utils import json_error

  from freshmaker.api_utils import pagination_metadata

  from freshmaker.auth import login_required, requires_role, require_scopes

- from freshmaker.errata import Errata, ErrataAdvisory

+ from freshmaker.parsers.internal.manual_rebuild import FreshmakerManualRebuildParser

  

  api_v1 = {

      'event_types': {

@@ -227,22 +228,46 @@ 

      @require_scopes('submit-build')

      @requires_role('admins')

      def post(self):

-         """Trigger image rebuild"""

+         """

+         Trigger manual image rebuild.

+ 

+         Accepts JSON in POST with following key/value pairs:

+             - "errata_id" - ID of Errata advisory to include in rebuild

+             - "container_images" - Optional. List of NVRs of leaf container

+               images to rebuild.

+         """

          data = request.get_json(force=True)

          if 'errata_id' not in data:

              return json_error(

                  400, 'Bad Request', 'Missing errata_id in request')

  

-         errata = Errata()

-         advisory = ErrataAdvisory.from_advisory_id(errata, data['errata_id'])

-         if 'rpm' not in advisory.content_types:

+         # Use the shared code to parse the POST data and generate right

+         # event based on the data. Currently it generates just

+         # ManualRebuildWithAdvisoryEvent.

+         parser = FreshmakerManualRebuildParser()

+         event = parser.parse_post_data(data)

+ 

+         # Check the the advisory is RPM advisory.

+         if 'rpm' not in event.advisory.content_types:

              return json_error(

                  400,

                  'Bad Request',

                  'Erratum {} is not a RPM advisory'.format(data['errata_id']))

  

+         # Store the event into database, so it gets the ID which we can return

+         # to client sending this POST request. The client can then use the ID

+         # to check for the event status.

+         db_event = models.Event.get_or_create_from_event(db.session, event)

+         db.session.commit()

+ 

+         # Forward the POST data (including the msg_id of the database event we

+         # added to DB) to backend using UMB messaging. Backend will then

+         # re-generate the event and start handling it.

+         data["msg_id"] = event.msg_id

          messaging.publish("manual.rebuild", data)

-         return jsonify(data), 200

+ 

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

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

  

  

  API_V1_MAPPING = {

@@ -26,7 +26,9 @@ 

  import freshmaker

  

  from freshmaker import db

- from freshmaker.events import ErrataAdvisoryRPMsSignedEvent

+ from freshmaker.events import (

+     ErrataAdvisoryRPMsSignedEvent,

+     ManualRebuildWithAdvisoryEvent)

  from freshmaker.handlers.errata import ErrataAdvisoryRPMsSignedHandler

  from freshmaker.lightblue import ContainerImage

  from freshmaker.models import Event, Compose

@@ -222,6 +224,16 @@ 

          super(TestErrataAdvisoryRPMsSignedHandler, self).tearDown()

          self.patcher.unpatch_all()

  

+     def test_can_handle_manual_rebuild_with_advisory_event(self):

+         event = ManualRebuildWithAdvisoryEvent(

+             "123", ["foo-container", "bar-container"],

+             ErrataAdvisory(123, "RHBA-2017", "REL_PREP", [],

+                            security_impact="",

+                            product_short_name="product"))

+         handler = ErrataAdvisoryRPMsSignedHandler()

+         ret = handler.can_handle(event)

+         self.assertTrue(ret)

+ 

      @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

          'ErrataAdvisoryRPMsSignedHandler': {

              'image': {'product_short_name': 'foo'}

@@ -1,69 +0,0 @@ 

- # -*- 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 Chenxiong Qi <cqi@redhat.com>

- 

- from mock import patch

- 

- from freshmaker.handlers.internal import FreshmakerManualRebuildHandler

- from freshmaker.events import FreshmakerManualRebuildEvent

- 

- from freshmaker.errata import ErrataAdvisory

- from freshmaker.models import Event

- from freshmaker.types import EventState

- from tests import helpers

- 

- 

- class TestFreshmakerManualRebuildHandler(helpers.ModelsTestCase):

- 

-     @patch('freshmaker.errata.Errata.advisories_from_event')

-     def test_rebuild_if_not_exists(self, advisories_from_event):

-         handler = FreshmakerManualRebuildHandler()

- 

-         advisories_from_event.return_value = [

-             ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"], "Critical")]

-         ev = FreshmakerManualRebuildEvent("msg123", errata_id=123)

-         ret = handler.handle(ev)

- 

-         self.assertEqual(len(ret), 1)

-         self.assertEqual(ret[0].advisory.errata_id, 123)

-         self.assertEqual(ret[0].advisory.state, "REL_PREP")

-         self.assertEqual(ret[0].manual, True)

- 

-         db_event = Event.query.filter_by(message_id=ev.msg_id).first()

-         self.assertEqual(db_event.state, EventState.COMPLETE.value)

-         self.assertEqual(db_event.state_reason,

-                          'Generated ErrataAdvisoryStateChangedEvent (msg123) for errata: 123')

- 

-     @patch('freshmaker.errata.Errata.advisories_from_event')

-     def test_rebuild_if_not_exists_unknown_errata_id(

-             self, advisories_from_event):

-         advisories_from_event.return_value = []

-         handler = FreshmakerManualRebuildHandler()

- 

-         ev = FreshmakerManualRebuildEvent("msg123", errata_id=123)

-         ret = handler.handle(ev)

- 

-         self.assertEqual(len(ret), 0)

- 

-         db_event = Event.query.filter_by(message_id=ev.msg_id).first()

-         self.assertEqual(db_event.state, EventState.FAILED.value)

-         self.assertEqual(db_event.state_reason, "Unknown Errata advisory 123")

file modified
+22 -6

@@ -456,8 +456,11 @@ 

          self.client = app.test_client()

  

      @patch('freshmaker.messaging.publish')

-     @patch('freshmaker.views.ErrataAdvisory.from_advisory_id')

-     def test_manual_rebuild(self, from_advisory_id, publish):

+     @patch('freshmaker.parsers.internal.manual_rebuild.ErrataAdvisory.'

+            'from_advisory_id')

+     @patch('freshmaker.parsers.internal.manual_rebuild.time.time')

+     def test_manual_rebuild(self, time, from_advisory_id, publish):

+         time.return_value = 123

          from_advisory_id.return_value = ErrataAdvisory(

              123, 'name', 'REL_PREP', ['rpm'])

  

@@ -466,10 +469,23 @@ 

                                  content_type='application/json')

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

  

-         self.assertEqual(data["errata_id"], 1)

-         publish.assert_called_once_with('manual.rebuild', {u'errata_id': 1})

- 

-     @patch('freshmaker.views.ErrataAdvisory.from_advisory_id')

+         # Other fields are predictible.

+         self.assertEqual(data, {

+             u'builds': [],

+             u'event_type_id': 13,

+             u'id': 1,

+             u'message_id': u'manual_rebuild_123',

+             u'search_key': u'123',

+             u'state': 0,

+             u'state_name': u'INITIALIZED',

+             u'state_reason': None,

+             u'url': u'/api/1/events/1'})

+         publish.assert_called_once_with(

+             'manual.rebuild',

+             {'msg_id': 'manual_rebuild_123', u'errata_id': 1})

+ 

+     @patch('freshmaker.parsers.internal.manual_rebuild.ErrataAdvisory.'

+            'from_advisory_id')

      def test_not_rebuild_nonrpm_advisory(self, from_advisory_id):

          from_advisory_id.return_value = ErrataAdvisory(

              123, 'name', 'REL_PREP', ['docker'])

Currently, the Manual rebuild flow looks like this:

  • User sends POST request with JSON data to Freshmaker Frontend.
  • Frontend checks that the data is valid and sends manual.rebuild UMB message with the data to backend.
  • Frontend returns the data back to client with 200 HTTP code.
  • Backend receives the data using the manual.rebuild message and generates FreshmakerManualRebuildEvent.
  • Backend handles FreshmakerManualRebuildEvent, this handler checks what should be the real work done for manual rebuild and generates another event, for example ErrataRPMsSignedEvent, which is further handled to do the real work.

Issues with current situation:

  • User sending the POST request with data has no way to find out what event is actually generated as result of that POST. He has to query the API and guess which event(s) was/were generated for his manual rebuild request.
  • Whole flow is unnecessary complex - we create FreshmakerHandlerEvent and in its handler create another Event and call its handler.

With this PR, manual rebuild looks like this:

  • User sends POST request with JSON data to Freshmaker Frontend.
  • Frontend checks that the data is valid and generates the models.Event representing the manual rebuild and stores it in the database.
  • Frontend sends data to backend using UMB with Event.msg_id, so backend can find just created event in database.
  • Frontend returns the JSON representation of models.Event back to client, including the Event.id, so client knows what event to check later.
  • Backend receives the data using the manual.rebuild message and generates the particular exact event directly. It does not generate FreshmakerManualRebuildEvent anymore, but for example ManualRebuildWithAdvisory event which is subclass of ErrataAdvisoryRPMsSignedEvent and is directly handled by right handler which does the real work to rebuild images.

Pull-Request has been merged by jkaluza

10 months ago