[Home]

Summary:ASTERISK-09953: [patch] Unload/load support for chan_skinny
Reporter:dea (dea)Labels:
Date Opened:2007-07-25 01:40:58Date Closed:2009-02-27 14:38:43.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_skinny-reload.txt
( 1) chan_skinny-reload-trunk.txt
( 2) chan_skinny-reload-v2.txt
( 3) chan_skinny-reload-v3.txt
( 4) chan_skinny-reload-v5.txt
( 5) skinny-reload-trunkv2.diff
( 6) skinny-reload-trunk-v3.txt
( 7) skinny-reload-trunk-v4.txt
Description:Reload is not working, but unload and load do work.
My reload code is ugly-simple, it calls the unload
function, then the load function.  I would have thought
if I could manually unload and load that would be enough.


My threading knowledge is more than a bit weak, but the
existing code was 90% there already.  We needed to store
the address of the skinny_session thread launch from within
accept_thread so we could later cancel it.

Interestingly the skinnysession structure had a member to
store the thread address, but it did not appear to be used.

I also move the teardown of the accept thread to a point before
tearing down the sessions (and their threads) to prevent a phone
from trying to re-register and recreating a session thread after
we had processed it.

Comments:By: dea (dea) 2007-07-25 12:04:29

Reload now works.  I had to reset a couple thread states and the skinnysocket
value to initial defaults to convice the reload to work.

Do we need a CLI for "skinny reload"  or is the prefered method to be
"module reload chan_skinny.so"?
By: Jason Parker (jparker) 2007-07-25 12:07:19

What happens to existing calls during a reload?

Also, no need for a "skinny reload" command.
By: dea (dea) 2007-07-25 12:19:07

Good question on existing calls, I'll test that.  What happens with
other channels?  What should happen?

** Edit **
Bad things.  Segfault.  I'll work on fixing it, but what should be the
behaviour be?  Wait for calls to complete?  Terminate the calls?

With my native bridging patch, we should be able to maintain calls
during a reload (as long as they are not terminating on * or gatewaying
to a non-RTP tech)


By: Jason Parker (jparker) 2007-07-25 12:24:51

SIP keeps existing calls up.

I tested it here, and it can't keep calls up the way it is now, because it tears down the session, forcing the device to re-register.
By: dea (dea) 2007-07-25 12:53:52

Trying to get reload to work without tearing down the sessions
will prove interesting.  Devices and sessions are linked, and
if the config change includes a new bind address/port the sessions
will be bogus.

Even is the config change is only a new phone, the config load
will need to check if a device being loaded already exists in
memory and copy over state information, then remove the old device.
That does not look trivial.

With CCM the device notes that a session has been destroyed, but will
not try to re-register if it is in a call.  The moment the call ends
it will re-register.  I think this is what we want, as the whole point
of the reload may have been to change a setting on that device, say
adding a line.

Perhaps simply not performing the softhangup in unload_module will be enough.
I'll test that.

** Edit  **
Yup, if we do not issue the softhangup, the call is preserved, the reload
completes without a segfault and once the call ends the phone re-registers.
Updated patch pending...


By: Jason Parker (jparker) 2007-07-26 14:27:34

It does keep the call going, however, the call never gets hung up (try it, and do a core show channels at any time after the reload).
By: Jason Parker (jparker) 2007-07-26 15:13:37

Also, a soft hangup afterwards will cause a segfault (I can't say I didn't expect that..)
By: dea (dea) 2007-07-26 15:18:47

I got distracted and did not get back to check that.

Perhaps on loading the channel it can try to locate existing
channels and re-attach to them.  But that will likely be ugly.

We do not want to hangup the channel, but do need a way to
detect orphaned channels.  It might work to queue a hangup
with already already gone set so that skinny_hangup does not
try to hangup the skinny channel, but I think that leave us
in the same spot.

Another option would be to traing the skinny_hangup callback to
acknowledge a hangup on a channel it does not know about.  That
way when the far end queues a hangup chan_skinny can signal that
it did hang it up...
By: dea (dea) 2007-07-26 18:39:57

OK, the segfault on an orphaned channel was easy to fix, just test for the
existence of d before testing d->registered in skinny_hangup()

Channels can be soft hungup.  If the far end is a channel that has it's
own hangup callback, and that end initiates the hangup, the channel will be
cleared.

Any tips on how to detect the orphaned channel to allow it to be cleaned up?
By: sbisker (sbisker) 2007-11-15 12:42:34.000-0600

Has this feature made any progress?  There hasn't been any activity for months.  It's one of the key missing administration pieces to chan_skinny.
By: dea (dea) 2007-11-15 13:28:15.000-0600

I have not touch the code since the last message I posted.

I'm still looking for hints on how to detect orphaned channels
and clear them.  I can keep working on it, but I think I will
need some help to figure out the issues outlined in post 0067944
By: sbisker (sbisker) 2008-01-22 07:59:41.000-0600

