Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RabbitMQ Transport Reconnect Logic #2651

Merged
merged 12 commits into from
May 10, 2021
Merged

RabbitMQ Transport Reconnect Logic #2651

merged 12 commits into from
May 10, 2021

Conversation

chriswiggins
Copy link
Contributor

Hey Janus team! 👋

As we're running Janus and RabbitMQ in Kubernetes, there are times where our Janus instance will connect to one of our RabbitMQ nodes that then eventually goes down as we're doing maintenance on our hosts. Anyone with a clustered RabbitMQ setup with a load balancer in front will have the same thing happen to them.

Currently, the RabbitMQ Event handler does reconnects, but the Transport does not. This PR brings reconnect logic into the Transport.

It looks like a large change, but realistically most of it is just moving the large connection-based logic into a new function which we call at startup, and again when we notice the connection has dropped. It also brings in RabbitMQ heartbeat functionality.

I'm opening this as a draft to get initial feedback (and while I'm doing tweaks and testing) so any feedback would be awesome 👍

@chriswiggins
Copy link
Contributor Author

chriswiggins commented Apr 29, 2021 •
edited
Loading

Looks like the checks are failing with the updated RabbitMQ-C imports - I was getting a deprecation when compiling this and using master rabbitmq-c - I will see if I can do a conditional import on those. Any pointers on how I can do that?

lminiero
lminiero reviewed Apr 29, 2021
View reviewed changes
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch! I've added a few notes inline, mostly editorial. I'll let people using the RMQ transport in production get back to you on testing and feedback.

events/janus_rabbitmqevh.c Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

As a side note, please notice one of the CI builds failed:

transports/janus_rabbitmq.c:36:10: fatal error: 'rabbitmq-c/amqp.h' file not found
#include <rabbitmq-c/amqp.h>

@lminiero
Copy link
Member

Looks like the checks are failing with the updated RabbitMQ-C imports - I was getting a deprecation when compiling this and using master rabbitmq-c - I will see if I can do a conditional import on those

Yep, sorry, I answered before seeing this message.

Any pointers on how I can do that?

Not really, no: maybe some finer check in configure.ac?

@atoppi
Copy link
Member

atoppi commented Apr 29, 2021

@chriswiggins as of CI build failure, bear in mind that the test script installs the package librabbitmq-dev from apt, so maybe the available version is not the expected one.

https://packages.ubuntu.com/focal/librabbitmq-dev

tmatth
tmatth reviewed Apr 29, 2021
View reviewed changes
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
@chriswiggins
Copy link
Contributor Author

I'm working on the reconnect logic now and over the weekend - more to follow next week. Thanks for the initial feedback!

@chriswiggins
Copy link
Contributor Author

Hey team - want to have a look at that now and see what you think?

@chriswiggins chriswiggins marked this pull request as ready for review May 4, 2021 08:44
lminiero
lminiero reviewed May 4, 2021
View reviewed changes
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added a few notes inline 👍

transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
atoppi
atoppi reviewed May 4, 2021
View reviewed changes
Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the effort @chriswiggins , I've left some comments

conf/janus.transport.rabbitmq.jcfg.sample Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Show resolved Hide resolved
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
transports/janus_rabbitmq.c Show resolved Hide resolved
transports/janus_rabbitmq.c Show resolved Hide resolved
atoppi
atoppi reviewed May 5, 2021
View reviewed changes
transports/janus_rabbitmq.c Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

lminiero commented May 6, 2021

@chriswiggins thanks for the quick fixes! This looks fine to me, so unless you wanted to do any additional changes, this is good to merge for me.

@chriswiggins
Copy link
Contributor Author

I've just done some extra testing and am now happy with the changes - I made a final couple of tweaks, mainly around the event handler logging so it's easier to see what relates to that vs the transport plugin.

If you're happy with those then I'm happy for you to merge!

@lminiero
Copy link
Member

Ack, thanks! Just FYI, I'll probably change the "connected successfully" verbosity you added recently from LOG_INFO to LOG_VERB.

