OK, guys,

It seems I did the pull request right this time. Now it contains only my changes, as I verified my forked repo was in full sync with kamailio:master.

if one of the maintainers could take a look at this pull request, please? It'd be great if it could be at release 5.4.0.

It's a very small change, just about 50 lines, and a very simple concept. Just rehash of hash, a limited amount of times, when destinations are not available.

Best regards,

Luis Rojas (Kalki70 at github)

On Wed, Jun 17, 2020 at 9:30 AM Kalki70 notifications@github.com wrote:

Pre-Submission Checklist

• [ X] Commit message has the format required by CONTRIBUTING guide
• [ X] Commits are split per component (core, individual modules,

libs, utils, ...)

• [ X] Each component has a single commit (if not, squash them into

one commit)

• [ X] No commits to README files for modules (changes must be done to

docbook files in doc/ subfolder, the README file is autogenerated)

Type Of Change

• Small bug fix (non-breaking change which fixes an issue)
• [X ] New feature (non-breaking change which adds new functionality)
• Breaking change (fix or feature that would change existing

functionality)

Checklist:

• PR should be backported to stable branches
• [ X] Tested changes locally
• [ X] Related to issue #2361

https://github.com/kamailio/kamailio/issues/2361

Description

When a destination is selected using one of the hash algorithms (over Call-ID, From, etc.) and destination is not available, then the next is selected. This causes the following problem : All messages to the unavailable destination will be sent to the same unique destination. This will cause that one destination will receive double load and the rest will continue with the same.

Solution : To better distribute messages, while also guaranteeing the same distribution of the hash, a re-hash of the hash is made.

As the rehash could select the same or other failed destination, it can rehash several times. The number of rehashes made is controlled by :

modparam("dispatcher", "ds_rehash_max", -1)

Which can take the following values :

-1 : The total count of destinations is used 0 : Disabled. Reverts to original implementation. Positive number. Maximum number of rehashes.

---

You can view, comment on, or merge this pull request online at:

https://github.com/kamailio/kamailio/pull/2363 Commit Summary

• dispatcher : better distribution when using hash and destination is

not available

File Changes

• *M* src/modules/dispatcher/dispatch.c

https://github.com/kamailio/kamailio/pull/2363/files#diff-b9d00a2976c1b1c5d3535a5ae0e24047 (49)

• *M* src/modules/dispatcher/dispatcher.c

https://github.com/kamailio/kamailio/pull/2363/files#diff-5e75274551c31eb6158e6815fd840331 (2)

Patch Links:

• https://github.com/kamailio/kamailio/pull/2363.patch
• https://github.com/kamailio/kamailio/pull/2363.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kamailio/kamailio/pull/2363, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABO7UZMG4REGMAG7GQAYX6TRXDAMNANCNFSM4OATJVEA . _______________________________________________ Kamailio (SER) - Development Mailing List sr-dev@lists.kamailio.org https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev

@Kalki70 pushed 1 commit.

ce177c134f00cd1509313b50163849f736b6e0a9 dispatcher : FIX. Previous implementation of rehash did not consider case when "use_default" flag is active.

First, the variables must be declared at the beginning of the functions/blocks, to be coherent with the rest of the code.

Then, the new code is doing new-hashing over an integer converted to string, which then do new-hashing over the returned index (again converted to string). I do not like the approach. Why not just do hashid modulo number of active destinations then selecting the destination address by iterating in the list with active destinations, instead of picking up directly by index.

Anyhow, I think the existing algorithms should stay like they are now, you can add a new algorithm hashing the value in a variable (like algorithm 7) with the re-hashing you want. Algorithm 7 can substitute all the other hashing algorithms by setting the appropriate value in the hashing variabel (eg, $var(dshash) = $ci for algorithm 0).

It is admirable that you are convinced it is a faulty implementation nobody observed since August 2004 when the module was added and the hashing over call-id was implemented, so you are going to fix it now for everyone.

Well, actually that is the wanted behaviour of the respective hashing algorithms, to go to next one if the selected destination is unavailable. It may not meet your expectation, therefore you can add a new hashing algorithm, as I proposed. Otherwise, your fix is breaking existing deployments for others. It is one of the benefits of open source to be able to accommodate most of the needs of its contributors.

Normally, a gateway/destination server should be down temporarily, otherwise it should be removed from the list if is shut down for maintenance. Also, you should do dispatching of the initial requests stateless and rely on record-routing for requests within dialog. Because if you have destination servers going up and down during a call, then you do not ensure BYE is going to same server as the INVITE, if the number if active servers is different.

