Summary:	ASTERISK-02877: [patch] Reload dynamic queue members on restart
Reporter:	kevinl (kevinl)	Labels:
Date Opened:	2004-11-23 17:13:45.000-0600	Date Closed:	2008-01-15 15:15:45.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) reload_queue_members-app_queue.c.diff.txt ( 1) reload_queue_members-app_queue[1].c.diff.txt ( 2) reload_queue_members-app_queue[3].c.patch ( 3) reload_queue_members-app_queue[3][1].c.patch.txt ( 4) reload_queue_members-app_queue[4].c.diff.txt ( 5) reload_queue_members-app_queue[5].c.diff.txt ( 6) reload_queue_members-queues.conf.sample[1].diff.txt
Description:	When asterisk is restarted, all queues are reloaded which inherently removes all dynamically added queue members. As this is potentially dangerious as calls may be missed if an agent forgets to or is not properly informed that they must relogin to the queue. It would be a nice feature to have dynamic members in each queue be automatically added after a restart. I've attached a patch for app_queue.c which will record, to a file (currently /var/lib/asterisk/queue_members.db) each member in each queue. When the load_module() function is called, each member in the file will be readded back into each specific queue. ****** ADDITIONAL INFORMATION ****** I have also added a configuration option which can turn this feature off. It must be located in the [general] group in queues.conf. I have set this feature to default on. To turn it off configure queues.conf as such: [general] persistent_members = 0 I have also faxed a disclaimer.
Comments:	By: Brian West (bkw918) 2004-11-23 17:24:32.000-0600 Wouldn't it be better to store this information in the astdb like we do in chan_sip for seeding? bkw By: kevinl (kevinl) 2004-11-23 17:32:00.000-0600 Yes, that probably sounds better. I will take a look into that. thanks. By: Brian West (bkw918) 2004-11-23 17:36:52.000-0600 Also if you use the astdb it keeps state if your box crashes. bkw By: Mark Spencer (markster) 2004-11-23 23:14:10.000-0600 DB is already atomic, so it should be relatively straight forward to store entries there. By: kevinl (kevinl) 2004-11-24 13:56:52.000-0600 The latest uploaded patch now uses astdb, thanks bkw918. By: Brian West (bkw918) 2004-11-24 14:11:18.000-0600 Is a malloc really needed here? Here is what I would do (and did in cdr_odbc.c) char value[512] = ""; snprintf(value, sizeof(value),"%s/%s;%s;", cur_member->tech, cur_member->loc, penalty_str); By: kevinl (kevinl) 2004-11-24 15:05:08.000-0600 no, a malloc isn't really needed there. Putting it on the stack would be best. For some reason I was thinking I was using 4096k but it is really 4k and I didn't want to fill up the stack. I will fix. Should I keep reuploading the patches? I also forgot to add credits so... By: kevinl (kevinl) 2004-11-25 12:09:25.000-0600 Fixed up the malloc, and the possible overrun which slipped past me. Let me know if you see anything else. By: Brian West (bkw918) 2004-11-25 12:36:27.000-0600 #define PM_MAX_LEN 2048 char value[PM_MAX_LEN]; or char value[2048]; memset(value, 0, sizeof(value)); Those small tweaks... 4096 is a bit large.. 2048 should be plenty. bkw edited on: 11-25-04 12:39 By: kevinl (kevinl) 2004-11-29 16:43:34.000-0600 Changes done. Let me know if you need anything else. By: Brian West (bkw918) 2004-11-29 20:40:34.000-0600 I vote for this to be on all the time with no option to turn it off.  We do this in chan_sip. bkw By: Mark Spencer (markster) 2004-12-01 00:19:49.000-0600 This is not the correct usage of strncat (man strncat) to prevent a buffer overrun.  I suggest: snprintf(value + strlen(value), sizeof(value) - strlen(tmp_value), ... ).  We also might think about how to most gracefully handle a failure. I do like the idea that this is an option (should be off by default) because it can incur a performance penalty.  Also we need to see a patch for the config file to show how the new option is used. This is a great contribution and I'm eager to get it included! By: kevinl (kevinl) 2004-12-01 12:33:00.000-0600 ack, thanks Mark. I'll fix that strncat. The single snprintf suggestion works for me, although I think you meant: ? snprintf(value + strlen(value), sizeof(value) - strlen(value), ... ). The performance hit should be quite low as the code is only run when an agent is added/removed from a queue, and on module start, which is generally not very often. The only issue I see with keeping it default to on is if asterisk is started after it being down for a long period of time and having dynamic agents automatically added to the queues when they are not desired. As for failing gracefully, if an error occures the result will either be a partial record of agents logged into each queue stored in the astdb, or upon startup, only some of the recorded members being added back into their queues. Errors are reported to help show if anything goes wrong. At least this is how I tried to design it. By: Kevin P. Fleming (kpfleming) 2004-12-05 16:04:54.000-0600 There is one more possible problem with doing this (and it's not a problem with your code, it's a problem with the whole idea, even though I support it <G>). If Asterisk is restarted while SIP/foo.phone is a dynamic queue member, it will be re-added to the queue, even if foo.phone is longer a valid SIP peer for this Asterisk instance (if it's been removed from the SIP peer list in sip.conf or realtime database). While this is not a terribly bad problem, it does mean that the queue can have no "real" members, but callers will still be placed into it because this one member is there, even though calls cannot be delivered to the member. This particular peer also cannot remove itself from the queue, because it is longer a valid peer (well, at least on a basic level, obviously there are ways to do it in spite of the configuration change). Is there a way that this startup process could do some sort of "validation" call into the relevant channel technology driver to see if it can still accept that member before re-adding the member to the queue? This is not fool-proof either, but it would certainly help. By: Mark Spencer (markster) 2004-12-06 00:56:26.000-0600 Added to CVS with some structural modifications and changing the option to "persistentmembers" rather than "persistent_members" and making the default "off" so as to retain current behavior for existing installations and making "yes" the default in the sample config for new installs.  Thank you very much for your contribution! By: Russell Bryant (russell) 2004-12-06 18:10:04.000-0600 not in 1.0 By: Digium Subversion (svnbot) 2008-01-15 15:15:45.000-0600 Repository: asterisk Revision: 4390 U   trunk/apps/app_queue.c U   trunk/configs/queues.conf.sample ------------------------------------------------------------------------ r4390 | markster | 2008-01-15 15:15:45 -0600 (Tue, 15 Jan 2008) | 2 lines Add persistent dynamic queue member support (bug ASTERISK-2877) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4390