rtpengine: hash table to keep the selected nodes #390
Conversation
Shared memory hash table with global hashtable lock. Add state maintaining the selected rtp node, for a given callid. Hashtable entry expiration time configurable using hash_entry_tout modparam. The actual deletion happens on the fly while insert/remove/lookup are called. Updated doku.
|
Basically tested it, with focus on entries expiration. Further tests will follow. What do you think about current expired entries deletion implementation? Do you have any other advices/proposals? |
This is my understanding of the current shared memory node list implementation. Correct me if I'm wrong.
Allow configurable table size. Updated doku.
Print the total number of hash entries in the hash table, at the given moment. Updated doku.
When the param is enabled, allow current sessions to finish and deny new sessions for manually deactivated rtpengine nodes via kamctl i.e. "disabled(permanent)" nodes. This is useful when deactivating the nodes for maintenance. Default value is 0, so the current behaviour is maintained (e.g. don't send commands to any deactivated proxy). Updated doku.
|
Updated pull request. Added some modparams and kamctl for the hashtable. Added a modparam to allow session deletion to manually disabled nodes (useful when performing maintenance). Updated doku. In my opinion tests were ok. Tested this for 100 calls with media, 10 calls/s rate. Sessions were deleted successfully upon BYE. No memory leaks found at the hash table level (used "kamcmd mod.stats rtpengine shm" for this). |
|
What would happen when there is a restart? I find the feature useful overall, any limitations need to be documented when it is enabled. Hopefully @rfuchs can comment from his point of view and merge when all is ok. |
|
On a restart (i.e. rtpengine's mod_destroy() is called), rtpengine_hash_table_destroy() is called which frees the entire shm hashtable. Possible limitations:
What is the current known limitation for kamailio (calls/sec) ? |
|
I was looking more at the perspective of the items in hash table being lost and if the restart was in a middle of the call, then after restart, when the BYE is processed, which rtpengine is updated? |
|
Hmm.. I think I get the point. If the kamailio restarts in the middle of a call, the hashtable is flushed. When the BYE comes, no entry is found for that callid and no rtpengine is updated. |
|
One approach to this would be to fallback to the stateless behaviour when no entry is found in the hashtable and just select a rtpengine based on the hashed callid (with 100% matching the rtpengine machine for that call if none crashed in the meantime). |
|
|
||
| /* Insert the key<->entry from the hashtable */ | ||
| if (!rtpengine_hash_table_insert(&callid, entry)) { | ||
| LM_ERR("rtpengine hash table fail to insert node=%.*s for calllen=%d callid=%.*s", |
There was a problem hiding this comment.
The error case needs some shm_free() I think
|
Why not just require the dialog module and store the selected rtpproxy in the dialog? What happens on spiraled/hairpin calls? The call-id alone does not seem specific enough. |
|
I would not like a dependency on dialog module, it is quite common to run rtp relay on edge proxy which is typically more or less a lightweight load balancer -- dialog management on those nodes will add a lot of overhead. Fallback to hasing callid after a restart seams reasonable for me (giving the current behaviour). For spiralled calls, iirc, there is an option to provide via branch to rtpproxy/rtpengine, but this is independent of selecting what rtpproxy/rtpengine to use. |
|
We also thought of using dialog module for keeping the node. The main reason against it was to keep the rtpengine module as independent as it could be. Maybe for spiraled calls this could be extended to consider also the from/to-tag when calculating the hash index ?! |
|
Alex writes:
Why not just require the dialog module and store the selected rtpproxy
in the dialog?
dialog module is a complicated mess and its use cannot be warranted just
for enabling this feature.
…-- juha
|
|
So, the dialog module is a no-go/ Have you considered just putting the selected rttproxy in an RR parameter? For me that has worked perfectly for years now. |
|
Alex writes:
Have you considered just putting the selected rttproxy in an RR
parameter? For me that has worked perfectly for years now.
that is what i'm doing also:
;pm[=setid]
…-- juha
|
|
That is only the setid not the selected rtp relay within the set. Afaik, there is no way to specify the rtp relay within a set from config file. |
|
Daniel-Constantin Mierla writes:
That is only the setid not the selected rtp relay within the
set. Afaik, there is no way to specify the rtp relay within a set from
config file.
it works if you have only one relay per set.
…-- juha
|
|
My understanding was that the patch was adding support for keeping the relation between the selected rtpengine and the callid, when there are more than one rtpengine per set. If one of the rtpengine in the set is unavailable, then the current hashing selection becomes inconsistent if the rtpengine turns back available. If there is only one rtpengine per set, then no hashing selection is done. |
- shm NULL checks and free already alloc'ed shm - default entry tout to 3600 sec - return node only, not the whole entry - zero shm hashtable parts - lookup and select new node if lookup fails; this is done for all commands and assures fallback behaviour - change void to struct specific - make set_rtp_inst_pvar() static -> used only in rtpengine.c - fix typos rtpproxy vs rtpengine
- hash table entry contains callid, viabranch - hash table lookup based on callid, viabranch (useful for branching scenarios); keep doing the hash table remove right away - remove op param when select_rtpp_node(); not needed
|
Updated pull request. Some of the most important updates:
|
|
Accidentally pushed all the commits when trying to "git push upstream p_usrloc_NULL_checks master"; forgot the ":". Shall I re-push the old files? (can't push -f a reset on previous commit due to locking). |
|
git revert is your friend |
|
Done. I reverted all your commits |
|
Thank you; didn't think of it :) |
|
@linuxmaniac -- you should add the commands you executed in the wiki: It will be useful for devs getting into git. |
Done |
Don't dup a NULL str->s to avoid warning message. This happened usually when viabranch is not used(default being NULL).
If allow_op modparam enabled, send commands to the disabled machines for the existing call. So far this was done only for manually deactivated machines. This is useful because there might be cases of proxy timeout, cases when you may want to still allow the operations for the existing calls.
This will further increase rtpengine's hash table access.
For consistency with the per row locks, statistics should be also per row.
- struct rtpengine_hash_table now contains the table size. - rename the entry_list to row_entry_list
|
Updated the pull request with focus on the hastable API:
|
| rtpengine_hash_table->row_entry_list = shm_malloc(rtpengine_hash_table->size * sizeof(struct rtpengine_hash_entry*)); | ||
| if (!rtpengine_hash_table->row_entry_list) { | ||
| LM_ERR("no shm left to create rtpengine_hash_table->row_entry_list\n"); | ||
| rtpengine_hash_table_destroy(); |
There was a problem hiding this comment.
A minor nitpick: These calls to _destroy() here won't ever actually do anything, because the _sanity_checks() will always fail.
There was a problem hiding this comment.
Right. The _destroy() should do sanity checking, with memory free, when possible. Will correct this.
|
Other than that, looks good to me. |
_destroy() sanity checking, with memory free, when possible: - alloc the locks first. - free the locks last. - consider content already hadled for a NULL lock (or NULL lock vector). - make _free_row_lock() static.
Shared memory hash table with global hashtable lock.
Add state maintaining the selected rtp node, for a given callid.
Hashtable entry expiration time configurable using hash_entry_tout modparam.
The actual deletion happens on the fly while insert/remove/lookup are called.
Updated doku.