Regarding the order of the addresses in the list, to ensure the one you want, you have to set the priority field. While with a text file you can have an order in the file, with database is harder to predict the order of records in a select query without an explicit order-by.

I didn't want to be sarcastic at all, just admired how you put your perspective that you fix a bug -- you are right and others are wrong. And yes, I am sure I am not aware of all SIP scenarios, every day I am surprised on what new issues I have to troubleshoot on SIP traffic. That's why I haven't listed all of them, just the case of BYE, but you actually prove my point of CANCEL and ACK -- if the servers active at CANCEL or ACK are not like for INVITE, is hard to ensure the same server is selected. A solution here could be to try to do some stickiness routing using htable.

I also agree that there could be different ways to achieve some new feature, but, imo, it is more clear to have a new algorithm that covers all your needs, instead of changing for all the other hashing algorithms, which are used by many others as they are. This brings more flexibility as well, one can use existing algorithms and the new one in the same config at the same time, not depending on a modparam.

Same approach was done when relative weight distribution algorithm was added, along with the exiting at that time, weight distribution algorithm.

You are more than welcome to propose PR to improve the documentation based on your findings and discussions on community forums/portals -- such contributions are very much appreciated!

You said "I am trying to FIX the current implementation", which implies there is a problem, a bug. Initial developer maybe thought about more that you think, just had to implement based on the requirements at that moment and they may still be valid for specific use cases even after 16 years, given that the code was changed. Anyhow, let's not go around the words and focus on the technical parts.

I didn't say what you propose as a new distribution logic is not useful for some cases, I proposed to do a new algorithm inspiring from the 7. The module was introduced before variables, once they were available, algorithm 7 can offer same like hashing over callid or other attributes of the sip message. There are clear benefits of this approach, some I listed in previous comments.

If you look in the code where the algorithm 7 (DS_ALG_HASHPV) you can see how the value of the pv specified by modparam hash_pvar is received. If you get troubles sorting it out, let us know what you coded (post a diff), and we will try to assist.

After careful consideration, I have decided not to implement a new algorithm. My proposal is an enhancement of current hash algorithms. It's an extension. Without changes it will behave like it has always been, since 2004. **It does not break anything**.

However, if the user wants, via modparam he can activate the extension, so rehash of hash can happen. As we must know, hash of rehash could lead to same original failed server. In that case, a loop of rehashing may happen. It's up to the user to decide how many tries of rehashing. Anyway, if after retrying hashing still no available server is found, algorithm stops and falls back to original, sequential implementation. Fully backward compatible.

I expected this place to be an interesting interacting place of ideas and debating, with several people arguing. For instance, mister @sergey-safarov, who seemed to like my proposal at issue #2361.

Now, if mister "god" Daniel Constantin Mierla decides to just reject this proposal, because he just did not like it, or because it's June, we will just happily use at my work my patched version of kamailio, and we can simply forget the subject.

This is one of the benefits of open source.

@Kalki70 - I think you start crossing the common sense line with your last comment on a personal attack. As I said my non-technical remark was more from an admiration point of view -- bugs that are found in code older than 15 years deserve somehow an "highlighting prize", but it is not the case here, you wrote you FIX current implementation, proves to be a new set of distribution algorithms. Everything else was technical review and comments about the code changes. If you want the kind of discussions with personal attacks, Kamailio is not a project for such approach.

The way I proposed to add the feature has many benefits: * cleaner enhancement, the existing algorithms not being affected at all * ability to use at the same time with other algorithms -- while you may want to do your distribution to transcoding servers, maybe the registrations are better with old style algorithm to know better where user end up registered on a server maintenance time frame * backward compatibility * follows the same approach with weight and rweight algorithms

Yours keeps the backward compatibility, nothing more adds to the decision complexity of setting modparams.

Besides the above aspects, I also had the specific code review remarks: declaration of variables at the beginning of blacks and the not-so-good second hashing which is done over an integer printed as string. For the second here, the scope of hashing is to get an integer from string, if you have already the integer, converting it to string to hash again is not really a good solution. Even from distribution point of view, if you have 3 server addresses in the set, then you will always has either "0", "1" or "2". Hashing 3 string values will return always 3 integers (hash id). I haven't spent time on analyzing much the code, but practically, if server 2 is down, then all traffic sent to it will be sent to the server corresponding to the hash id computed from "1", so you still send to another **single** server. Second hashing has to be done on something that can ensure better distribution.

As code issues in the patch, one seems to be a potential infinite loop in the line:

``` while (( maxRehash > 0 ) && ds_skip_dst(idx->dlist[fullHash % idx->nr].flags ) ); ```

Code indentation is also not following the style in the file. And those were what I could spot quickly so far.

Closed #2363.