#291 Drop support for fedmsg and replace by fedora-messaging
Merged 5 years ago by rayson. Opened 5 years ago by cverna.
cverna/waiverdb replace_fedmsg  into  master

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

+ # A sample configuration for fedora-messaging. This file is in the TOML format.

+ # For complete details on all configuration options, see the documentation.

+ 

+ amqp_url = "amqp://"

+ 

+ publish_exchange = "amq.topic"

+ 

+ # The topic_prefix configuration value will add a prefix to the topics of every sent message.

+ # This is used for migrating from fedmsg, and should not be used afterwards.

+ topic_prefix = ""

+ 

+ [tls]

+ ca_cert = "/etc/pki/tls/certs/ca-bundle.crt"

+ keyfile = "/my/client/key.pem"

+ certfile = "/my/client/cert.pem"

+ 

+ [client_properties]

+ app = "WaiverDB"

file removed
-129
@@ -1,129 +0,0 @@ 

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

- # SPDX-License-Identifier: GPL-2.0+

- 

- import socket

- 

- hostname = socket.gethostname()

- 

- config = dict(

-     active=True,

-     # Set this to dev if you're hacking on fedmsg or an app.

-     # Set to stg or prod if running in the Fedora Infrastructure

-     environment="dev",

- 

-     # Default is 0

-     high_water_mark=0,

-     io_threads=1,

- 

-     ## For the fedmsg-hub and fedmsg-relay. ##

- 

-     # This is a status dir to keep a record of the last processed message

-     #status_directory=os.getcwd() + "/status",

-     #status_directory='/var/run/fedmsg/status',

- 

-     # This is the URL of a datagrepper instance that we can query for backlog.

-     #datagrepper_url="https://apps.fedoraproject.org/datagrepper/raw",

- 

-     # We almost always want the fedmsg-hub to be sending messages with zmq as

-     # opposed to amqp or stomp.  You can send with only *one* of the messaging

-     # backends: zeromq or amqp or stomp.  You cannot send with two or more at

-     # the same time.  Here, zmq is either enabled, or it is not.  If it is not,

-     # see the options below for how to configure stomp or amqp.

-     zmq_enabled=True,

- 

-     # On the other hand, if you wanted to use STOMP *instead* of zeromq, you

-     # could do the following...

-     #zmq_enabled=False,

-     #stomp_uri='localhost:59597,localhost:59598',

-     #stomp_user='username',

-     #stomp_pass='password',

-     #stomp_ssl_crt='/path/to/an/optional.crt',

-     #stomp_ssl_key='/path/to/an/optional.key',

- 

-     # When subscribing to messages, we want to allow splats ('*') so we tell

-     # the hub to not be strict when comparing messages topics to subscription

-     # topics.

-     zmq_strict=False,

- 

-     # Number of seconds to sleep after initializing waiting for sockets to sync

-     post_init_sleep=0.5,

- 

-     # Wait a whole second to kill all the last io threads for messages to

-     # exit our outgoing queue (if we have any).  This is in milliseconds.

-     zmq_linger=1000,

- 

-     # See the following

-     #   - http://tldp.org/HOWTO/TCP-Keepalive-HOWTO/overview.html

-     #   - http://api.zeromq.org/3-2:zmq-setsockopt

-     zmq_tcp_keepalive=1,

-     zmq_tcp_keepalive_cnt=3,

-     zmq_tcp_keepalive_idle=60,

-     zmq_tcp_keepalive_intvl=5,

- 

-     # Number of miliseconds that zeromq will wait to reconnect until it gets

-     # a connection if an endpoint is unavailable.

-     zmq_reconnect_ivl=100,

-     # Max delay that you can reconfigure to reduce reconnect storm spam. This

-     # is in miliseconds.

-     zmq_reconnect_ivl_max=1000,

- 

-     # This is a dict of possible addresses from which fedmsg can send

-     # messages.  fedmsg.init(...) requires that a 'name' argument be passed

-     # to it which corresponds with one of the keys in this dict.

-     endpoints={

-         "waiverdb.%s" % hostname: [

-             "tcp://127.0.0.1:5011",

-         ],

-         "relay_outbound": [

-             "tcp://127.0.0.1:4001",

-         ],

-     },

-     # This is the address of an active->passive relay.  It is used for the

-     # fedmsg-logger command which requires another service with a stable

-     # listening address for it to send messages to.

-     # It is also used by the git-hook, for the same reason.

-     # It is also used by the mediawiki php plugin which, due to the oddities of

-     # php, can't maintain a single passive-bind endpoint of it's own.

-     relay_inbound=[

-         "tcp://127.0.0.1:2003",

-     ],

-     sign_messages=False,

-     validate_signatures=False,

- 

-     # Use these implementations to sign and validate messages

-     crypto_backend='x509',

-     crypto_validate_backends=['x509'],

- 

-     ssldir="/etc/pki/fedmsg",

-     crl_location="https://fedoraproject.org/fedmsg/crl.pem",

-     crl_cache="/var/run/fedmsg/crl.pem",

-     crl_cache_expiry=10,

- 

-     ca_cert_location="https://fedoraproject.org/fedmsg/ca.crt",

-     ca_cert_cache="/var/run/fedmsg/ca.crt",

-     ca_cert_cache_expiry=0,  # Never expires

- 

-     certnames={

-         # In prod/stg, map hostname to the name of the cert in ssldir.

-         # Unfortunately, we can't use socket.getfqdn()

-         #"app01.stg": "app01.stg.phx2.fedoraproject.org",

-     },

- 

-     # A mapping of fully qualified topics to a list of cert names for which

-     # a valid signature is to be considered authorized.  Messages on topics not

-     # listed here are considered automatically authorized.

-     routing_policy={

-         # Only allow announcements from production if they're signed by a

-         # certain certificate.

-         "org.fedoraproject.prod.announce.announcement": [

-             "announce-lockbox.phx2.fedoraproject.org",

-         ],

-     },

- 

-     # Set this to True if you want messages to be dropped that aren't

-     # explicitly whitelisted in the routing_policy.

-     # When this is False, only messages that have a topic in the routing_policy

-     # but whose cert names aren't in the associated list are dropped; messages

-     # whose topics do not appear in the routing_policy are not dropped.

-     routing_nitpicky=False,

- )

