New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send events to rabbitmq (to finally terminate hermes) #2592

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@coolo
Copy link
Member

coolo commented Jan 23, 2017

This is actually a feature request, but for reasons that are beyond logic I'm presenting a proof of concept.

Hermes is about dead, but what people still want is a way to get random events. The requests for notifications I'm hearing are "when there are source commits in GNOME:*", "when requests are made", "when my repo was published", "when there is a review for X",...

All these are solved by polling OBS in intervals - and it sucks. What we need is a notifcation bus for tools to hook into, so everyone can build its filters in its own tools.

@asdil12 did some research on how to do this for openQA and they came up with a very plain rabbitmq usage - just one topic where every tool publishes events to and every tool can subscribe filters to.

So this is how it could look like in OBS - plus another table column in the events (unless you guys feel like killing hermes right away, then we can get rid of backend notification plugins completely and reuse the queued column) plus tests (https://github.com/scottwb/bunny-mock looks reasonable).

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

The events look like this on the bus:

 [x] 'opensuse.obs.package.commit':'{"project":"BaseDistro2.0","package":"pack2","user":"unknown","files":"Added:\\n  myfile\\n\\n","rev":"2","time":1377961010}'
 [x] 'opensuse.obs.package.build_fail':'{"project":"home:Iggy","package":"TestPack","repository":"10.2","arch":"i586","release":"42.1","versrel":"1.0-42","readytime":"1377958901","srcmd5":"1ac07842727acaf13d0e2b3213b47785","rev":"2","revtime":"1377958897","reason":"new build","bcnt":"1","verifymd5":"1ac07842727acaf13d0e2b3213b47785","workerid":"build12","time":1377961012}'
 [x] 'opensuse.obs.package.commit':'{"project":"CopyTest","package":"test","comment":"","user":"unknown","files":"Added:\\n  my_file\\n\\n","rev":1,"time":1377006131}'
 [x] 'opensuse.obs.repo.publish_state':'{"project":"home:tom","repo":"home_coolo_standard","state":"publishing","time":1377006134}'
 [x] 'opensuse.obs.package.version_change':'{"package":"test","comment":"","files":"Added:\\n  package.spec\\n\\n","project":"CopyTest","rev":2,"newversion":"1.0","user":"unknown","oldversion":"unknown","time":1377006131}'
 [x] 'opensuse.obs.project.update_project_config':'{"project":"BaseDistro","sender":"unknown","time":1377961010}'
 [x] 'opensuse.obs.repo.published':'{"project":"home:Iggy","repo":"10.2","time":1377961012}'
 [x] 'opensuse.obs.package.update':'{"project":"Apache","package":"Taskjuggler","sender":"king","time":1377006132}'
 [x] 'opensuse.obs.package.create':'{"project":"kde4","package":"kdelibs","sender":"unknown","time":1377961009}'
 [x] 'opensuse.obs.project.create':'{"project":"home:adrian","sender":"unknown","time":1377961009}'
 [x] 'opensuse.obs.project.update':'{"project":"BaseDistro","sender":"king","time":1377006139}'
 [x] 'opensuse.obs.package.commit':'{"project":"BaseDistro","package":"pack1","user":"unknown","files":"Added:\\n  my_file\\n\\n","rev":"1","time":1377961010}'
 [x] 'opensuse.obs.package.service_fail':'{"project":"home:Iggy","package":"TestPack","rev":"2","comment":"trigger service run","error":"service daemon error:\\n400 remote error: /usr/lib/obs/service//not_existing  No such file or directory","user":"Admin","time":1377961012}'
@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

Note that https://github.com/openSUSE/open-build-service/blob/master/src/backend/plugins/notify_rabbitmq.pm doing something similiar - but its very suboptimal to require on the src server. But in case Intel is still using this, we might better support it directly in the API. But that requires some more config.

The config we require at minimum is AMQP URL and exchange name.

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Jan 23, 2017

@coolo Can this also be used to support something like fedmsg? OpenQA also has support for this, and I'd prefer to be able to use it with OBS, too...

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

Not directly. fedmsg is using 0mq

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

But once the events are out, you can have a proxy to whatever other service you want - with little extra effort.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

@Conan-Kudo as you are the last to touch the backend plugin - are you using that?

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

And while we're it - there could be more events interesting to tools. When I created the current event system I only added events that are interesting as email (or existed before :).

E.g ReviewStateChange is one of those that are too spammy for humans, but quite interesting for tools

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Jan 23, 2017

@coolo I have been working on something to plug it into fedmsg, yes. I mainly fixed the AMQP backend plugin because of issues with the original dependency.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

ok, but that supports my suspicion that we can kill the backend notification plugins completely. To support fedmsg you need a rabbitmq on localhost and a little pyton script subscribing to all amqp events and acting as 0mq endpoint: http://zeromq.org/docs:welcome-from-amqp

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Jan 23, 2017

@coolo I don't disagree (the backend services are a bit strange). But providing events that can be sent along the wire does not mean they need to be tied specifically to AMQP. Those two aspects can be separated.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

AMQP and ZMQ are 2 different gems with 2 different APIs and 2 different ways to test them. So I don't think we want to support everything - especially if proxying is easy

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 23, 2017

But I know openSUSE needs AMQP - and this is just a proof of concept for a feature request.

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch 2 times, most recently from 007c61a to d6502f2 Jan 25, 2017

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 25, 2017

I killed the backend notification plugins now

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from d6502f2 to 3efabc2 Jan 25, 2017

@asdil12

This comment has been minimized.

Copy link
Member

asdil12 commented Jan 26, 2017

@coolo: For a working auth you will need a change like this: os-autoinst/openQA#1202

See also: https://w3.suse.de/~dheidler/amqp.html

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from 3efabc2 to 54d98f7 Jan 30, 2017

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 30, 2017

With a proper config this works now. Off by default. The options to mocking AMQP are a bit overwhelming, so I need some feedback here

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Jan 30, 2017

@coolo I do not get why you are tightly coupling the message emitter with AMQP. That is not necessary. You want to move the message emitter from the backend to the frontend? Fine. But nothing says it needs to be coupled tightly with AMQP.

What if OpenSUSE wants to change from AMQP to something else (as Fedora did years ago)? It shouldn't take much work to split this so that it has a generic message emitter and an AMQP module. That way, someone like myself can reuse the emitter and write a fedmsg module, an MQTT module, or something else altogether.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 30, 2017

IMO you need to have the depencies in Gemfile.lock - that makes it so much less fun to support multiple ways

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Jan 30, 2017

replace IMO with 'as far as I know' - this is less about my opinion than about facts :)

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Jan 30, 2017