@lminiero lminiero merged commit f8e8c5e into meetecho:master May 10, 2021
@chriswiggins chriswiggins deleted the rabbitmq-reconnect branch May 11, 2021 00:29
BogdanovKirill pushed a commit to 3dEYE/janus-gateway that referenced this pull request Jun 10, 2021
Squashed commit of the following:
69df84b 
commit 9c9d335
Author: Lionel Nicolas <[email protected]>
Date:   Thu Jun 10 03:04:43 2021 -0400

    Fix streaming plugin mutex unlock when disabling mountpoint (meetecho#2690)

commit 2d83e96
Author: Yurii Cherniavskyi <[email protected]>
Date:   Mon Jun 7 16:02:41 2021 +0300

    Fix SIP plugin unhold request docs typo (meetecho#2688)

commit 2cd0118
Author: August Black <[email protected]>
Date:   Mon Jun 7 01:10:49 2021 -0700

    minor adjustment to the audiobridge docs (meetecho#2687)

commit de2117e
Author: nicolasduteil <[email protected]>
Date:   Tue Jun 1 11:26:29 2021 +0200

    fix: [janus_sip] Fix "call_id" property in "missed_call" events (meetecho#2679)

commit 9eeeb38
Author: Alessandro Toppi <[email protected]>
Date:   Mon May 31 15:57:41 2021 +0200

    Fix status vector parsing for incoming twcc feedbacks (resolves meetecho#2677).

commit 8a25f6e
Merge: d3b39b9 394fb48
Author: Alessandro Toppi <[email protected]>
Date:   Fri May 28 13:29:54 2021 +0200

    Merge pull request meetecho#2675 from kmeyerhofer/actions/fix

    GH Actions, fix variable name

commit d3b39b9
Author: Lorenzo Miniero <[email protected]>
Date:   Fri May 28 11:09:30 2021 +0200

    Fixed race condition in VideoRoom

commit 394fb48
Author: Kurt Meyerhofer <[email protected]>
Date:   Thu May 27 14:52:08 2021 -0600

    Fixes variable name.

commit b45cd37
Author: Lorenzo Miniero <[email protected]>
Date:   Thu May 27 18:31:55 2021 +0200

    Clarify that libnice 0.1.18 is recommended

commit 5757a37
Author: Lorenzo Miniero <[email protected]>
Date:   Thu May 27 17:08:17 2021 +0200

    Spatial audio support in AudioBridge via stereo mixing (meetecho#2446)

commit 161fe7a
Author: Luca Barbato <[email protected]>
Date:   Thu May 27 15:29:01 2021 +0200

    Cleanup avformat-based preprocessors (meetecho#2665)

commit 7b010cd
Author: lucylu-star <[email protected]>
Date:   Tue May 25 17:09:40 2021 +0800

    Fixed broken simulcast support in VideoCall plugin (meetecho#2671)

commit 4ae44a4
Author: nicolasduteil <[email protected]>
Date:   Mon May 24 17:57:34 2021 +0200

    feat: support for custom call-id in subscribe request + add 'call_id' property to subscribe & notify related events (meetecho#2664)

commit 4294f20
Author: Lorenzo Miniero <[email protected]>
Date:   Mon May 24 11:02:48 2021 +0200

    Fixed missing macro when using pthread mutexes (fixes meetecho#2666)

commit f22ab0d
Author: Lorenzo Miniero <[email protected]>
Date:   Wed May 19 12:03:32 2021 +0200

    Fixed warning

commit b3f3f17
Author: Alessandro Toppi <[email protected]>
Date:   Tue May 18 12:10:47 2021 +0200

    Remove duplicated flag for fuzzing coverage.

commit 4a7560c
Author: nu774 <[email protected]>
Date:   Fri May 14 00:26:36 2021 +0900

    janus-pp-rec: support HEVC AP(aggregation packet) (meetecho#2662)

commit 5db4be2
Author: Lorenzo Miniero <[email protected]>
Date:   Wed May 12 17:43:43 2021 +0200

    Fixed out of bounds array access

commit 69f56f4
Author: nicolasduteil <[email protected]>
Date:   Tue May 11 14:36:22 2021 +0200

    feat: support for SUBSCRIBE expiry (Expires header) in sip plugin (meetecho#2661)

commit b047ccf
Author: Lorenzo Miniero <[email protected]>
Date:   Mon May 10 09:33:27 2021 +0200

    Fixed types

commit f8e8c5e
Author: Chris Wiggins <[email protected]>
Date:   Mon May 10 19:26:45 2021 +1200

    RabbitMQ Transport Reconnect Logic (meetecho#2651)

commit 280e8e4
Author: Lorenzo Miniero <[email protected]>
Date:   Fri May 7 12:54:30 2021 +0200

    Add per-participant recording options in AudioBridge to join API as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmatth tmatth tmatth left review comments

@atoppi atoppi atoppi left review comments

@lminiero lminiero lminiero left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chriswiggins @lminiero @atoppi @tmatth