file modified
+1 -1
@@ -1,7 +1,7 @@ 

  # This is a list of pypi packages to be installed into virtualenv. Alternatively,

  # you can install these as RPMs instead of pypi packages.

  

- fedmsg[consumers,commands]

+ fedora_messaging

  Flask

  Flask-RESTful!=0.3.6

  Flask-SQLAlchemy

file modified
+3 -1
@@ -2,6 +2,7 @@ 

  

  import os

  from copy import copy

+ from mock import patch

  import pytest

  from sqlalchemy import create_engine

  from waiverdb.app import create_app
@@ -63,7 +64,8 @@ 

      by default.

      """

      with app.test_client() as client:

-         yield client

+         with patch('fedora_messaging.api._session_cache'):

+             yield client

  

  

  @pytest.fixture()

file modified
+21 -37
@@ -2,12 +2,13 @@ 

  

  """This module contains tests for :mod:`waiverdb.events`."""

  from __future__ import unicode_literals

- import mock

+ from fedora_messaging import api, testing

+ from flask_restful import marshal

  from waiverdb.models import Waiver

+ from waiverdb.fields import waiver_fields

  

  

- @mock.patch('waiverdb.events.fedmsg')

- def test_publish_new_waiver_with_fedmsg(mock_fedmsg, session):

+ def test_publish_new_waiver_with_fedmsg(session):

      waiver = Waiver(

          subject_type='koji_build',

          subject_identifier='glibc-2.26-27.fc27',
@@ -17,29 +18,21 @@ 

          waived=True,

          comment='This is a comment',

      )

+ 

      sesh = session()

      sesh.add(waiver)

-     sesh.commit()

-     mock_fedmsg.publish.assert_called_once_with(

-         topic='waiver.new',

-         msg={

-             'id': waiver.id,

-             'subject_type': 'koji_build',

-             'subject_identifier': 'glibc-2.26-27.fc27',

-             'subject': {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'},

-             'testcase': 'testcase1',

-             'username': 'jcline',

-             'proxied_by': None,

-             'product_version': 'something',

-             'waived': True,

-             'comment': 'This is a comment',

-             'timestamp': waiver.timestamp.isoformat(),

-         }

+     sesh.flush()

+ 

+     expected_msg = api.Message(

+         topic='waiverdb.waiver.new',

+         body=marshal(waiver, waiver_fields)

      )

  

+     with testing.mock_sends(expected_msg):

+         sesh.commit()

  

- @mock.patch('waiverdb.events.fedmsg')

- def test_publish_new_waiver_with_fedmsg_for_proxy_user(mock_fedmsg, session):

+ 

+ def test_publish_new_waiver_with_fedmsg_for_proxy_user(session):

      waiver = Waiver(

          subject_type='koji_build',

          subject_identifier='glibc-2.26-27.fc27',
@@ -52,20 +45,11 @@ 

      )

      sesh = session()

      sesh.add(waiver)

-     sesh.commit()

-     mock_fedmsg.publish.assert_called_once_with(

-         topic='waiver.new',

-         msg={

-             'id': waiver.id,

-             'subject_type': 'koji_build',

-             'subject_identifier': 'glibc-2.26-27.fc27',

-             'subject': {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'},

-             'testcase': 'testcase1',

-             'username': 'jcline',

-             'proxied_by': 'bodhi',

-             'product_version': 'something',

-             'waived': True,

-             'comment': 'This is a comment',

-             'timestamp': waiver.timestamp.isoformat(),

-         }

+     sesh.flush()

+ 

+     expected_msg = api.Message(

+         topic='waiverdb.waiver.new',

+         body=marshal(waiver, waiver_fields)

      )

+     with testing.mock_sends(expected_msg):

+         sesh.commit()

file modified
+1 -1
@@ -42,7 +42,7 @@ 

  [flake8]

  show-source = True

  max-line-length = 100

- exclude = .git,.tox,dist,*egg,env_waiverdb,*fedmsg.d,docs,conf,waiverdb/migrations

+ exclude = .git,.tox,dist,*egg,env_waiverdb,docs,conf,waiverdb/migrations

  

  # E124: closing bracket does not match visual indentation

  # W503 line break before binary operator

file modified
+2 -2
@@ -42,7 +42,7 @@ 

  BuildRequires:  python3-click

  BuildRequires:  python3-flask-migrate

  BuildRequires:  python3-stomppy

- BuildRequires:  python3-fedmsg

+ BuildRequires:  python3-fedora-messaging

  BuildRequires:  python3-prometheus_client

  BuildRequires:  python3-six

  Requires:       python3-flask
@@ -57,7 +57,7 @@ 

  Requires:       python3-click

  Requires:       python3-flask-migrate

  Requires:       python3-stomppy

- Requires:       python3-fedmsg

+ Requires:       python3-fedora-messaging

  Requires:       python3-prometheus_client

  Requires:       waiverdb-common = %{version}-%{release}

  %endif

file modified
-4
@@ -6,10 +6,6 @@ 

  class Config(object):

      """

      A WaiverDB Flask configuration.

