git:master:6d87794c: ndb_redis: Added REDIS-Cluster support [BETA] - sr-dev

Daniel-Constantin Mierla

4 Apr 4 Apr

2:11 p.m.

New subject: git:master:6d87794c: ndb_redis: Added REDIS-Cluster support [BETA]

Hello, I haven't checked the merged version, but if I spotted correctly from the patch, it moves the redis response structure in shared memory. Is is right? If yes, why? Having it in shared memory will create races between kamailio processes that store results on same container id -- one worker can overwrite on data stored by another ine. Cheers, Daniel On 04.04.17 12:55, Carsten Bock wrote:

...

Module: kamailio Branch: master Commit: 6d87794cac3f6f3bb967d6e0bf6714a4d78f1bd5 URL: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Author: Carsten Bock <carsten(a)ng-voice.com> Committer: Carsten Bock <carsten(a)ng-voice.com> Date: 2017-04-04T12:55:26+02:00 ndb_redis: Added REDIS-Cluster support [BETA] --- Modified: src/modules/ndb_redis/doc/ndb_redis.xml Modified: src/modules/ndb_redis/doc/ndb_redis_admin.xml Modified: src/modules/ndb_redis/ndb_redis_mod.c Modified: src/modules/ndb_redis/redis_client.c Modified: src/modules/ndb_redis/redis_client.h --- Diff: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Patch: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Daniel-Constantin Mierla www.twitter.com/miconda -- www.linkedin.com/in/miconda Kamailio Advanced Training - May 22-24 (USA) - www.asipto.com Kamailio World Conference - May 8-10, 2017 - www.kamailioworld.com

Reply

Daniel-Constantin Mierla

6:02 p.m.

New subject: git:master:6d87794c: ndb_redis: Added REDIS-Cluster support [BETA]

Hi Carsten, On 04.04.17 14:43, Carsten Bock wrote:

...

Hi Daniel, i've just noticed, that in one line, I tried to free a reply from shmem instead of pkgmem (fixed).

ok, I see.

...

I only put the list of REDIS-Connections into shmem, so other workers can access (and re-use) the newly added connections to REDIS.

Is this safe? If there is a tcp connection behind and the structure references it via the file descriptor, then it is not going to work for connections opened after kamailio forks the child processes. Cheers, Daniel

...

Note to myself: I should add some locking to the list.... Thanks, Carsten 2017-04-04 13:11 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

Hello, I haven't checked the merged version, but if I spotted correctly from the patch, it moves the redis response structure in shared memory. Is is right? If yes, why? Having it in shared memory will create races between kamailio processes that store results on same container id -- one worker can overwrite on data stored by another ine. Cheers, Daniel On 04.04.17 12:55, Carsten Bock wrote:

Module: kamailio Branch: master Commit: 6d87794cac3f6f3bb967d6e0bf6714a4d78f1bd5 URL: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Author: Carsten Bock <carsten(a)ng-voice.com> Committer: Carsten Bock <carsten(a)ng-voice.com> Date: 2017-04-04T12:55:26+02:00 ndb_redis: Added REDIS-Cluster support [BETA] --- Modified: src/modules/ndb_redis/doc/ndb_redis.xml Modified: src/modules/ndb_redis/doc/ndb_redis_admin.xml Modified: src/modules/ndb_redis/ndb_redis_mod.c Modified: src/modules/ndb_redis/redis_client.c Modified: src/modules/ndb_redis/redis_client.h --- Diff: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Patch: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Daniel-Constantin Mierla www.twitter.com/miconda -- www.linkedin.com/in/miconda Kamailio Advanced Training - May 22-24 (USA) - www.asipto.com Kamailio World Conference - May 8-10, 2017 - www.kamailioworld.com

Reply

Carsten Bock

18 Apr 18 Apr

1:20 p.m.

New subject: git:master:6d87794c: ndb_redis: Added REDIS-Cluster support [BETA]

