- Added parameter to control the interval
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> This PR adds a timer that its interval can be controlled with the param `ping_interval`.
The timer pings all registered rtpengines and disables if any are found not responsive and vice versa. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4016
-- Commit Summary --
* Add timer to ping rtpengines
-- File Changes --
M src/modules/rtpengine/rtpengine.c (50)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4016.patch https://github.com/kamailio/kamailio/pull/4016.diff
@xkaraman pushed 1 commit.
1f6fcef35c5af052115a6316b594a48e5c4e24be rtpengine: Re-enable down servers (but not disabled ones)
Hi, we have a similar task for this on our TODO list. There was this idea that the timer ping should take the pinging responsability from routing processes.
Here is some improvement suggestions, if they apply to you too: 1. Register timer callback only if "rtpengine_ping_interval" modparam is > 0 (default can be 0, disabled) 2. In rtpp_test() function check if "force" param is == 0 and if "rtpengine_ping_interval" modparam is > 0, then return imediately => this should take the pinging responsability from SIP routing processes 3. Please check what happens with build_rtpp_socks() inside the ping timer callback, when there is at least 1 rtpengine in the list down. I think sockets will be re-built each time timer triggers
Thank you, Stefan
Hey there @stefan-mititelu-idt,
Thanks for taking time to review this.
Indeed point 1 and 2 makes sense, and I will implement them as well.
About point for the `build_rtpp_socks()`. What is the concern exactly here?
I can see that it may rebuild the sockets continuously and I favor finding a way to modify this. But even before these changes, it can happen since this function is called in `select_rtpp_node()`, which is called in every `rtpp_function_call`, meaning basically in every usage of `rtp_manage` and similar functions.
Correct me if have a wrong understanding and see some other issue.
Thanks, Xenofon
@xkaraman first commit doesn't follow the [commit-guidelines](https://www.kamailio.org/wikidocs/devel/git-commit-guidelines/#commit-messag...)
@xkaraman pushed 2 commits.
51037dc20cb236d23cb3a0c25e3c789fdabbfbfd rtpengine: Add timer to ping rtpengine 5d0ff702c797baecbc2684775e2d9cc1eb1bd497 rtpengine: Re-enable down servers (but not disabled ones)
@xkaraman I think you are correct for nr 3. Please ignore it.
Thanks, Stefan
@stefan-mititelu-idt commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
I think this new if condition needs to be after the existing if(isdisabled==0) condition. Otherwise may interfere with the "found:" label in select_rtpp_node_new() and automatically disable a found node
@stefan-mititelu-idt commented on this pull request.
@@ -2176,6 +2184,11 @@ static int mod_init(void)
} }
+ /* Enable ping timer if interval is positive */ + if(rtpengine_ping_interval > 0) { + register_timer(rtpengine_ping_check_timer, 0, rtpengine_ping_interval);
question: after this timer callback is registered, when will it trigger first time? Is it right away, or after rtpengine_ping_interval seconds?
@xkaraman commented on this pull request.
@@ -2176,6 +2184,11 @@ static int mod_init(void)
} }
+ /* Enable ping timer if interval is positive */ + if(rtpengine_ping_interval > 0) { + register_timer(rtpengine_ping_check_timer, 0, rtpengine_ping_interval);
I can't find documentation about it but it seems to be to be after interval seconds and not right away.
@stefan-mititelu-idt commented on this pull request.
@@ -2176,6 +2184,11 @@ static int mod_init(void)
} }
+ /* Enable ping timer if interval is positive */ + if(rtpengine_ping_interval > 0) { + register_timer(rtpengine_ping_check_timer, 0, rtpengine_ping_interval);
Thank you. I think this should still be OK because I see rtpengines are forcefully pinged once, in child_init(), when building sockets, depending on "ping_mode" modparam. :+1:
@xkaraman pushed 3 commits.
f8d0de772e480f7b782d464c370d5ea128fe0839 rtpengine: Add timer to ping rtpengine 39fa35d0ecd98a7c472ee63c5cad97ee6cf2326e rtpengine: Re-enable down servers (but not disabled ones) 0e883bcb55af5878db9cb53d5e2e872a74e7d7f8 rtpengine: Add brief comments
@xkaraman pushed 2 commits.
9d823e99599ea039524e84a016a9b2e9d060fcd5 rtpengine: Add brief function descriptions 429aed5f7e160485f135fd2894cb4436ebaeb270 rtpengine/docs: Add ping_interval docs
@xkaraman pushed 2 commits.
3b994ff43519db59c3d8b2c4ae004eac5b9f4a42 rtpengine: Add brief function descriptions ff6979700946a0134085ca8df2febbefd8eebc05 rtpengine/docs: Add ping_interval docs
@xkaraman pushed 4 commits.
508d88fea66ac63bab8e1e7e99736dc5b3c9e824 rtpengine: Add timer to ping rtpengine fe5d139c019ecd108fa569f5d87010db6dc25b3a rtpengine: Re-enable down servers (but not disabled ones) 2896edca41f8b76c4c835429c82212ed9a8fe590 rtpengine: Add brief function descriptions 39f91edb6b91403aa84569450c19c8a434b6738d rtpengine/docs: Add ping_interval docs
@xkaraman commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
I think you are right, since this function has a bit different semantics that `rtpp_test_ping()`.
This function should return whether the node `isdisabled` or not (not if it's reachable since other checks are in play, like pings or already enabled).
I have made some modification to the check of `ping_interval` and now we return whatever status we already were if ping timer is enabled.
Does this make more sense?
@stefan-mititelu-idt commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
I like this change, but I think you should still place it inside "if (force==0) {" condition. Otherwise, when timer pinging is enabled, will cancel all the forced pings of rtpp_test(). More specifically will cancel the effect of (at least) these 2 modparams: 1. aggressive_redetection -> maybe should still be ok since the purpose of the timer pinging is to disable pinging done by the routing processes, but I think should be documented 2. ping_mode -> rtpengines won't be able to be pinged first time when kamailio starts, but will be pinged first time when timer triggers (e.g. after the initial rtpengine_ping_interval seconds). Will all be considered available untill that. Please check the child_init()->build_rtpp_socks()->rtpp_test() flow for this.
Please let me know what you think about this.
Thanks, Stefan
@xkaraman commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
I am currently thinking that if a forced ping is required, regardless of ticks, disabled or not, and so on, the function `rtpp_test_ping` should be used. It does exactly that and only that.
So for 1. It seems to be ok. 2. We should use the function above instead of `rtpp_test()`
Thoughts on this?
@stefan-mititelu-idt commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
I think rtpp_test_ping() and rtpp_test() are very similar, except rtpp_test() skips sending the actual ping in some cases. Definitely first one can replace the second one when force pinging is required (maybe additionally checking manually disabled rtpengines before calling it -> RTPENGINE_MAX_RECHECK_TICKS)
I think, a much more simpler change would be the previous one suggested. So pings will happen additionally, besides the timer pings, only when they are force pings. I wonder if you see any problems with it, that I am missing?
@xkaraman commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
Well, i don't really see any problems no. Adding the interval check in `force==0` block can work as expected probably.
What I suggested was just a thought, that might have simplified how all these are handled and more logically structured.
I will update it accordingly.
@xkaraman pushed 4 commits.
98202587325b619df36970cc086419042267f6cc rtpengine: Add timer to ping rtpengine 0458f4cd5dd1e2507e3a3c817815e3f12945b550 rtpengine: Re-enable down servers (but not disabled ones) df0d4ce788e03888eaeef0553f26c7d84999b832 rtpengine: Add brief function descriptions 65e714cc352aa0c38f7f243b08c828bb2b64a9ba rtpengine/docs: Add ping_interval docs
@xkaraman pushed 2 commits.
f93a7a78ba714a6dd830cc17b09c323dca7bf080 rtpengine: Add doxygen function descriptions 0a981e20115a3def935206118097ad8603feed62 rtpengine/docs: Add ping_interval docs
@xkaraman commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
Updated.
@stefan-mititelu-idt commented on this pull request.
@@ -3549,6 +3606,10 @@ static int rtpp_test(struct rtpp_node *node, int isdisabled, int force)
return 1; } if(force == 0) { + /* If ping_interval is set, the timer will ping and test + the rtps. No need to do something during routing */ + if(rtpengine_ping_interval > 0) + return 1;
Thank you. Looks good to me.
Untested but looks OK to me. I can merge if there are no objections.
@xkaraman pushed 4 commits.
8dbd95bc3ddf5b5e59a0eea801be0f3809105fed rtpengine: Add timer to ping rtpengine 22dd98aafa5ab109e9230cd39baeebbfede7b34f rtpengine: Re-enable down servers (but not disabled ones) b726f7b5aec4038ee1d2e587fa5b5a50a7e127ec rtpengine: Add doxygen function descriptions aa4785cd545142331847cb57ea242f167a651e5c rtpengine/docs: Add ping_interval docs
@rfuchs Thanks for the quick review. I am merging it!
Merged #4016 into master.
-
Richard Fuchs -
Stefan Mititelu
-
Victor Seva
-
Xenofon Karamanos