-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add new Drain feature #1529
base: master
Are you sure you want to change the base?
Add new Drain feature #1529
Conversation
-when coturn server is in drain mode -current allocations will continue to work as usual -new allocations will be rejected with a 300 (Try Alternate) response -when all allocations go away, then coturn will shutdown -Enable drain mode with either -signaling SIGUSR1 -turn_admin_server "drain" CLI command
FYI - This contribution is from Wire. https://wire.com/ |
-undo move of dtls_listener_relay_server to header file, put back in c file -remove drain mode enum - multi-staged approach refactored -move code to enable is_draining on turn servers to netengine.c where access to all turn servers (including ones for TCP relays) are available -use _Atomic for global_allocation_count to avoid mutex use -use 403 error code instead of 300 for new allocations when in drain mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from minor grumbling about stdbool.h and thinking std::atomic<size_t> would be a better fit on MSVC (i believe coturn always compiles as C++ on MSVC on purpose, but i'm not 100% sure about that) this seems like an excellent contribution.
Thanks for all your time looking at this, really appreciate it. A number
of great changes came out of it.
As for the minor grumbling... :)
1. I'm happy to change to use bool (vs the current implementation that is
consistent with the rest of the code) - if there is consensus from project
maintainers on this.
2. I'm running out of patience today with the atomic stuff, just trying to
get the interlocked solution took me 7 or so commits, it is tedious to get
OS sensitive code right without reasonable access to all the various
compilers. But I am happy to take another run at it, if needed for
acceptance. :)
Thanks again for your time.
Scott
…On Thu, Jun 27, 2024 at 3:02 PM Michael Jones ***@***.***> wrote:
***@***.**** approved this pull request.
Aside from minor grumbling about stdbool.h and thinking
std::atomic<size_t> would be a better fit on MSVC (i believe coturn always
compiles as C++ on MSVC on purpose, but i'm not 100% sure about that) this
seems like an excellent contribution.
—
Reply to this email directly, view it on GitHub
<#1529 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVU2UL5Z5AXILN6TICYCA3ZJROUVAVCNFSM6AAAAABJ4U7ZW2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBWGI4DQOJXGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Conversion to use bool is complete, I think this PR may be ready for acceptance at this point. Thanks for all the feedback. Scott |
Hi Guys - just checking in. Is there anything else you need changed on this PR for it to be accepted? Thanks - I appreciate your time! |
Sorry @sgodin somehow this slipped in between everything else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgodin a few questions on the PR: code deleted I do not understand and about logging. Thanks!
src/apps/relay/userdb.c
Outdated
|
||
#ifndef _MSC_VER | ||
global_allocation_count++; | ||
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "Global turn allocation count incremented, now %ld\n", global_allocation_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be INFO
DEBUG seems like a better fit - would we need a new log line for every allocation? We already have a log line for every allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I will make that change.
src/apps/relay/userdb.c
Outdated
|
||
#ifndef _MSC_VER | ||
global_allocation_count--; | ||
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "Global turn allocation count decremented, now %ld\n", global_allocation_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be INFO when the server is in drain mode - I think it will be useful to see in logs that the draining is happening...
But not while in regular operation. We already have too much logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll make a change for that.
@@ -5010,7 +5017,7 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io | |||
|
|||
server->response_origin_only_with_rfc5780 = response_origin_only_with_rfc5780; | |||
|
|||
server->respond_http_unsupported = respond_http_unsupported; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find - must have been a bad resolution of a merge conflict. I'll fix it up.
-fix merge error in ns_turn_server.c
Okay I've made all the changes. However, I can't quite understand why I'm all of a sudden getting clang-format errors on TurnMsgLib.h. Since I haven't modified this file for my changes, I've even tried using the file checked into master, but it still has issues. If I make the changes clang-format wants, I ended up with merge conflicts. Perhaps I need to merge coturn master with my branch again? Does anyone have any insight? Probably something simple, that I'm just missing. :) |
Ah I see lint / clang-format is failing on master too. I can't identify a change that would have caused this. Perhaps the clang-format version was bumped in the CI pipeline?? |
@eakraly I have addressed your points with this PR. Can it be merged in now? I'm a little baffled at the lint problem (see previous comments), does that need addressing first, or is it a problem that is bigger than this PR? |
Add new Drain feature
-when coturn server is in drain mode
-current allocations will continue to work as usual
-new allocations will be rejected with a 403 (Forbidden) response
-when all allocations go away, then coturn will shutdown
-Enable drain mode with either
-signaling SIGUSR1
-turn_admin_server "drain" CLI command
This contribution is from Wire. https://wire.com/