Hi @miconda
As we spoke yesterday, this is the PR to add the option about sending EVAPI data to a specific destination instead to all hosts.
Could you review/add comments?
Regards and thanks for the help!
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/596
-- Commit Summary --
* evapi: Added destination address on evapi_relay
-- File Changes --
M modules/evapi/doc/evapi_admin.xml (13) M modules/evapi/evapi_dispatch.c (131) M modules/evapi/evapi_dispatch.h (2) M modules/evapi/evapi_mod.c (148)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/596.patch https://github.com/kamailio/kamailio/pull/596.diff
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596
There is a shm memory leak because the data is not allocated along with the evapi_relay_cmd structure.
Also, the indentation seems to be done with whitespaces instead of tabs, breaking the formatting of the blocks.
Thinking more of it, I think it could be rather hard to match on source IP and port for tcp connections, because the kernel may allocate random ports, even one tries to bind on a specific one.
Maybe we can add sort of connection tagging (or channel name) - when the connection is open by the client app, it gives the tag/channel name. Or eventually, a new function can be exported to set the tag/channel name inside the event_route[evapi:connection-new]. The tag/channel name will be a new field in the connection structure. Then the evapi relay will match on this name rather than source ip port. What do you think?
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-216663484
Hey @miconda
About the memory leak, I think that now is fixed but if you can confirm I'll be great. About indention, should be ok now. Sorry!
About tags/IP:PORT. When I did this I thought about tagging, but after a few changes I used HOST:PORT. We use evapi mainly with Cgrates, but in the distributed environment when we have n instances of Cgrates we need to send the data to each cgrates, if we send to all instances will be billed few times.
The idea to use host:port is mainly about host and port is given in the `evapi:connection-new`, so the info can be saved in htables + can be compared with the service discovery tool (in our case consul), so I think that is the easiest way.
If we use tag (uuid for example) more code will be needed in kamailio.cfg, so the simple way that I found is using host+port. I spoke yesterday with @danbogos about this, and he was agree with my use case.
If you need I can squash the commits into one, the second one doesn't add a real value. Regards
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-216828268
If you get the IP:PORT in event route, then you can also set the tag there and can be inside the evapi connection structure, no need for htable anymore. You need to keep the relation of the IP:PORT and service somewhere, same can be done also with the tag name and service.
The benefit is that the tagging allows two or more connections to be tagged with same name and then send the message to all of these ones. Practically, more granularity, allowing to send to one, many and all. Also, at some point one may want to add transport as a constraint there, not only the address and port.
You indented now everything there, apparently there was other code not indented with tags, but now the patch looks bigger.
For the new structure, the data field can be str, not pointer to it, because it is all in memory. Also, all should be allocated at once, like it was before, making easier to deallocate and simplify the code in the evapi side.
Anyhow, I will add the support for tagging and get the framework with a data structure, you can then add support for ip:port if you need specific this case -- I am fine with that.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217374772
Hey Daniel,
Agree with the label, make sense so I can add.
If you want I can do, I need this for us in the following days, so If you want I'll do at the beginning of the next week.
Regards
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217377376
Hi Eloy, I added the framework for tagging connections to simplify the development of further extensions via sending a structure packaged at once.
I haven't extended the config functions for evapi relay, because I thought we may need to decide on naming.
We can add a second parameter for the tags in evapi_*relay() or we can add a new pair of functions: evapi_relay_multicast(data, tag), plus the one for async.
Then, if you want to filter by ip:port, then evapi_relay_unicast(data, ip, port) can be added. Even when sticking to tags only, could make sense to have evapi_relay_unicast(data, tag) which stops after matching first tag. The multicast is walking to all connections to see if the tag matches.
This unicast/multicast can be a new field in evapi_msg_t as an integer, so it won't need any speciall allocation to pass to evapi workers.
What do you think?
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217393993
Hey Daniel,
Tags make sense for me, and I love the concept TBH. Related with multicast functions I'm totally agree and I think that is too useful.
About the unicast, It's ok for me and should work with Cgrates too, the only thing that maybe I'm worried about that is that unicast is going to send the info to the first server connected, so maybe added a option to sort the connections, I mean maybe you're interested to connect to the last connected instead the first one.
In the other hand, you can use labels, but maybe a few RPC methods need to be written, I mean what happens when a server is going to be inactive and you want to delete a tag for specific connection? If you can only assign that tag in the [evapi:new-connection] you'll need to work in the endpoint and I think that it's kamailio configuration.
What do you think? Regards
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217398400
Right now there is one tag per connection support -- you can extend it if you need more, as well as rpc commands to control connections. You can add timestamp per connection and have further rules to select it for a unicast.
I pushed cfg multicast functions. I let testing (and unicast implementation if you want it) for you.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217406766
Hi,
I've just updated the PR:
- In multicast function I fix the issues with the parameters, was only 1. - Added evapi_unicast and evapi_async_unicast functions. - (evapi_dispatch.c) Rename evapi_relay_multicast, to _evapi_relay and provide 3 different functions to relay.
Should be ok now, but if you can confirm. I'll check in our app, and if I need more functions I'll added in the near future.
Thanks
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217450702
Thanks!
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#issuecomment-217612812
Merged #596.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/596#event-653368669
-
Daniel-Constantin Mierla -
Eloy Coto