dispatcher: limit a number of dst to failover to - sr-dev

Daniel-Constantin Mierla

21 Aug 21 Aug

7:19 p.m.

Hello, thanks for sending the patch! Two remarks for now: 1) you changed the inter-module API prototype for ds_select_dst_f, respectively: typedef int (*ds_select_dst_f)(struct sip_msg *msg, int set, - int alg, int mode); + int alg, unsigned int limit, int mode); I think the reason for that function is an export to app_lua (and maybe other embedded languages). With this change, things get broken there. I would do: - rename the function ds_select_dst() to ds_select_dst_limit() - add ds_select_dst() as a wrapper function calling the new ds_select_dst_limit() The API stay unchanged, and eventually one can add a new member for ds_select_dst_limit() when needed Otherwise, the change of ds_select_dst() prototype should involve reviewing other modules and seeing if people are happy with this change, because it breaks existing embedded scripts. 2) You need to document the new functions exported to kamailio config, in modules/dispatcher/doc/dispatcher_admin.xml Once the patch is overall ok, I can commit it to the repository. Cheers, Daniel On 21/08/14 17:40, Alekzander Spiridonov wrote:

...

Hi list, I'd like to propose a patch that allows user to limit the number of items that appears in dst_avp after ds_select_dst or ds_select_domain call. There are some use cases when one could have couple of dst sets and would like to failover from the first to another without checking all items but have just 3 or 4 of them dead in a row. What do you think about that? -- Best regards, Alekzander Spiridonov _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Daniel-Constantin Mierla http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA

Reply

Alekzander Spiridonov

2:32 p.m.

One more thing I've figured out: In dispatcher module guide there is a way to mark dst as Active, though in src w_ds_mark_dst1 has no such option. On the other hand, w_ds_mark_dst1 has an option to mark dst as "admin disabled" that is not documented in the guide. Could you clarify what is the expected behavior so I could send a fix for that. 2014-08-22 13:25 GMT+04:00 Alekzander Spiridonov <alekz(a)li.ru>ru>:

...

Hi Daniel, 1. Yep, had the same concern, but somehow missed the app_lua. Fixed as you've described. 2. Done. New patch attached. 2014-08-21 20:19 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>: Hello,

thanks for sending the patch! Two remarks for now: 1) you changed the inter-module API prototype for ds_select_dst_f, respectively: typedef int (*ds_select_dst_f)(struct sip_msg *msg, int set, - int alg, int mode); + int alg, unsigned int limit, int mode); I think the reason for that function is an export to app_lua (and maybe other embedded languages). With this change, things get broken there. I would do: - rename the function ds_select_dst() to ds_select_dst_limit() - add ds_select_dst() as a wrapper function calling the new ds_select_dst_limit() The API stay unchanged, and eventually one can add a new member for ds_select_dst_limit() when needed Otherwise, the change of ds_select_dst() prototype should involve reviewing other modules and seeing if people are happy with this change, because it breaks existing embedded scripts. 2) You need to document the new functions exported to kamailio config, in modules/dispatcher/doc/dispatcher_admin.xml Once the patch is overall ok, I can commit it to the repository. Cheers, Daniel On 21/08/14 17:40, Alekzander Spiridonov wrote: Hi list, I'd like to propose a patch that allows user to limit the number of items that appears in dst_avp after ds_select_dst or ds_select_domain call. There are some use cases when one could have couple of dst sets and would like to failover from the first to another without checking all items but have just 3 or 4 of them dead in a row. What do you think about that? -- Best regards, Alekzander Spiridonov _______________________________________________ sr-dev mailing listsr-dev@lists.sip-router.orghttp://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev -- Daniel-Constantin Mierlahttp://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Best regards, Alekzander Spiridonov

Reply

Alekzander Spiridonov

5:48 p.m.

Ok, so at the moment src supports the following options: if (len>1 && (str1[1]=='p' || str1[1]=='P')) state |= DS_PROBING_DST; if(str1[0]=='i' || str1[0]=='I') state |= DS_INACTIVE_DST; else if(str1[0]=='t' || str1[0]=='T') state |= DS_TRYING_DST; else if(str1[0]=='d' || str1[0]=='D') state = DS_DISABLED_DST; else if(str1[0]=='p' || str1[0]=='P') state = DS_INACTIVE_DST|DS_PROBING_DST; I do believe that flag still should be single letter, so I'd suggest removing the first "if" since we still have the last "else if". What do you think about that? Also I'd add "A" option to this list. 2014-08-22 18:26 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

...

On 22/08/14 13:32, Alekzander Spiridonov wrote: One more thing I've figured out: In dispatcher module guide there is a way to mark dst as Active, though in src w_ds_mark_dst1 has no such option. On the other hand, w_ds_mark_dst1 has an option to mark dst as "admin disabled" that is not documented in the guide. Could you clarify what is the expected behavior so I could send a fix for that. The purpose of that function is to be able to set a destination in one of supported states: - completely disabled (admin disabled) -- I guess is 'd' -- not used and no chance to activate it automatically upon a successful probing request (actually no probing will be done) - inactive state - is not used currently, but if in probing mode, it can be activated automatically if a probe request is answered - active state - used currently The probing mode that comes as a flag, is related to whether to send periodically (probe) requests to check if targets are online. Trying wasn't coded by me (iirc :-) ), but I think the purpose was to still keep the target in active mode until a certain number of failures occur and then make it inactive (or disabled). If you find discrepancies in the code, let us know -- of course, patches are more than welcome! Cheers, Daniel 2014-08-22 13:25 GMT+04:00 Alekzander Spiridonov <alekz(a)li.ru>ru>:

Hi Daniel, 1. Yep, had the same concern, but somehow missed the app_lua. Fixed as you've described. 2. Done. New patch attached. 2014-08-21 20:19 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>: Hello,

thanks for sending the patch! Two remarks for now: 1) you changed the inter-module API prototype for ds_select_dst_f, respectively: typedef int (*ds_select_dst_f)(struct sip_msg *msg, int set, - int alg, int mode); + int alg, unsigned int limit, int mode); I think the reason for that function is an export to app_lua (and maybe other embedded languages). With this change, things get broken there. I would do: - rename the function ds_select_dst() to ds_select_dst_limit() - add ds_select_dst() as a wrapper function calling the new ds_select_dst_limit() The API stay unchanged, and eventually one can add a new member for ds_select_dst_limit() when needed Otherwise, the change of ds_select_dst() prototype should involve reviewing other modules and seeing if people are happy with this change, because it breaks existing embedded scripts. 2) You need to document the new functions exported to kamailio config, in modules/dispatcher/doc/dispatcher_admin.xml Once the patch is overall ok, I can commit it to the repository. Cheers, Daniel On 21/08/14 17:40, Alekzander Spiridonov wrote: Hi list, I'd like to propose a patch that allows user to limit the number of items that appears in dst_avp after ds_select_dst or ds_select_domain call. There are some use cases when one could have couple of dst sets and would like to failover from the first to another without checking all items but have just 3 or 4 of them dead in a row. What do you think about that? -- Best regards, Alekzander Spiridonov _______________________________________________ sr-dev mailing listsr-dev@lists.sip-router.orghttp://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev -- Daniel-Constantin Mierlahttp://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Best regards, Alekzander Spiridonov

-- Best regards, Alekzander Spiridonov -- Daniel-Constantin Mierlahttp://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA

-- Best regards, Alekzander Spiridonov

Reply

Daniel-Constantin Mierla

26 Aug 26 Aug

1:45 p.m.

Thanks for the patch. One note about -- use tab based indentation for patches to existing modules, to keep the code looking coherent. I can apply the patch and reset the formatting this time, but have in mind for the future. Otherwise, patch looks ok. Cheers, Daniel On 25/08/14 15:32, Alekzander Spiridonov wrote:

...

Patch for flag parsing attached. 2014-08-22 18:48 GMT+04:00 Alekzander Spiridonov <alekz(a)li.ru <mailto:alekz@li.ru>>: Ok, so at the moment src supports the following options: if (len>1 && (str1[1]=='p' || str1[1]=='P')) state |= DS_PROBING_DST; if(str1[0]=='i' || str1[0]=='I') state |= DS_INACTIVE_DST; else if(str1[0]=='t' || str1[0]=='T') state |= DS_TRYING_DST; else if(str1[0]=='d' || str1[0]=='D') state = DS_DISABLED_DST; else if(str1[0]=='p' || str1[0]=='P') state = DS_INACTIVE_DST|DS_PROBING_DST; I do believe that flag still should be single letter, so I'd suggest removing the first "if" since we still have the last "else if". What do you think about that? Also I'd add "A" option to this list. 2014-08-22 18:26 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com <mailto:miconda@gmail.com>>: On 22/08/14 13:32, Alekzander Spiridonov wrote:

One more thing I've figured out: In dispatcher module guide there is a way to mark dst as Active, though in src w_ds_mark_dst1 has no such option. On the other hand, w_ds_mark_dst1 has an option to mark dst as "admin disabled" that is not documented in the guide. Could you clarify what is the expected behavior so I could send a fix for that.

The purpose of that function is to be able to set a destination in one of supported states: - completely disabled (admin disabled) -- I guess is 'd' -- not used and no chance to activate it automatically upon a successful probing request (actually no probing will be done) - inactive state - is not used currently, but if in probing mode, it can be activated automatically if a probe request is answered - active state - used currently The probing mode that comes as a flag, is related to whether to send periodically (probe) requests to check if targets are online. Trying wasn't coded by me (iirc :-) ), but I think the purpose was to still keep the target in active mode until a certain number of failures occur and then make it inactive (or disabled). If you find discrepancies in the code, let us know -- of course, patches are more than welcome! Cheers, Daniel

2014-08-22 13:25 GMT+04:00 Alekzander Spiridonov <alekz(a)li.ru <mailto:alekz@li.ru>>: Hi Daniel, 1. Yep, had the same concern, but somehow missed the app_lua. Fixed as you've described. 2. Done. New patch attached. 2014-08-21 20:19 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com <mailto:miconda@gmail.com>>: Hello, thanks for sending the patch! Two remarks for now: 1) you changed the inter-module API prototype for ds_select_dst_f, respectively: typedef int (*ds_select_dst_f)(struct sip_msg *msg, int set, - int alg, int mode); + int alg, unsigned int limit, int mode); I think the reason for that function is an export to app_lua (and maybe other embedded languages). With this change, things get broken there. I would do: - rename the function ds_select_dst() to ds_select_dst_limit() - add ds_select_dst() as a wrapper function calling the new ds_select_dst_limit() The API stay unchanged, and eventually one can add a new member for ds_select_dst_limit() when needed Otherwise, the change of ds_select_dst() prototype should involve reviewing other modules and seeing if people are happy with this change, because it breaks existing embedded scripts. 2) You need to document the new functions exported to kamailio config, in modules/dispatcher/doc/dispatcher_admin.xml Once the patch is overall ok, I can commit it to the repository. Cheers, Daniel On 21/08/14 17:40, Alekzander Spiridonov wrote:

Hi list, I'd like to propose a patch that allows user to limit the number of items that appears in dst_avp after ds_select_dst or ds_select_domain call. There are some use cases when one could have couple of dst sets and would like to failover from the first to another without checking all items but have just 3 or 4 of them dead in a row. What do you think about that? -- Best regards, Alekzander Spiridonov _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Daniel-Constantin Mierla http://twitter.com/#!/miconda <http://twitter.com/#%21/miconda> -http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 -http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev -- Best regards, Alekzander Spiridonov -- Best regards, Alekzander Spiridonov

-- Daniel-Constantin Mierla http://twitter.com/#!/miconda <http://twitter.com/#%21/miconda> -http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 -http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA -- Best regards, Alekzander Spiridonov -- Best regards, Alekzander Spiridonov

-- Daniel-Constantin Mierla http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA

Reply

Alekzander Spiridonov

5:13 p.m.

Sure, thank you! 2014-08-22 18:03 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

...

Hello, I just pushed your patch, with a small update: when there is no limit wanted, instead of 'limit=-1' is now 'limit=0xffffffff -- limit is unsigned it and some compilers and analysing tools issue warning when detecting working with negative values in such cases -- the behavior stays the same. Cheers, Daniel On 22/08/14 11:25, Alekzander Spiridonov wrote: Hi Daniel, 1. Yep, had the same concern, but somehow missed the app_lua. Fixed as you've described. 2. Done. New patch attached. 2014-08-21 20:19 GMT+04:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

Hello, thanks for sending the patch! Two remarks for now: 1) you changed the inter-module API prototype for ds_select_dst_f, respectively: typedef int (*ds_select_dst_f)(struct sip_msg *msg, int set, - int alg, int mode); + int alg, unsigned int limit, int mode); I think the reason for that function is an export to app_lua (and maybe other embedded languages). With this change, things get broken there. I would do: - rename the function ds_select_dst() to ds_select_dst_limit() - add ds_select_dst() as a wrapper function calling the new ds_select_dst_limit() The API stay unchanged, and eventually one can add a new member for ds_select_dst_limit() when needed Otherwise, the change of ds_select_dst() prototype should involve reviewing other modules and seeing if people are happy with this change, because it breaks existing embedded scripts. 2) You need to document the new functions exported to kamailio config, in modules/dispatcher/doc/dispatcher_admin.xml Once the patch is overall ok, I can commit it to the repository. Cheers, Daniel On 21/08/14 17:40, Alekzander Spiridonov wrote: Hi list, I'd like to propose a patch that allows user to limit the number of items that appears in dst_avp after ds_select_dst or ds_select_domain call. There are some use cases when one could have couple of dst sets and would like to failover from the first to another without checking all items but have just 3 or 4 of them dead in a row. What do you think about that? -- Best regards, Alekzander Spiridonov _______________________________________________ sr-dev mailing listsr-dev@lists.sip-router.orghttp://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev -- Daniel-Constantin Mierlahttp://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Best regards, Alekzander Spiridonov -- Daniel-Constantin Mierlahttp://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda Next Kamailio Advanced Trainings 2014 - http://www.asipto.com Sep 22-25, Berlin, Germany ::: Oct 15-17, San Francisco, USA

-- Best regards, Alekzander Spiridonov

Reply