- 

-     Attributes:

-         ZEROMQ_PUBLISH (bool): When true, ZeroMQ messages will be emitted via

-             fedmsg when new waivers are created.

      """

      DEBUG = True

      DATABASE_URI = 'postgresql+psycopg2:///waiverdb'

file modified
+13 -5
@@ -12,11 +12,12 @@ 

  import logging

  

  from flask_restful import marshal

- import fedmsg

  import stomp

  import json

  import waiverdb.monitor as monitor

  

+ from fedora_messaging.api import Message, publish

+ from fedora_messaging.exceptions import PublishReturned, ConnectionException

  from flask import current_app

  from waiverdb.fields import waiver_fields

  from waiverdb.models import Waiver
@@ -28,7 +29,7 @@ 

  def publish_new_waiver(session):

      """

      A post-commit event hook that emits messages to a message bus. The messages

-     can be published by either fedmsg-hub with zmq or stomp.

+     can be published by either fedora-messaging or stomp.

  

      This event is designed to be registered with a session factory::

  
@@ -91,10 +92,17 @@ 

                  continue

              _log.debug('Publishing a message for %r', row)

              try:

-                 fedmsg.publish(topic='waiver.new', msg=marshal(row, waiver_fields))

+                 msg = Message(

+                     topic='waiverdb.waiver.new',

+                     body=marshal(row, waiver_fields)

+                 )

+                 publish(msg)

                  monitor.messaging_tx_sent_ok_counter.inc()

-             except Exception:

-                 _log.exception('Couldn\'t publish message via fedmsg')

+             except PublishReturned as e:

+                 _log.exception('Fedora Messaging broker rejected message %s: %s', msg.id, e)

+                 monitor.messaging_tx_failed_counter.inc()

+             except ConnectionException as e:

+                 _log.exception('Error sending message %s: %s', msg.id, e)

                  monitor.messaging_tx_failed_counter.inc()

                  raise

  

Hi Fedora's infrastructure is planning to replace fedmsg by fedora-messaging a library that uses AMQP protocol. You can find more details about this work here.

This PR drops support for fedmsg and replace it by fedora-messaging. Note I left the configuration key to fedmsg since fedmsg can be seen as short fedora-messaging.

Signed-off-by: Clement Verna cverna@tutanota.com

rebased onto 7128308e50ac9840e559217b5c16417312a39c49

5 years ago

