<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [ ] 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 - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> When we use DNS failover in case of timeout kamailio executes the branch_failure route. In the case of the 503 - on_reply route. This commit is to equate behavior in both cases. So if there are more candidates we will always execute branch_failure route and on_reply route only if this is the last candidate. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3138
-- Commit Summary --
* branch_failure route in case 503 and dns failover
-- File Changes --
M src/modules/tm/t_reply.c (52)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3138.patch https://github.com/kamailio/kamailio/pull/3138.diff
The onreply_route is designed for executing when a sip reply is received and should be kept like that otherwise many configs will get unexpected results. Even without dns failover, the onreply route is not executed on retransmission timeout, but the failure route block.
Branch failure event route was added later and I am not sure how coherent its behaviour is. If it is not executed on 503, the it should be. But also onreply_route should be executed on 503 when the response is received from the network.
From my perspective, if you want to change when the onreply route is executed, you have to make it configurable, via a modparam, with the default behaviour being the one so far.
@vlitvinov pushed 1 commit.
0b351170023ea88e3d6b613f119554bea5c99607 branch_failure route in case 503 and dns failover
You have to also document the new module parameter in the doc/ folder of tm. PR can be merged once the docs are update and no other comments to PR are made.
@vlitvinov any update from your side?
@vlitvinov pushed 1 commit.
c7c8084d7dd05602cfb38dc3a8e71e8ee9dce0f8 branch_failure route in case 503 and dns failover
This got somehow out of sight for pretty long, but looking now at combined diff it seems that order of code execution changes and can have significant impact. As I understand, the loop with `t_send_branch()` is now executed before `onreply_route` handling. If I got it right, that can have unwanted effects, like sending out new branches before handling the response.
This PR needs proper review and testing before merging.
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
This is the main idea do not execute on_reply route in case 503 and available failover endpoint. The same behavior as with timeout. There is no possibility to know did we try all candidates. In our case when we use the rtpengine we do delete but after that we can have successful reply from other failover candidate. As I understand the behavior will change only in case when reply status will be changed in the reply route (to or from 503 in default). So only in case when we want to try or visa-versa don't want to try other candidates, but again we don't know anything about candidates. This changes were done more than year ago and already works on few thousands installations without any problems (but only with new behavior).
Please let me know if any additional checks can be done from my side.
Thank you for providing the update. Right now there seems to be some conflicts probably after some refactoring changes that have been done. Could you maybe re-base your changes and then developers could have another close look.
@vlitvinov pushed 3 commits.
30f4133c8e39c22cba63466d11391b1cf443ecb6 branch_failure route in case 503 and dns failover 9364ca5b71b42a528d88fe0ca4c275a3970c49a6 branch_failure route in case 503 and dns failover 74babe6a41a54b528c697de01d2746b31f65e623 branch_failure route in case 503 and dns failover
Hello. Thank you for your reply. Re-based.
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
Closed #3138.
Reopened #3138.
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
Closed #3138.
-
Daniel-Constantin Mierla -
github-actions[bot]
-
Henning Westerholt
-
vlitvinov