@coolo Your Rails app need not be a monolith. The generic emitter would be part of the main application, while you could have a separate module repository (with its own Gemfile and lockfile) that plugs in the AMQP bits.

@adrianschroeter

This comment has been minimized.

Copy link
Member

adrianschroeter commented Jan 30, 2017

Blocked until 2.8 branch got created

@adrianschroeter

This comment has been minimized.

Copy link
Member

adrianschroeter commented Mar 1, 2017

We have the 2.8 branch, so we could merge it, but conflicts need to get resolved.

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch 2 times, most recently from 6f80efb to a73bd7f Mar 17, 2017

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from a73bd7f to 9be7cc4 Mar 27, 2017

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Mar 27, 2017

I resolved conflicts, but the final polish needs to come from you guys

@hennevogel

This comment has been minimized.

Copy link
Member

hennevogel commented Mar 28, 2017

@coolo can you elaborate on "final polish" please?

@mdeniz mdeniz added the Frontend 👻 label Apr 5, 2017

@hennevogel

This comment has been minimized.

Copy link
Member

hennevogel commented May 17, 2017

@Conan-Kudo you have some specific generic message emitter in mind?

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented May 17, 2017

@hennevogel Basically, it could emit generic JSON messages and topics, and plugins for various messaging systems (MQTT, AMQP, Fedmsg-ZeroMQ, etc.) can set up around those to support emitting messages in the form those systems want.

For example, a fedmsg emitter would emit the JSON message body and the message topic with the fedmsg specific "modname". AMQP emitters would be able to do something similar. And so on...

That maximizes the flexibility and ensures that you're not stuck with one system forevermore for infrastructure messaging.

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented May 17, 2017

An example thing produced by the generic message generator would be something like this:

topic: obs.build.state.change
msg: { project: 'foo', package: 'bar', version: '1', release: '2.1', state: 'building', instance: 'build.opensuse.org' }

Various plugins/adapters for different messaging systems can plug those pieces of information into something useful.

@asdil12

This comment has been minimized.

Copy link
Member

asdil12 commented May 17, 2017

It would be helpful to stick to this layout: https://github.com/openSUSE/suse_msg/blob/master/amqp_infra.md#topic-format

So in your example:

suse.obs.build_state.change

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented May 17, 2017

@asdil12 Yes, that works too. I didn't know about that and just came up with something off the cuff. ;)

@hennevogel

This comment has been minimized.

Copy link
Member

hennevogel commented May 17, 2017

@Conan-Kudo okay but you don't have a ready made ruby solution in mind. I understand your need but we are not going to implement this ourselves, sorry. We have bigger fish to fry...

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from 9be7cc4 to a4af63a May 27, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 27, 2017

Codecov Report

Merging #2592 into master will decrease coverage by 0.04%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2592      +/-   ##
==========================================
- Coverage   88.93%   88.89%   -0.05%     
==========================================
  Files         263      263              
  Lines       17590    17607      +17     
==========================================
+ Hits        15643    15651       +8     
- Misses       1947     1956       +9
Flag Coverage Δ
#api 83.63% <84.21%> (-0.05%) ⬇️
#rspec 65.91% <88.57%> (+0.02%) ⬆️
#webui 63.48% <89.18%> (-1%) ⬇️
[api] Do not use class variables but global variables
bunny will start a thread - and it might be good to actually start this
thread on initial startup, but this would be specific to the web server
used

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from a4af63a to 0016b97 May 27, 2017

@coolo

This comment has been minimized.

Copy link
Member

coolo commented May 27, 2017

I rebased - just for the fun of it

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from 454131c to f2bff30 May 27, 2017

@coolo coolo force-pushed the coolo:send_to_rabbitmq branch from f2bff30 to ef3dfd8 May 27, 2017

@coolo

This comment has been minimized.

Copy link
Member

coolo commented May 27, 2017

travis passes now

@hennevogel

This comment has been minimized.

Copy link
Member

hennevogel commented Sep 29, 2017

partly (the send to rabbitmq part) superseded by #3924

@mdeniz

This comment has been minimized.

Copy link
Contributor

mdeniz commented Oct 5, 2017

Closing it, thanks @coolo we can follow up this work in other PRs

@mdeniz mdeniz closed this Oct 5, 2017

@coolo coolo deleted the coolo:send_to_rabbitmq branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment