4 Dec
2015
4 Dec
'15
11:28 a.m.
- Add 'disabled' and 'weight' column name
- Add kamctl fifo nh_reload_rtpp command to reload nodes from db table (update state or
add new ones; does not delete the nodes that are not in the table anymore)
- Add sanity URL checks. Ignore URL weight for database nodes; consider the one from the
column
- Add rtpengine DB doku, db-tables doku and table creation scripts
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/429
-- Commit Summary --
* rtpengine: Add db 'disabled' column
* rtpengine: Add setid_col modparam
* rtpengine: Update doku for rtpengine database
* rtpengine: Add setid_default modparam
* rtpengine: Add kamctl nh_reload_rtpp
* rtpengine: Check set uniqueness for rtpp node
* rtpengine: Add db 'weight' column
* rtpengine: Update kamailio-db-devel .xml
* rtpengine: Check node address when adding to set
* rtpengine: Update kamailio-db-devel db scripts
-- File Changes --
A lib/srdb1/schema/kamailio-rtpengine.xml (12)
A lib/srdb1/schema/rtpengine.xml (60)
M modules/rtpengine/doc/rtpengine_admin.xml (212)
M modules/rtpengine/rtpengine.c (207)
M modules/rtpengine/rtpengine.h (9)
M modules/rtpengine/rtpengine_db.c (32)
A utils/kamctl/db_berkeley/kamailio/rtpengine (10)
M utils/kamctl/db_berkeley/kamailio/version (2)
A utils/kamctl/db_sqlite/rtpengine-create.sql (9)
A utils/kamctl/dbtext/kamailio/rtpengine (1)
M utils/kamctl/dbtext/kamailio/version (1)
A utils/kamctl/mysql/rtpengine-create.sql (9)
A utils/kamctl/oracle/rtpengine-create.sql (17)
A utils/kamctl/postgres/rtpengine-create.sql (9)
M utils/kamctl/xhttp_pi/pi_framework.xml (32)
A utils/kamctl/xhttp_pi/rtpengine-mod (23)
A utils/kamctl/xhttp_pi/rtpengine-table (9)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/429.patch
https://github.com/kamailio/kamailio/pull/429.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429
10 Dec
10 Dec
1:33 p.m.
Updated pull request. Solved conflicts after c73b9cd8b5f8074c43fd2a30f7b7e6df97208a3d.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-163597681
11 Dec
11 Dec
3:34 p.m.
Updated pull request:
- 'kamctl fifo nh_show_rtpp all' reflects the rtpengine table state (for
additions, updates and deletions)
- added tab code indentation
Upon deletion, the node is disabled permanent and hidden for printing. Thus, using
allow_op, the sessions can still finish for the table deleted nodes.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-163950125
14 Jan
14 Jan
3:32 p.m.
@rfuchs , did you already have time to take a look on this pull request?
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-171658291
4:06 p.m.
Only briefly. Gonna look at it now, sorry for the delay
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-171666775
4:13 p.m.
No problem, didn't intend to hustle you. Just wanted to check the state as sometimes
things may slither out-of-sight unintendedly :smirk:
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-171668479
5:19 p.m.
OK, so one thing I've noticed is that it doesn't seem to take concurrency into
account. Previously the linked lists were created at startup before Kamailio forks and
then left alone, but modifying a linked list during runtime with multiple processes
accessing the shared memory simultaneously is not safe. There are ways to make this safe
using atomic ops and CAS (not sure if Kamailio really supports this), or alternatively
locking must be used. This would also make it possible (and preferable) to support full
deletion of allocated structs (sets and nodes) instead of just marking them as deleted.
The var rtpp_set_list deserves special mention as it's initialized to NULL and only
updated during startup if any proxy sets are actually defined. If this is not the case,
then the var will remain NULL. A later reload from DB would then cause the MI process to
allocate all the structs and would set rtpp_set_list to non-NULL, but the other processes
won't see this change as the var itself is not in shared memory (only its contents
are).
As a minor note, Kamailio provides the macro STR_EQ(), which does the same thing as
str_cmp().
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-171688307
15 Jan
15 Jan
4:25 p.m.
You are right with the atomic ops. Even if there's only the MI process who does the
changes, it should do them atomically. I am thinking of using per rtpp_set locks.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-171990615
19 Jan
19 Jan
3:20 p.m.
I've spotted another problem, with ```rtpp_socks``` variable, because it's pkg
memory initialized at startup. So all the processes will allocate ```rtpp_no``` sockets;
after reload, nothing happens to the sockets, so the newly added machine won't be
taken into consideration. Do you have any idea how to solve this (i.e. after ctl reload,
rebuild rtpp_socks, but still keep them in private memory)?
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-172867716
4:35 p.m.
Ah yes, good catch and good question. A quick solution would be to move initialization of
rtpp_socks out of child_init() and instead do it on demand whenever it's needed, i.e.
just before sending a request and only when something in the list of proxies has changed
(or if it hasn't been initialized yet).
The code then just needs a way to detect changes to the lists. This can be done for
example by keeping a global running counter in shared memory (protected by a lock) and a
private per-child counter. Every time a change is made, the global shared counter is
increased by one and every time a child wants to send a request, it compares its private
counter with the shared one and if it doesn't match, it rebuilds its rtpp_socks.
Another option would be to use pnode->idx and have each child keep the size of its
rtpp_socks array. If a child wants to send a request to a node with an idx larger than its
array size, it grows and rebuilds the rtpp_socks array as needed. This would only catch
new additions to the linked lists but no other changes.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-172890382
21 Jan
21 Jan
4:44 p.m.
Updated pull request with the following:
- add locks per rtpp_set_head and per rtpp_set
- make rtpp_no shm counter (which only counts additions in the db because deletion is not
handled); add rtpp_socks_size per process counter; rebuild rtpp_socks before selecting
node; reassign active_set.
I basically tested it and it works decent, taking in consideration the newly added nodes
when selecting the node. We will be testing this code in our system.
Deletion of shm nodes won't be handled in this pull request mainly because of
interaction with rtpengine hash table(in hashtable only pointers to shm nodes are kept to
save memory).
Additional code feedback would be highly apreciated.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173611266
5:43 p.m.
build_rtpp_socks needs to clean up previously allocated sockets. Right now it always
creates new sockets for each node and leaks those allocated in previous runs.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173629529
22 Jan
22 Jan
9:54 a.m.
I think build_rtpp_socks() does not leak pkg memory because it uses pkg_**re**alloc;
comparing the old rtpp_socks with the new rtpp_socks, they point to the same address.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173851436
2:57 p.m.
It's not leaking memory, it's leaking the file descriptors from the sockets.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173927308
4:02 p.m.
Updated pull-request, solving all the above comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173943311
7:12 p.m.
Hi, build_rtpp_socks still needs fixing I think. There should be a close() somewhere in
there.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-173997202
26 Jan
26 Jan
12:25 p.m.
Hi @rfuchs,
Don't you agree that rtpengine.c build_rtpp_socks() lines 1795 - 1800 should take care
of closing the sockets that you mention in your last comment?
Thank you
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-174962943
5:56 p.m.
Oh yes, a second loop... Totally didn't see that, sorry.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#issuecomment-175114627
6:34 p.m.
Merged #429.
---
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/429#event-526823458
3019
days inactive
3072
days old
18 comments
4 participants
participants (4)
-
Lucian Balaceanu -
Pawel Kuzak
-
Richard Fuchs
-
Stefan Mititelu