#### 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: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
This change allows to enable/disable the insertion of SIP traces into the DB at runtime (i know we could do this with `siptrace.status`, but this is a different case). This feature is strictly important if you start `Kamailio` with `trace_to_database` parameter disabled, and you want to enable it later and at runtime. Nowadays, if you try to start `Kamailio` with `trace_to_database` enabled, but without a connection to the DB, the service won't start (`child_init` logic). In this type of situation, it makes sense to start `Kamailio` disabling the `trace_to_database` parameter, and when the DB is ok, at runtime, we enable `trace_to_database` parameter. This prevents the need to restart the service, and also we can start `Kamailio` without problems, if DB of SIP Traces is NOK at that time.
My PR may need some improvements, especially on the enable side, so feel free to make any changes to improve that.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3877
-- Commit Summary --
* siptrace: handle trace_to_database param at runtime
-- File Changes --
M src/modules/siptrace/siptrace.c (100)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3877.patch https://github.com/kamailio/kamailio/pull/3877.diff
Thanks for the PR. I did not reviewed the code yet, but noticed that the format check is failing. Could you please fix the format by executing clang-format on the file(s) and do a force-push?
@bundasmanu pushed 1 commit.
14d5c67ceec97ef61d1f03d6fad0582b2223cc02 siptrace: apply clang-format
@bundasmanu pushed 3 commits.
9bc8939cc9b529e9a414a302b67eb9dc971c885d Revert "siptrace: apply clang-format" a5071a1aa1cfe8a34fdb3e75ec0ed5d694750f4d Revert "siptrace: handle trace_to_database param at runtime" e0110aa4f59a87ef0a830b69141b1a14b06be34e siptrace: handle trace_to_database param at runtime
@bundasmanu pushed 0 commits.
@bundasmanu pushed 0 commits.
Closed #3877.
Reopened #3877.
I am not sure if this is actually working, or at least some operations to be done in the rpc command callback. Have you tested it?
Checking the DB capability and opening the DB connection inside the rpc command callback is not enough, because the module write from SIP workers not from RPC worker process. Actually, the RPC worker process might not need database connection at all, because it might never handle SIP traffic.
Are the SIP workers also opening connections after the flag is enabled via RPC? The PR does not seem to have code for it.
I am not sure if this is actually working, or at least some operations to be done in the rpc command callback. Have you tested it?
Checking the DB capability and opening the DB connection inside the rpc command callback is not enough, because the module write from SIP workers not from RPC worker process. Actually, the RPC worker process might not need database connection at all, because it might never handle SIP traffic.
Are the SIP workers also opening connections after the flag is enabled via RPC? The PR does not seem to have code for it.
Hi @miconda,
Yes, i have tested this changes. And seems to work fine. The logic i introduce is similar (equal) to the one applied inside `child_init`. Example of test (startup without connection to DB):
```sh modparam("siptrace", "db_url", "MY_DATABSE_CONNECTION") modparam("siptrace", "table", "sip_trace") modparam("siptrace", "trace_flag", FLT_SIPTRACE) modparam("siptrace", "trace_sl_acks", 0) modparam("siptrace", "trace_to_database", 0) modparam("siptrace", "trace_on", 0) ```
Doing a `kamcmd siptrace.database enable`, SIP Traces are now added to database. In the inverse order, the opposite logic happens.
But, feel free to test, and also add additional things, if needed.
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.
Somehow this got out of focus, but reviewing agan, I can't see how this is working unless kamailio is started with:
``` modparam("siptrace", "trace_to_database", 1) ```
so every Kamailio worker process get the connection to database initialized at startup. Then tracing to database can be disabled/(re-)enabled during runtime.
With this PR, if trace_to_database=0 at startup, then the global static db_con is NULL for worker processes and the RPC command initializes it only for the RPC process, it cannot do it for all the other Kamailio processes. There, db_con stays NULL, eventually resulting in crashes or not writing to database.
Somehow this got out of focus, but reviewing agan, I can't see how this is working unless kamailio is started with:
modparam("siptrace", "trace_to_database", 1)so every Kamailio worker process get the connection to database initialized at startup. Then tracing to database can be disabled/(re-)enabled during runtime.
With this PR, if trace_to_database=0 at startup, then the global static db_con is NULL for worker processes and the RPC command initializes it only for the RPC process, it cannot do it for all the other Kamailio processes. There, db_con stays NULL, eventually resulting in crashes or not writing to database.
Hi @miconda,
From the tests i made, i don't have problems, but i see your point. What do you suggest, to improve? Can you help here?
Thanks
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 #3877.
-
Daniel-Constantin Mierla -
github-actions[bot]
-
Gustavo Almeida
-
Henning Westerholt