5 Nov
2024
5 Nov
'24
4:41 p.m.
- 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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4016(a)github.com&gt;
5 Nov
5 Nov
7:51 p.m.
@xkaraman pushed 1 commit.
1f6fcef35c5af052115a6316b594a48e5c4e24be rtpengine: Re-enable down servers (but not
disabled ones)
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/89944b6291b3a2d3cfb9c9…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/89944b6291b3a2d3cfb9c9eb0b2f2768453ee814/after/1f6fcef35c5af052115a6316b594a48e5c4e24be(a)github.com>
6 Nov
6 Nov
12:35 p.m.
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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#issuecomment-2459124310
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4016/c2459124310(a)github.com&gt;
7:39 p.m.
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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#issuecomment-2460265127
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4016/c2460265127(a)github.com&gt;
8:29 p.m.
@xkaraman first commit doesn't follow the
[commit-guidelines](https://www.kamailio.org/wikidocs/devel/git-commit-guide…
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#issuecomment-2460376402
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4016/c2460376402(a)github.com&gt;
7 Nov
7 Nov
1:58 p.m.
@xkaraman pushed 2 commits.
51037dc20cb236d23cb3a0c25e3c789fdabbfbfd rtpengine: Add timer to ping rtpengine
5d0ff702c797baecbc2684775e2d9cc1eb1bd497 rtpengine: Re-enable down servers (but not
disabled ones)
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/1f6fcef35c5af052115a63…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/1f6fcef35c5af052115a6316b594a48e5c4e24be/after/5d0ff702c797baecbc2684775e2d9cc1eb1bd497(a)github.com>
4:10 p.m.
@xkaraman I think you are correct for nr 3. Please ignore it.
Thanks,
Stefan
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#issuecomment-2462201382
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4016/c2462201382(a)github.com&gt;
5:15 p.m.
@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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#pullrequestreview-2421106460
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2421106460(a)github.com>
5:22 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#pullrequestreview-2421126548
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2421126548(a)github.com>
9:55 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1833201055
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2421874945(a)github.com>
8 Nov
8 Nov
7:41 p.m.
@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:
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1834709628
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2424318107(a)github.com>
11 Nov
11 Nov
5:31 p.m.
@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
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/5d0ff702c797baecbc2684…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/5d0ff702c797baecbc2684775e2d9cc1eb1bd497/after/0e883bcb55af5878db9cb53d5e2e872a74e7d7f8(a)github.com>
5:48 p.m.
@xkaraman pushed 2 commits.
9d823e99599ea039524e84a016a9b2e9d060fcd5 rtpengine: Add brief function descriptions
429aed5f7e160485f135fd2894cb4436ebaeb270 rtpengine/docs: Add ping_interval docs
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/0e883bcb55af5878db9cb5…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/0e883bcb55af5878db9cb53d5e2e872a74e7d7f8/after/429aed5f7e160485f135fd2894cb4436ebaeb270(a)github.com>
12 Nov
12 Nov
3:01 p.m.
@xkaraman pushed 2 commits.
3b994ff43519db59c3d8b2c4ae004eac5b9f4a42 rtpengine: Add brief function descriptions
ff6979700946a0134085ca8df2febbefd8eebc05 rtpengine/docs: Add ping_interval docs
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/429aed5f7e160485f135fd…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/429aed5f7e160485f135fd2894cb4436ebaeb270/after/ff6979700946a0134085ca8df2febbefd8eebc05(a)github.com>
3:28 p.m.
@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
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/ff6979700946a0134085ca…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/ff6979700946a0134085ca8df2febbefd8eebc05/after/39f91edb6b91403aa84569450c19c8a434b6738d(a)github.com>
3:34 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1838026033
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2429474074(a)github.com>
4:14 p.m.
@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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1838081619
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2429564552(a)github.com>
5:12 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1838173666
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2429716854(a)github.com>
7:11 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1838377890
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2430052970(a)github.com>
7:42 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1838435857
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2430148860(a)github.com>
13 Nov
13 Nov
4:35 p.m.
@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
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/39f91edb6b91403aa84569…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/39f91edb6b91403aa84569450c19c8a434b6738d/after/65e714cc352aa0c38f7f243b08c828bb2b64a9ba(a)github.com>
5:10 p.m.
@xkaraman pushed 2 commits.
f93a7a78ba714a6dd830cc17b09c323dca7bf080 rtpengine: Add doxygen function descriptions
0a981e20115a3def935206118097ad8603feed62 rtpengine/docs: Add ping_interval docs
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4016/files/65e714cc352aa0c38f7f24…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4016/before/65e714cc352aa0c38f7f243b08c828bb2b64a9ba/after/0a981e20115a3def935206118097ad8603feed62(a)github.com>
5:43 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1840456436
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2433458652(a)github.com>
14 Nov
14 Nov
1:05 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#discussion_r1841926958
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/review/2435612583(a)github.com>
19 Nov
19 Nov
3:31 p.m.
Untested but looks OK to me. I can merge if there are no objections.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4016#issuecomment-2485570282
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4016/c2485570282(a)github.com>
6
days inactive
20
days old
24 comments
4 participants
participants (4)
-
Richard Fuchs -
Stefan Mititelu
-
Victor Seva
-
Xenofon Karamanos