Any luck on moving this forward?  As far as handling orphaned channels, wouldn't the same principles apply that are used for chan_sip and chan_zap?
By: dea (dea) 2008-01-22 11:08:54.000-0600

No progress at all.  I have not had time to work on it, and the last dive into
chan_sip and chan_zap to get idea on how to handle orphaned channels was not
very successful.  

I'll try to make time to work on it this week.

*** Update ***
It appears the loader has been modified to use reference counters for
all modules.  So it will take a good bit more work than I will have time
for in the near future.


By: sbisker (sbisker) 2008-03-05 07:58:05.000-0600

Is there a way to make it so that the reload command will work only when there are no active chan_skinny channels.  Right now I have 2 asterisk servers.  One for chan_skinny activity and one for everything else.  Reason being, that every time I have to modify or add a skinny device, the server has to be restarted.  Not good for a production system.  Just a thought.
By: Michiel van Baak (mvanbaak) 2008-05-08 17:13:20

realtime to the rescue !
If I only had enough time to actually get some code done for this ....
By: Michiel van Baak (mvanbaak) 2008-09-25 20:43:42

in 13524 the unload is fixed.
Reload is still an issue.
By: dea (dea) 2008-09-26 17:03:12

Asked and delivered.

The conversion to AST_LIST macros made it fairly straight forward.

Tests passed-
1.  New device added to config
2.  Device removed from config
3.  Device settings (line) changed

Calls are not distrupted during a call.  A soft reset of each registered
device is performed on a reload.  Devices in a call will soft reset once
the active call is ended.
By: Niklas Larsson (pnlarsson) 2008-11-11 08:39:34.000-0600

Tested with 7960 and 7905 and works without issues. Great job!
By: dea (dea) 2008-11-21 16:28:13.000-0600

Quite a few changes have occured in trunk and 90% of the patch had to be tossed, so here is a fresh version for trunk.  Quick tests against trunk seem solid...
By: Damien Wedhorn (wedhorn) 2008-11-22 16:59:22.000-0600

Been having a play with the latest patch. DEA, the code at the beginning of config_line and config_device you removed were added for mvanbaak's realtime stuff he's working on. Simple enough to add back (skinny-reload-trunkv2.diff).

I think we need to decide how were going to handle the device resets. DEA's patch sends a soft reset to the device which basically removes chan_skinny from the loop. How should we handle background calls (such as ones on hold)?

Ideally, I think we should not reset the device until there are no subs under any of the lines attached to that device (which could take a while). How about subsequent calls to the device, should we allow these or send a DEVICE_NOT_CONNECTED until after the device is reset? Should we allow a device that is set for pruning but has a call on hold, initiate a new call?

At the moment the patch works. It happily handles single calls (via the soft reset). I can't test much more than that because we need to work on the sub handling before multiple calls work semi reliably on my old devices.
By: dea (dea) 2008-11-22 20:28:39.000-0600

The skinny phones are not too smart, but one thing they do get right
is the reset/soft reset.  The device will queue the reset until after
the phone is idle, so we should not need any special handling of
subs with regards to the reset.  As for subsequent calls, I could
go either way, allow it through or allow the dialplan to handle it
as a busy signal.  I would lean towards allowing it through...

The reason I decided to create new devices/lines instead of reconfiguring
was simple.  The phone needs to be reset for the changes to apply and,
is seemed cleaner to build new lines and devices than to try to merge
the old and new configs.

If we do re-use and existing loaded device, we must remove all lines and
speeddials before attempting to add new ones.  Otherwise devices that
do not change might end up with duplicates.
By: Damien Wedhorn (wedhorn) 2008-11-22 20:39:01.000-0600

Do you know what "idle" means? eg ON_HOOK with a sub on hold wouldn't be a good time to reset.

My patch still creates the new structures for reload, but allows for structures to be edited (which is currently unused but there to help realtime). I'm leaning toward allowing additional calls as well.
By: dea (dea) 2008-11-23 01:28:51.000-0600

At least on CCM, idle means no calls in any state.  I'll need to retest,
but I believe that we have most call states handled already and I would
not expect a call during a hold.

I am not opposed to trying to re-use and existing device/line structure,
but we will need to do more work to ensure we do not create duplicate
lines and speeddial associations.  I did try that initially, and found
it easier to just create new objects and prune the old, which reminds
me that I forgot to add line pruing to this patch.

I am interested in seeing how mvanbaak is handling the RealTime code.
I modelled the reload code on chan_sip, which also creates new objects
and prunes any that no longer exist in the config.
By: Damien Wedhorn (wedhorn) 2008-11-23 01:35:54.000-0600

Sounds like the soft reset may be enough (might need some work on setting the proper callstates on our part, but would be worth it).

The difference is that realtime (I assume) will only change one or two variables rather than reloading the whole config that a reload does (well that's what I wrote those stubs to do anyway). But yes, it will need some work because some of the changes will basically result in having to create a new structure, but if he just wants to set earlyrtp on for example, why go through the hassle.
By: Michiel van Baak (mvanbaak) 2009-01-23 15:00:32.000-0600

I like the way wedhorn's patch handles this.
and it wont interfere with the realtime plans I have. I was using chan_sip's way of doing stuff as well.

Let me make another testround with this patch and get it in if it succeeds.

[edit: I meant wedhorn's patch (trunkv2). I'm using that in production for some time now and it works great]


By: Michiel van Baak (mvanbaak) 2009-01-25 08:43:44.000-0600

Any objections for putting the latest patch on this bug (skinny-reload-trunkv2.diff) in ?
If no objections in a week I'll put it in.
By: dea (dea) 2009-01-30 14:15:01.000-0600

V3 is buggered.   Ignore it for the moment....


By: dea (dea) 2009-01-30 18:25:41.000-0600

After proper clean up of previously allocated devices and lines, I found that
the soft reset no longer worked.

The patch now 'migrates' state information about existing subchannels and
line hookstate, which is required if we expect stimulus or softkey events
to be handled.

The relationship between lines, device, sessions and subchannels is amazingly
incestuous.  I think I have covered all of the relationships, and my single
skinny phone development lab survives back to back reloads, even with active
calls.

Close scrutiny and testing suggested...


By: Michiel van Baak (mvanbaak) 2009-02-21 07:46:57.000-0600

pj, wedhorn, Can you two test this as well?
It works for my 2-phone setup here and if you dont see real trouble I really like to get this in.
By: Michiel van Baak (mvanbaak) 2009-02-21 09:10:23.000-0600

I put latest patch up for review: http://reviewboard.digium.com/r/130/
By: pj (pj) 2009-02-21 12:57:34.000-0600

tested with one cisco7940, module unload/load, config change and reload and everything is working as expected, doesn't lock or crash, good work!
Asterisk SVN-trunk-r175699 + skinny-reload-trunk-v4.txt
By: Digium Subversion (svnbot) 2009-02-27 14:34:00.000-0600

Repository: asterisk
Revision: 179122

U   trunk/channels/chan_skinny.c

------------------------------------------------------------------------
r179122 | mvanbaak | 2009-02-27 14:34:00 -0600 (Fri, 27 Feb 2009) | 16 lines

Add reload support to chan_skinny.

Special thanks goes to DEA who had to redo this patch twice
because we first put unload/load support in and later redid the way
we configure devices and lines.

(closes issue ASTERISK-9953)
Reported by: DEA
Patches:
     skinny-reload-trunkv2.diff uploaded by wedhorn (license 30)
     skinny-reload-trunk-v4.txt uploaded by DEA (license 3)
 With mods by me based on feedback from wedhorn and Russell and seanbright
Tested by: DEA, mvanbaak, pj

Review: http://reviewboard.digium.com/r/130/

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

http://svn.digium.com/view/asterisk?view=rev&revision=179122
By: Digium Subversion (svnbot) 2009-02-27 14:36:47.000-0600

Repository: asterisk
Revision: 179123

_U  branches/1.6.0/

------------------------------------------------------------------------
r179123 | mvanbaak | 2009-02-27 14:36:47 -0600 (Fri, 27 Feb 2009) | 22 lines

Blocked revisions 179122 via svnmerge

........
 r179122 | mvanbaak | 2009-02-27 21:34:00 +0100 (Fri, 27 Feb 2009) | 16 lines
 
 Add reload support to chan_skinny.
 
 Special thanks goes to DEA who had to redo this patch twice
 because we first put unload/load support in and later redid the way
 we configure devices and lines.
 
 (closes issue ASTERISK-9953)
 Reported by: DEA
 Patches:
       skinny-reload-trunkv2.diff uploaded by wedhorn (license 30)
       skinny-reload-trunk-v4.txt uploaded by DEA (license 3)
   With mods by me based on feedback from wedhorn and Russell and seanbright
 Tested by: DEA, mvanbaak, pj
 
 Review: http://reviewboard.digium.com/r/130/
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=179123
By: Digium Subversion (svnbot) 2009-02-27 14:38:42.000-0600

Repository: asterisk
Revision: 179124

_U  branches/1.6.1/

------------------------------------------------------------------------
r179124 | mvanbaak | 2009-02-27 14:38:41 -0600 (Fri, 27 Feb 2009) | 22 lines

Blocked revisions 179122 via svnmerge

........
 r179122 | mvanbaak | 2009-02-27 21:34:00 +0100 (Fri, 27 Feb 2009) | 16 lines
 
 Add reload support to chan_skinny.
 
 Special thanks goes to DEA who had to redo this patch twice
 because we first put unload/load support in and later redid the way
 we configure devices and lines.
 
 (closes issue ASTERISK-9953)
 Reported by: DEA
 Patches:
       skinny-reload-trunkv2.diff uploaded by wedhorn (license 30)
       skinny-reload-trunk-v4.txt uploaded by DEA (license 3)
   With mods by me based on feedback from wedhorn and Russell and seanbright
 Tested by: DEA, mvanbaak, pj
 
 Review: http://reviewboard.digium.com/r/130/
........

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

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