- 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
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
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
@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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Merged #429.
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/429#event-526823458
-
Lucian Balaceanu -
Pawel Kuzak
-
Richard Fuchs
-
Stefan Mititelu