[Home]

Summary:ASTERISK-12220: [patch] Queue is treated as empty if it isn't, but no agents meet the QUEUE_MIN_PENALTY and QUEUE_MAX_PENALTY criteria
Reporter:Nick Barnes (bcnit)Labels:
Date Opened:2008-06-18 12:19:27Date Closed:2008-10-29 15:05:14
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 12884.patch
( 1) 12884-1.6.0.patch
( 2) 12884v2.patch
( 3) 12884v2-1.6.0.patch
( 4) 12884v2-1.6.0-beta9.patch
( 5) 12884v3-1.6.0-beta9.patch
( 6) 12884v4-1.6.0-branch.patch
Description:In a situation where a call is placed into a queue as follows:

exten => s,n,Set(QUEUE_MAX_PENALTY=1)
exten => s,n,Set(QUEUE_MIN_PENALTY=1)
exten => s,n,Queue(${ORGCODE}_${ORGQUEUE},ih,,${ORGCODE}/announce-incoming)
)
The queue is deemed to be empty if there are no agents with a penalty of 1 as a member of the queue.

We use queuerules.conf to raise the QUEUE_MAX_PENALTY value to overflow calls to other departments and in our case (and I can imagine, other people's too), it is perfectly legitimate to have members with a penalty of 2 logged in and no members with penalty 1.

As a result of this, the function QUEUE_MEMBER() returns incorrect data (i.e. QUEUE_MEMBER(free) may return the "number of logged-in members for the specified queue available to take a call" as 10, but in reality and because of the penalty issue, it is zero.

I have been able to work around this by checking QUEUE_MEMBER(count) after I receive a JOINEMPTY bounce out, incrementing QUEUE_MAX_PRIORITY and trying again, but this is far from ideal!
Comments:By: Mark Michelson (mmichelson) 2008-06-18 12:34:24

A temporary workaround would be to set joinempty=yes. I'll work on a better solution right now, though. Ideally, if the queue is going to exit due to a joinempty or leaveempty condition, we should move to the next penalty rule.
By: David Woolley (davidw) 2008-06-18 12:50:55

I'm not clear, to me, whether you are suggesting that QUEUE_MEMBER() is wrong, or whether you are suggesting that a queue shouldn't reject calls if it has members, even though the members are disqualified from handling the call.

My own feeling is that one needs much more fine grained control.  The problem we had was that having all members paused will produce similar results, but we were using pause as part of our normal operation.  We had to go to joinempty=yes.
By: Mark Michelson (mmichelson) 2008-06-18 13:13:09

davidw: Keep in mind that this issue is 1.6-specific. In 1.6, there is a means by which the QUEUE_MAX_PENALTY may change in mid-call. My feeling is that if there are rules defined for changing the QUEUE_MAX_PENALTY, then a call which would otherwise exit with JOINEMPTY status should instead first try moving through the defined rules to see if increasing the maximum penalty produces reachable members.

As far as tighter control in defining what it means for members to be reachable, that seems like a different issue altogether, though I agree that more control couldn't hurt.
By: Mark Michelson (mmichelson) 2008-06-18 15:32:22

I have uploaded a patch which takes care of the joinempty problem reported. I still need to patch app_queue to advance to the next defined rule in a leaveempty situation as well.
By: Mark Michelson (mmichelson) 2008-06-18 17:10:10

Use v2, the first one actually has a problem if there really are no queue members. V2 fixes it.
By: Nick Barnes (bcnit) 2008-06-19 14:30:08

Tried to apply the patch, but hunk 4 failed:

[root@pbx09a apps]# patch < ~/queuepatch
patching file app_queue.c
Hunk #1 succeeded at 510 (offset -16 lines).
Hunk #2 succeeded at 1692 (offset 27 lines).
Hunk #3 succeeded at 1657 (offset -16 lines).
Hunk #4 FAILED at 1665.
Hunk ASTERISK-1 succeeded at 4348 (offset -138 lines).
Hunk ASTERISK-2 succeeded at 4617 (offset -18 lines).
1 out of 6 hunks FAILED -- saving rejects to file app_queue.c.rej
By: Mark Michelson (mmichelson) 2008-06-19 15:11:35

Sorry about that. When issues are reported against 1.6.0, I tend to patch trunk so that I can commit the change there and then merge that change into 1.6.0. I forgot to generate the patch for 1.6.0 so what's there is the trunk patch. I have now added 12884v2-1.6.0.patch. This should apply cleanly to an svn checkout of the 1.6.0 branch.
By: Nick Barnes (bcnit) 2008-06-20 04:07:29

I must be doing something silly - apologies, but still getting hunk 4 failing.
By: Mark Michelson (mmichelson) 2008-06-20 09:11:50

Nope, not your fault at all. I keep making the patches against the wrong areas. I'm uploading a new one now which is made against the 1.6.0-beta9 tag. This one will apply cleanly.
By: Mark Michelson (mmichelson) 2008-06-25 14:20:56

I was curious how this patch has been working out for you. I have the second part (which advances to the next penalty rule on a potential leavewhenempy condition) ready for testing as well and will be uploading it shortly.
By: Mark Michelson (mmichelson) 2008-06-25 14:34:06

12884v3-1.6.0-beta9.patch is ready for testing. It combines the changes from v2 as well as adding the behavior of traversing the penalty rules when a "leavewhenempty" condition is reached too.
By: Nick Barnes (bcnit) 2008-06-26 12:32:49

The patch has been live on a production system receiving approximately 500calls/day since Saturday and it's been performing flawlessly since then. I will apply the next patch when I return to work on Monday. Cheers.
By: Mark Michelson (mmichelson) 2008-06-26 18:15:53

Wonderful! Do you think this might actually be the underlying issue behind ASTERISK-12216 which you also reported?
By: Nick Barnes (bcnit) 2008-07-03 10:33:48

Patch applied. I'll let you know how we get on.
By: Nick Barnes (bcnit) 2008-07-11 02:29:25

Major problem. It's started core dumping.

It was working absolutely fine until the middle of the day yesterday when it started crashing Asterisk:

e.g.
-------------------8<------------------
   -- SIP/agent774-01bcf781 is ringing
   -- SIP/agent323-09c2adf1 is ringing
   -- SIP/agent774-01bcf781 is ringing
   -- SIP/agent323-09c2adf1 is ringing
   -- SIP/agent774-01bcf781 is ringing
   -- SIP/agent323-09c2adf1 is ringing
   -- SIP/agent774-01bcf781 is ringing
   -- SIP/agent323-09c2adf1 is ringing
   -- Nobody picked up in 10000 ms
   -- Nobody picked up in 10000 ms
   -- Nobody picked up in 10000 ms
pbx13*CLI>
Disconnected from Asterisk server
-------------------8<------------------

and

-------------------8<------------------
   -- SIP/agent380-09cb3899 is ringing
   -- SIP/agent714-09c5de09 is ringing
   -- SIP/agent380-09cb3899 is ringing
   -- SIP/agent714-09c5de09 is ringing
   -- SIP/agent380-09cb3899 is ringing
   -- SIP/agent714-09c5de09 is ringing
   -- SIP/agent380-09cb3899 is ringing
   -- SIP/agent714-09c5de09 is ringing
   -- Nobody picked up in 10000 ms
   -- Nobody picked up in 10000 ms
   -- Nobody picked up in 10000 ms
pbx13*CLI>
Disconnected from Asterisk server
-------------------8<------------------

This is a production box and I hadn't removed optimisation when I compiled it, so although I have the dumps, I imagine they'd be useless.

I reverted to the original app_queue.c and it's been OK overnight and this morning so far.
By: Mark Michelson (mmichelson) 2008-07-16 15:33:18

So 12884v3-1.6.0-beta9.patch crashed, but 12884v2-1.6.0-beta9.patch worked properly? Do I understand correctly?
By: Leif Madsen (lmadsen) 2008-10-27 11:52:25

This issue has been open for quite a while with no progress after the core dumps. Any more information on this issue?
By: Terry Wilson (twilson) 2008-10-27 21:44:33

The crash is in the for (; !exit || qe.pr; update_qe_rule(&qe)) lines.  if !exit and qe.pr == NULL, update_qe_rule(&qe) is called and crashes when it tries to access qe->pr.

I'm still trying to get a test going that actually successfully switches over to a higher penalty queue member based on my queuerules.conf entry after removing the original (lower penalty) member.
By: Terry Wilson (twilson) 2008-10-28 19:12:17

Ok, this patch fixes the crash, and some issues where we were calling update_qe_rule() when we (I think) didn't want to.

putnopvut, if you could look over my changes to make sure they make sense, I'd appreciate it.

bcnit, it would be nice if you could test as well.  I tested several different cases, and am fairly confident, but more testing is always nice.
By: Mark Michelson (mmichelson) 2008-10-29 12:44:30

I discussed this with otherwiseguy and the patch meets my approval.

The only problems with the patch are silly things that are my fault like trailing whitespace and some weird indentation on the comments near the bottom of join_queue. Functionally, though, it's great! Thanks for picking this one up.
By: Digium Subversion (svnbot) 2008-10-29 15:02:33

Repository: asterisk
Revision: 152644

U   branches/1.6.0/apps/app_queue.c

------------------------------------------------------------------------
r152644 | twilson | 2008-10-29 15:02:33 -0500 (Wed, 29 Oct 2008) | 7 lines

Small modification to putnopvut's patch to fix this issue.  Thanks for all the help, putnopvut!
(closes issue ASTERISK-12220)
Reported by: bcnit
Patches:
     12884v4-1.6.0-branch.patch uploaded by otherwiseguy (license 396)
Tested by: otherwiseguy

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=152644
By: Digium Subversion (svnbot) 2008-10-29 15:05:11

Repository: asterisk
Revision: 152645

U   branches/1.6.1/apps/app_queue.c

------------------------------------------------------------------------
r152645 | twilson | 2008-10-29 15:05:11 -0500 (Wed, 29 Oct 2008) | 8 lines

Small modification to putnopvut's patch to fix this issue.  Thanks for all the help, putnopvut!
(closes issue ASTERISK-12220)
   Reported by: bcnit
   Patches:
         12884v4-1.6.0-branch.patch uploaded by otherwiseguy (license 396)
   Tested by: otherwiseguy


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=152645