#### 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 - [*] Commits are split per component (core, individual modules, libs, utils, ...) - [*] Each component has a single commit (if not, squash them into one commit) - [*] 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) - [*] 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 - [*] Tested changes locally - [*] Related to issue #4249
#### Description When sending an INVITE using t_uac_send, the transaction layer automatically generates ACK on final response. However this ack doesn't invoke the tm:local-request callback. This patch adds this functionality You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4250
-- Commit Summary --
* tm: invoke tm:local-request on generated ACK messages
-- File Changes --
M src/modules/tm/uac.c (127)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4250.patch https://github.com/kamailio/kamailio/pull/4250.diff
miconda left a comment (kamailio/kamailio#4250)
I wonder if we should add a modparam to be able to enable this, the existing configs will get get "unexpectedly" the local ACKs in the event route starting with the next major release.
henningw left a comment (kamailio/kamailio#4250)
@miconda Its more a bug fix, similar as we have done e.g. in faca8e7a20255d90 and ccc1069228
The tm module documentation clearly states that it should work for all locally generated requests: "Executed when the tm module generates a request, in other words, the request originates from Kamailio and a transaction is created for it. The source of the requests can be tm itself or other modules that use tm internally (e.g., msilo, uac)."
I don't think we need to add a module parameter for it, documenting it clearly in the migration guide should be fine.
tsearle left a comment (kamailio/kamailio#4250)
Is there something I need to do ref updating the release notes?
miconda left a comment (kamailio/kamailio#4250)
I started travelling and this one got out of my sight. I wanted to check if the event route is executed only for transactions started locally, it is also for hop-by-hop ACKs generated on failed received+forwarded INVITEs.
Same question would be also for the CANCEL requests (https://github.com/kamailio/kamailio/commit/faca8e7a20255d90a4786fd4043005ea... and https://github.com/kamailio/kamailio/commit/ccc106922802213253f03f181d10fe83...), is the event route executed only for CANCELs of locally initiated INVITEs, or for all hop-by-hop CANCELs?
If the event route is executed only for ACK/CALCEL of locally initiated INVITEs, then I am fine with no new modparam, but if it also executed for cases of the hop-by-hop ACK/CALCEL of received+forwarded INVITEs, then I consider that the impact can be significant for instances with high SIP traffic and I would prefer to be able to control via a modparam.
henningw left a comment (kamailio/kamailio#4250)
Thanks, I think the same. I would expect that the new behaviour only applies to locally generated requests. @xkaraman please have a look to the code regarding this PR and the referenced commits above in this matter. It might be easier to just do a bunch of tests on one of our test systems.
tsearle left a comment (kamailio/kamailio#4250)
@miconda says this pr it can be merged if needed now, and later we can add a param to control it better if executes for all acks, not only those for local invites ...
Merged #4250 into master.
henningw left a comment (kamailio/kamailio#4250)
This seems to have introduced a new warning:
CC (gcc) [M tm.so] uac.o In file included from ../../core/mem/shm.h:43, from ../../core/mem/shm_mem.h:32, from uac.c:31: uac.c: In function ‘local_ack_rb’: ../../core/mem/../dprint.h:360:22: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 11 has type ‘long unsigned int’ [-Wformat=] 360 | fprintf(stderr, "%2d(%d) %s: %.*s%s%s%s" fmt, process_no, \ | ^~~~~~~~~~~~~~~~~~~~~~~~ ../../core/mem/../dprint.h:388:2: note: in expansion of macro ‘LOG_FX’ 388 | LOG_FX(facility, level, lname, prefix, _FUNC_NAME_, fmt, ##args) | ^~~~~~ ../../core/mem/../dprint.h:394:2: note: in expansion of macro ‘LOG_FL’ 394 | LOG_FL(facility, level, NULL, prefix, fmt, ##args) | ^~~~~~ ../../core/mem/../dprint.h:397:2: note: in expansion of macro ‘LOG_FP’ 397 | LOG_FP(DEFAULT_FACILITY, (level), LOC_INFO, fmt, ##args) | ^~~~~~ ../../core/mem/../dprint.h:450:27: note: in expansion of macro ‘LOG’ 450 | #define ERR(fmt, args...) LOG(L_ERR, fmt, ##args) | ^~~ ../../core/mem/../dprint.h:499:16: note: in expansion of macro ‘ERR’ 499 | #define LM_ERR ERR | ^~~ ../../core/mem/shm.h:142:2: note: in expansion of macro ‘LM_ERR’ 142 | LM_ERR("could not allocate shared memory from shm pool - " fmt, ##args) | ^~~~~~ uac.c:909:3: note: in expansion of macro ‘SHM_MEM_ERROR_FMT’ 909 | SHM_MEM_ERROR_FMT("required (%u)\n", sizeof(dlg_t)); | ^~~~~~~~~~~~~~~~~
xkaraman left a comment (kamailio/kamailio#4250)
@miconda @tsearle
After some brief tests and evaluation here are some remarks:
1. Indeed previous commits (https://github.com/kamailio/kamailio/commit/faca8e7a20255d90a4786fd4043005ea... and https://github.com/kamailio/kamailio/commit/ccc106922802213253f03f181d10fe83...) for CANCEL produce the event for hop-by-hop which is not required. I will see what can be done about this. 2. Using this patch, the result for RPC command seems to be not what is described: Issuing the below command for a real invite to a registered number `sudo sbin/kamctl rpc tm.t_uac_start INVITE sip:alice2@ip_proxy:50739 . . From:sip:alice2@app01.com\r\nTo:sip:alice@app01.net\r\n` does produce event for the INVITE as expected but NOT for the ACK as this patch suggest. 3. I couldn't verify it but this patch seems to suggest that the change is related to the `t_uac_send` kamialio config function and it's not used accross, like for example in RPC methods.
tsearle left a comment (kamailio/kamailio#4250)
@xkaraman thanks for you testing, indeed my testing was done against the `t_uac_send ` config function/kemi method
tsearle left a comment (kamailio/kamailio#4250)
@xkaraman just to confirm, your build has the flag WITH_EVENT_LOCAL_REQUEST set?
as far as I can tell there should be no different between invoking via rpc and config script should end up at this call https://github.com/kamailio/kamailio/blob/4de3270830c08641e3bc3e585851324408...
and local_ack_rb will invoke the event callback if WITH_EVENT_LOCAL_REQUEST compile flag is set
xkaraman left a comment (kamailio/kamailio#4250)
yep i got mutiple other events emitted and confirmed that the define is used ofc!
I did some review of the code as well it should be what you described, but it seems something else is missing!
Initiating an INVITE from rpc as described above, yields a local request event for the INVITE and logging it but not for the ACK. I can see the ACK message using the `sngrep` sent but not the event.
henningw left a comment (kamailio/kamailio#4250)
This already get backported to stable branch aparently. @tsearle Could you maybe quickly test the "kamctl rpc tm.t_uac_start" method if you can reproduce what @xkaraman reported? If we need longer to clarify if the code works as intended we probably should revert it in 6.0.
tsearle left a comment (kamailio/kamailio#4250)
`kamctl rpc tm.t_uac_start INVITE sip:siprec@54.220.60.194 sip:siprec@10.151.0.55:5080 . "From: sip:3228080970@54.228.98.158:5060\r\nTo: sip:siprec@35.71.164.202\r\nRoute: <sip:siprec@10.151.0.55:5080;transport=UDP;lr>\r\nContact: <sip:siprec@10.151.0.238>\r\nX-Voicenet-SipReq: yes\r\n" `
I get the following `Jun 11 07:58:29 dev-ew1-blue-app-238 /usr/sbin/kamailio[2605]: { "level": "INFO", "module": "core", "file": "core/kemi.c", "line": 109, "function": "sr_kemi_core_info", "callid": "5cd47ca80f8b0732-2616@10.151.0.238", "logprefix": "{INVITE 200 10.151.0.55 5cd47ca80f8b0732-2616@10.151.0.238 } ", "message": "tm_event: tm:local-request method=ACK" } `
tsearle left a comment (kamailio/kamailio#4250)
@xkaraman your callflow was it a successful call or an un-successful call? Non-2xx acks are handled by the transaction layer and not the UA layer.
miconda left a comment (kamailio/kamailio#4250)
The ACKs for 200ok are end-to-end, should not be generated by Kamailio, just forwarded (stateless). But the ACKs for 300+ replies of received+relayed INVITEs are hop-by-hop, and they should not trigger by default the event route (it can be via a modparam).
tsearle left a comment (kamailio/kamailio#4250)
@tsearle I am hoping that is the difference between my test and the other test :-)
xkaraman left a comment (kamailio/kamailio#4250)
@tsearle The ACK response i see is for an unsuccessful call. I got a 400 Bad Request response from my client after the invite.
tsearle left a comment (kamailio/kamailio#4250)
Ok, so not seeing the ACK is expected then. Could you test an ok Flow?
xkaraman left a comment (kamailio/kamailio#4250)
@tsearle Yep, it matches your expectations. Getting an OK from the client and then ACK from proxy, will emit the event!
henningw left a comment (kamailio/kamailio#4250)
It got a bit confusing for me, so I tried to summarize the different cases:
1. INVITE between phones A-Proxy-B, no ACK logging in local request event route for 200 OK and non 200 OK
This seems to be working as expected with the PR according to the tests.
3. INVITE generated between Proxy - B with 200 OK, ACK logging in local request event route
This seems to be working according the tests from @tsearle and @xkaraman
5. INVITE generated between Proxy - B with non 200 OK, ACK logging in local request event route
This seems to be not working according to the tests from @xkaraman, @tsearle can you also confirm?
tsearle left a comment (kamailio/kamailio#4250)
@henningw Correct, only 2xx OK to Generated INVITE will have an ACK in the local request. I tested the 2xx case, @xkaraman tested the non-2xx case, I am not sure if he retested with a 2xx case.
henningw left a comment (kamailio/kamailio#4250)
@henningw Correct, only 2xx OK to Generated INVITE will have an ACK in the local request. I tested the 2xx case, @xkaraman tested the non-2xx case, I am not sure if he retested with a 2xx case.
Thanks for the feedback. But it should trigger the event route also for the ACK in the non 200 OK case 3, right?
tsearle left a comment (kamailio/kamailio#4250)
@henningw no, as @miconda mentions.
The ACKs for 200ok are end-to-end, should not be generated by Kamailio, just forwarded (stateless). But the ACKs for 300+ replies of received+relayed INVITEs are hop-by-hop, and they should not trigger by default the event route (it can be via a modparam).
In summary, UAC is only responsible for generating ACK for 2xx responses. ACKs to Error responses are in-transaction (and not in dialog) and should be a completely different feature if desired.
henningw left a comment (kamailio/kamailio#4250)
@henningw no, as @miconda mentions.
The ACKs for 200ok are end-to-end, should not be generated by Kamailio, just forwarded (stateless). But the ACKs for 300+ replies of received+relayed INVITEs are hop-by-hop, and they should not trigger by default the event route (it can be via a modparam).
In summary, UAC is only responsible for generating ACK for 2xx responses. ACKs to Error responses are in-transaction (and not in dialog) and should be a completely different feature if desired.
If the Kamailio generates the INVITE, the non-200 OK ACKS are originated from the Kamailio. But I agree, they are not a separate transaction. I just checked the tm module documentation, it is also documented like this, great. Xenofon is reviewing the CANCEL case, but that should be probably discussed elsewhere.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
tsearle
-
Xenofon Karamanos