[asterisk-dev] [Code Review] 4490: astdb: Allow clustering of the Asterisk Database between multiple Asterisk servers
Matt Jordan
reviewboard at asterisk.org
Sun Apr 5 21:14:18 CDT 2015
> On April 2, 2015, 2:41 p.m., Mark Michelson wrote:
> > /trunk/main/db.c, lines 958-977
> > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line958>
> >
> > The dialplan function allows an omitted type to automatically be interpreted as "global" but the CLI command requires that the type is present.
> >
> > I'm not a fan of the inconsistency. My preference would be that the dialplan function is changed to require an explicit type.
Works for me.
> On April 2, 2015, 2:41 p.m., Mark Michelson wrote:
> > /trunk/main/db.c, lines 419-441
> > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line419>
> >
> > There is potential for deadlock due to lock inversion.
> >
> > When ast_db_put_shared() is called, it locks the shared_families container and then calls into db_put_common(), which locks the dblock.
> >
> > When ast_db_put() is called, it first calls into db_put_common(), which locks the dblock. Since ast_db_put() sets the "shared" parameter to 1 when calling db_put_common(), db_put_common() will call into db_entry_put_shared(), which will then try to lock the shared_families lock.
> >
> > Probably the easiest way to go here would be to unlock shared_families before calling db_put_common(), establishing a locking order of dblock and then shared_families.
Thanks for catching this! I think that locking order would be fine - and there's no real danger in unlocking the shared_families container after you've put the family in the container but before updating the key. Even if something else sneaks in and removes the shared family, we've already committed to changing the value.
> On April 2, 2015, 2:41 p.m., Mark Michelson wrote:
> > /trunk/main/db.c, lines 1674-1677
> > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line1674>
> >
> > family can't be NULL here since whatever strchr returns has 1 added to it.
This really should be:
family = strchr(cur->key + 1, '/');
if (!family) {
continue;
}
family++;
> On April 2, 2015, 2:41 p.m., Mark Michelson wrote:
> > /trunk/tests/test_db.c, lines 62-63
> > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73363#file73363line62>
> >
> > Any reason you went with a manually-resized array instead of an AST_VECTOR?
The code to synchronize and handle Stasis messages already had a template for it in another set of unit tests, so this was just adapted from that.
> On April 2, 2015, 2:41 p.m., Mark Michelson wrote:
> > /trunk/tests/test_db.c, lines 427-446
> > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73363#file73363line427>
> >
> > If any of these calls to ast_test_validate() fails the GLOBAL_SHARED_FAMILY and UNIQUE_SHARED_FAMILY will not be cleaned up.
> >
> > Actually, this pattern is common throughout the test code and should be addressed.
RAII_VAR to the rescue.
- Matt
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4490/#review15032
-----------------------------------------------------------
On March 29, 2015, 8:46 p.m., Matt Jordan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4490/
> -----------------------------------------------------------
>
> (Updated March 29, 2015, 8:46 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-24909
> https://issues.asterisk.org/jira/browse/ASTERISK-24909
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> As described on the asterisk-dev mailing list [1], I got some inspiration from seeing Kamailio's htable implementation, and thought a similar mechanism would work for the Asterisk Database. This patch is the result.
>
> This patch provides a mechanism to mark families within the AstDB as "shared." The marking of a family as shared is independent of the existance of the family, and is independent of the updates already made to the family. Shared families are subject to distribution with other Asterisk instances, as well as subject to updates from other Asterisk instances. Two strategies for sharing are implemented:
>
> * Global: A 'global' shared family shares the family/key space with all other Asterisk instances. Say we have shared family 'foo', and we have two Asterisk instances. Say the first Asterisk instance (ast1) updates a key in family 'foo' to the following:
>
> ast1
> /foo/key = bar
>
> The second Asterisk instance (ast2) would then receive an update in its AstDB:
>
> ast2
> /foo/key = bar
>
> If ast2 later updates the same key in its local AstDB to 'foobar', ast1 will receive a notification to update the same key in its AstDB:
>
> ast2
> /foo/key = foobar
> ast1
> /foo/key = foobar
>
> * Unique: A 'unique' shared family shares its values with other Asterisk instances, however, updates from other Asterisk instances are placed in unique families in the local AstDB for each Asterisk instance. Again, say we have shared family 'foo', and we have two Asterisk instances - ast1 and ast2. ast1 has an EID of 11:11:11:11:11:11, while ast2 has an EID of ff:ff:ff:ff:ff:ff. Say ast1 updates a key in family 'foo':
>
> ast1
> /foo/key = bar
>
> ast2 would receive the update for family 'foo', but instead of updating its local copy, it would instead store the value in a new family for ast1 corresponding to its EID:
>
> ast2
> /11:11:11:11:11:11/foo/key = bar
>
> If ast2 later updates the same ky in its local AstDB to 'foobar', the received value from ast1 will not be updated. However, ast1 will receive the update, and store the value in a new family for ast2 corresponding to its EID:
>
> ast2
> /foo/key = foobar
> /11:11:11:11:11:11/foo/key = bar
> ast1
> /foo/key = bar
> /ff:ff:ff:ff:ff:ff/foo/key = foobar
>
> In order to manipulate shared families, two new dialplan functions have been added, DB_SHARED and DB_SHARED_EXISTS. DB_SHARED allows for creation of a shared family, as well as deletion, while DB_SHARED_EXISTS returns whether or not a family is shared:
> same => n,Set(DB_SHARED(put,global)=foo) ; share family 'foo' globally
> same => n,Set(DB_SHARED(put,unique)=foobar) ; share family 'foobar' uniquely
> same => n,NoOp(${DB_SHARED_EXISTS(foo)}) ; returns '1'
> same => n,Set(DB_SHARED(delete)=foo) ; remove shared family status for 'foo'
>
> CLI commands were also added to create/delete shared families, and the output of 'database show|showkey' updated to show the shared status of a family/key/value tuple.
>
> Finally, a mechanism for sharing AstDB information was added to the PJSIP stack's res_pjsip_publish_asterisk. This includes a new event type, 'asterisk-db', which contains the values being created/deleted. Necessary configuration parameters were added to the existing configuration objects that support inbound/outbound PUBLISH support. An example of a PUBLISH request with the new event type is shown below:
>
> PUBLISH sip:ast1 at 127.0.0.1:5061 SIP/2.0
> Via: SIP/2.0/UDP 127.0.0.1:5060;rport;branch=z9hG4bKPj1eb182a7-a0aa-4d73-995d-d2ad4b096db2
> From: <sip:ast1 at 127.0.0.1>;tag=7b294642-06ae-4ecf-8637-db8ba2dc4397
> To: <sip:ast1 at 127.0.0.1>
> Call-ID: b9463adc-e364-440d-8ce1-842372813b08
> CSeq: 48111 PUBLISH
> Event: asterisk-db
> Expires: 3600
> Max-Forwards: 70
> User-Agent: Asterisk PBX SVN-mjordan-trunk-astdb-cluster-URL:-r432916M-/trunk
> Content-Type: application/json
> Content-Length: 156
>
> {"type":"dbstate","eid":"11:11:11:11:11:11","dbstate":{"verb":"put","family":"global_shared","share_type":"global","entries":[{"data":"foo","key":"key1"}]}}
>
>
> As a note on the power of the frameworks in Asterisk 13 - in this case, both Stasis and PJSIP - the vast bulk of this was written on two plane flights, plus a weekend or so of test writing and cleanup.
>
> [1] http://lists.digium.com/pipermail/asterisk-dev/2015-March/073192.html
>
>
> Diffs
> -----
>
> /trunk/tests/test_db.c 433716
> /trunk/res/res_pjsip_pubsub.c 433716
> /trunk/res/res_pjsip_publish_asterisk.c 433716
> /trunk/res/res_pjsip_outbound_publish.c 433716
> /trunk/main/utils.c 433716
> /trunk/main/db.c 433716
> /trunk/include/asterisk/utils.h 433716
> /trunk/include/asterisk/astdb.h 433716
> /trunk/funcs/func_db.c 433716
> /trunk/configs/samples/pjsip.conf.sample 433716
> /trunk/CHANGES 433716
>
> Diff: https://reviewboard.asterisk.org/r/4490/diff/
>
>
> Testing
> -------
>
> Existing AstDB tests execute successfully, as do the existing PJSIP PUBLISH tests. In addition:
>
> * Unit tests were written (included in this review) that verify the API additions to the AstDB. This includes:
> - Verification of creation/delation of shared families
> - Verification that creating/deleting an AstDB entry in a shared family publishes the correct Stasis message
> - Verification that all keys/values in all shared families can be published via a 'refresh'
>
> * Asterisk Test Suite tests for the PJSIP PUBLISH distribution of the AstDB information are available at: https://reviewboard.asterisk.org/r/4508/
>
>
> Thanks,
>
> Matt Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150406/46de8de8/attachment-0001.html>
More information about the asterisk-dev
mailing list