Hi @cverna, Jenkins build failed because new dependencies are not present in the Jenkins slave image and warnings came from upgraded Flake8. Hopefully #293 and #294 can solve or mitigate those issues.

rebased onto 883c30950abb201b078e6df838f0aa36168c9532

5 years ago

rebased onto b21ba5ef364a5d3abcaa2955cd06cd18bdb01813

5 years ago

pylint gives this warnings:
waiverdb/events.py:102:31: W1202: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)
waiverdb/events.py:105:31: W1202: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)

@cverna Please rebase again, #294 has been merged, so it should eliminate the pylint's woof-woof. :)

rebased onto ff9f03b0b7ea833f4b773481e6f071b131d123b1

5 years ago

The other stuff (W1202, mentioned by @gnaponie ) needs to be fixed in your PR.

rebased onto caa696a31308f693e093c70adc851cadfa70309f

5 years ago

@gnaponie and @fivaldi Ok that should be fixed and rebased :fireworks:

Currently fails in these tests:

12:13:04 tests/test_events.py::test_publish_new_waiver_with_fedmsg FAILED         [ 89%]
12:13:04 tests/test_events.py::test_publish_new_waiver_with_fedmsg_for_proxy_user FAILED [ 90%]

on:

12:13:04 >                   assert msg == expected
12:13:04 E                   AssertionError

in mock_sends(*expected_messages)

12:13:04 expected_messages = (Message(id='2673684e-d975-496b-b9d3-cbfa29ea2664', topic='waiver.new', body=OrderedDict([('id', 183), ('subject_type'...on', 'something'), ('waived', True), ('comment', 'This is a comment'), ('timestamp', '2019-03-05T11:13:03.784722')])),)
12:13:04 mock_cache = <MagicMock name='_session_cache' id='140624069483320'>
12:13:04 messages = [Message(id='6aaebea8-a94b-4a46-b7b2-2e82fe551c37', topic='waiverdb.waiver.new', body=OrderedDict([('id', 183), ('subj...ion', 'something'), ('waived', True), ('comment', 'This is a comment'), ('timestamp', '2019-03-05T11:13:03.784722')]))]

Seems like an issue with list/tuple when comparing...

rebased onto 586488223ce72ff67104a84682decdaa9d8fd985

5 years ago

Ok I have updated the test :smile:

Thank you for this change!

fedora-messaging only supports AMQP 0.9, which we don't have available internally (STOMP or AMQP 1.0).
Can we make it so it uses either depending on configuration?

FWIW, we are planning to look into extending fedora-messaging to also handle AMQP 1.0.

cc @mikeb, @mprahl

@pingou, you're right. Sorry for the confusion. Change LGTM.

Hi @cverna, can you do a rebase to include #294 for eliminating the W504 flake8 warning?

BTW, subsequent Jenkins builds will fail with RPM build errors because new RPM dependencies can't be installed automatically during a Jenkins job run (we don't have root access to the Jenkins slave pod on OpenShift). That is a known limitation of the WaiverDB CI/CD pipeline. I have run oc start-build waiverdb-premerge-jenkins-slave --commit=pull/291/head for you to rebuild the Jenkins slave image with new dependencies introduce by this PR. So the Jenkins build should be green after you rebase.

rebased onto 60348305ee19a2a779ed53c95f035e471543b45a

5 years ago

Just did the rebase and the CI seems to be happy from what I can see

Ok latest rebase made the CI to fail :cry:

I think it is caused by a network failure:
fatal: unable to access 'https://pagure.io/waiverdb.git/': Could not resolve host: pagure.io

Let me trigger it again manually.

I think it is caused by a network failure:
fatal: unable to access 'https://pagure.io/waiverdb.git/': Could not resolve host: pagure.io
Let me trigger it again manually.

:thumbsup: that fixed the issue.

rebased onto e6bf065

5 years ago

Any chance to have this merged soon ? We would like to be able to test it in staging before the end of week if possible.

Pull-Request has been merged by rayson

5 years ago

@rayson Thanks. Any chance to have a container built with a dedicated tag that we could use in staging to test this ?

I am not sure what is your process for releases :-)

@cverna, yes, we can run waiverdb-postmerge job with WAIVERDB_GIT_REF parameter set to the Git tag you want to build and WAIVERDB_DEV_IMAGE_TAG parameter set to the image tag.

For example:

oc start-build waiverdb-postmerge -e WAIVERDB_GIT_REF=my-git-tag -e WAIVERDB_DEV_IMAGE_TAG=my-image-tag

Then you can pull the image from quay.io/factory2/waiverdb:my-image-tag