Add new Drain feature #1529
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
|
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 |
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.
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.
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.
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.
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.
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/