Hi Daniel, after some further testing, I have to admit, you are right. However, I've noticed another issue with ndb_redis, which is associated with this issue: The File-Discriptor is also corrupt, when reconnecting to ReDIS (e.g. connection was down or similar), as the new connection is done in the child process. I have a setup, where I connect to REDIS using stunnel and it happens quite frequently, that I have to reconnect. A workaround for this, is to store the information about the connection status in the structure as well. Once I had to reconnect (which happens from a child process, so corrupt file descriptors), I need to reconnect each time I use REDIS. This is not the nicest solution, but solves the issue about corrupt file-descriptors. I've had this change running for ~5 days now. Please let me know your thoughts. Thanks, Carsten 2017-04-04 17:02 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

...

Hi Carsten, On 04.04.17 14:43, Carsten Bock wrote:

Hi Daniel, i've just noticed, that in one line, I tried to free a reply from shmem instead of pkgmem (fixed).

ok, I see.

I only put the list of REDIS-Connections into shmem, so other workers can access (and re-use) the newly added connections to REDIS.

Is this safe? If there is a tcp connection behind and the structure references it via the file descriptor, then it is not going to work for connections opened after kamailio forks the child processes. Cheers, Daniel

Note to myself: I should add some locking to the list.... Thanks, Carsten 2017-04-04 13:11 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

Hello, I haven't checked the merged version, but if I spotted correctly from the patch, it moves the redis response structure in shared memory. Is is right? If yes, why? Having it in shared memory will create races between kamailio processes that store results on same container id -- one worker can overwrite on data stored by another ine. Cheers, Daniel On 04.04.17 12:55, Carsten Bock wrote:

Module: kamailio Branch: master Commit: 6d87794cac3f6f3bb967d6e0bf6714a4d78f1bd5 URL: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Author: Carsten Bock <carsten(a)ng-voice.com> Committer: Carsten Bock <carsten(a)ng-voice.com> Date: 2017-04-04T12:55:26+02:00 ndb_redis: Added REDIS-Cluster support [BETA] --- Modified: src/modules/ndb_redis/doc/ndb_redis.xml Modified: src/modules/ndb_redis/doc/ndb_redis_admin.xml Modified: src/modules/ndb_redis/ndb_redis_mod.c Modified: src/modules/ndb_redis/redis_client.c Modified: src/modules/ndb_redis/redis_client.h --- Diff: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… Patch: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… _______________________________________________ sr-dev mailing list sr-dev(a)lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- Daniel-Constantin Mierla www.twitter.com/miconda -- www.linkedin.com/in/miconda Kamailio Advanced Training - May 22-24 (USA) - www.asipto.com Kamailio World Conference - May 8-10, 2017 - www.kamailioworld.com

-- Carsten Bock CEO (Geschäftsführer) ng-voice GmbH Millerntorplatz 1 20359 Hamburg / Germany http://www.ng-voice.com mailto:carsten@ng-voice.com Office +49 40 5247593-40 Fax +49 40 5247593-99 Sitz der Gesellschaft: Hamburg Registergericht: Amtsgericht Hamburg, HRB 120189 Geschäftsführer: Carsten Bock Ust-ID: DE279344284 Hier finden Sie unsere handelsrechtlichen Pflichtangaben: http://www.ng-voice.com/imprint/

Reply

Carsten Bock

3:57 p.m.

New subject: git:master:6d87794c: ndb_redis: Added REDIS-Cluster support [BETA]

Hi Daniel, damn it! You are right again! You are the best.... I'll commit a fix for my patch later this week. Thanks, Carsten 2017-04-18 14:14 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

...

Hi Carsten, On 18.04.17 12:20, Carsten Bock wrote:

Hi Daniel, after some further testing, I have to admit, you are right. However, I've noticed another issue with ndb_redis, which is associated with this issue: The File-Discriptor is also corrupt, when reconnecting to ReDIS (e.g. connection was down or similar), as the new connection is done in the child process. I have a setup, where I connect to REDIS using stunnel and it happens quite frequently, that I have to reconnect. A workaround for this, is to store the information about the connection status in the structure as well. Once I had to reconnect (which happens from a child process, so corrupt file descriptors), I need to reconnect each time I use REDIS. This is not the nicest solution, but solves the issue about corrupt file-descriptors. I've had this change running for ~5 days now.

I expect you have to reconnect only once from each child when the redis server connection structure is in private memory. Maybe I don't understand exactly what do mean by 'reconnect each time'. I don't have any experience with redis cluster, but with single redis server each kamailio worker process needed to reconnect just once. Cheers, Daniel

Please let me know your thoughts. Thanks, Carsten 2017-04-04 17:02 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>:

Hi Carsten, On 04.04.17 14:43, Carsten Bock wrote:

Hi Daniel, i've just noticed, that in one line, I tried to free a reply from shmem instead of pkgmem (fixed).

ok, I see.

I only put the list of REDIS-Connections into shmem, so other workers can access (and re-use) the newly added connections to REDIS.

Is this safe? If there is a tcp connection behind and the structure references it via the file descriptor, then it is not going to work for connections opened after kamailio forks the child processes. Cheers, Daniel

Note to myself: I should add some locking to the list.... Thanks, Carsten 2017-04-04 13:11 GMT+02:00 Daniel-Constantin Mierla <miconda(a)gmail.com>om>: > Hello, > > I haven't checked the merged version, but if I spotted correctly from > the patch, it moves the redis response structure in shared memory. > > Is is right? If yes, why? Having it in shared memory will create races > between kamailio processes that store results on same container id -- > one worker can overwrite on data stored by another ine. > > Cheers, > Daniel > > On 04.04.17 12:55, Carsten Bock wrote: >> Module: kamailio >> Branch: master >> Commit: 6d87794cac3f6f3bb967d6e0bf6714a4d78f1bd5 >> URL: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… >> >> Author: Carsten Bock <carsten(a)ng-voice.com> >> Committer: Carsten Bock <carsten(a)ng-voice.com> >> Date: 2017-04-04T12:55:26+02:00 >> >> ndb_redis: Added REDIS-Cluster support [BETA] >> >> --- >> >> Modified: src/modules/ndb_redis/doc/ndb_redis.xml >> Modified: src/modules/ndb_redis/doc/ndb_redis_admin.xml >> Modified: src/modules/ndb_redis/ndb_redis_mod.c >> Modified: src/modules/ndb_redis/redis_client.c >> Modified: src/modules/ndb_redis/redis_client.h >> >> --- >> >> Diff: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… >> Patch: https://github.com/kamailio/kamailio/commit/6d87794cac3f6f3bb967d6e0bf6714a… >> >> >> _______________________________________________ >> sr-dev mailing list >> sr-dev(a)lists.sip-router.org >> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > -- > Daniel-Constantin Mierla > www.twitter.com/miconda -- www.linkedin.com/in/miconda > Kamailio Advanced Training - May 22-24 (USA) - www.asipto.com > Kamailio World Conference - May 8-10, 2017 - www.kamailioworld.com >

-- Daniel-Constantin Mierla www.twitter.com/miconda -- www.linkedin.com/in/miconda Kamailio Advanced Training - May 22-24 (USA) - www.asipto.com Kamailio World Conference - May 8-10, 2017 - www.kamailioworld.com

-- Carsten Bock CEO (Geschäftsführer) ng-voice GmbH Millerntorplatz 1 20359 Hamburg / Germany http://www.ng-voice.com mailto:carsten@ng-voice.com Office +49 40 5247593-40 Fax +49 40 5247593-99 Sitz der Gesellschaft: Hamburg Registergericht: Amtsgericht Hamburg, HRB 120189 Geschäftsführer: Carsten Bock Ust-ID: DE279344284 Hier finden Sie unsere handelsrechtlichen Pflichtangaben: http://www.ng-voice.com/imprint/

Reply