[asterisk-dev] [Code Review] 4490: astdb: Allow clustering of the Asterisk Database between multiple Asterisk servers
Mark Michelson
reviewboard at asterisk.org
Thu Apr 2 14:41:52 CDT 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4490/#review15032
-----------------------------------------------------------
/trunk/funcs/func_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25701>
There are two ways you can go here:
1) Omit the highlighted section. Since you've already ensured that an argument has been given to DB_SHARED_EXISTS, you can take whatever has been input as the name of the family, with no further parsing.
2) If you want to ensure that no more than a single argument is passed in, then keep this code here but change the warning message to indicate that too many arguments were passed to the function.
The reason I'm not going to advocate one approach over the other is that AFAIK, there is nothing currently that prevents a DB family from having a comma in its name, since DB sections are divided by forward slashes.
/trunk/funcs/func_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25702>
I think this is impossible since you've already ensured that parse is not zero-length. What string could result in args.argc being less than 1?
/trunk/funcs/func_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25703>
I suggest moving this to before parsing the function arguments since it's an easy thing to check and bail out on.
/trunk/main/db.c
<https://reviewboard.asterisk.org/r/4490/#comment25704>
The clone local variable doesn't really serve a purpose.
/trunk/main/db.c
<https://reviewboard.asterisk.org/r/4490/#comment25705>
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.
/trunk/main/db.c
<https://reviewboard.asterisk.org/r/4490/#comment25706>
The same lock inversion with db_put_common exists here for db_del_common
/trunk/main/db.c
<https://reviewboard.asterisk.org/r/4490/#comment25707>
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.
/trunk/main/db.c
<https://reviewboard.asterisk.org/r/4490/#comment25708>
family can't be NULL here since whatever strchr returns has 1 added to it.
/trunk/tests/test_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25710>
Any reason you went with a manually-resized array instead of an AST_VECTOR?
/trunk/tests/test_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25709>
Nitpick:
s/othet/other/
/trunk/tests/test_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25711>
Grammar nitpick:
Either:
"...that the AstDB handlers for that topic have written..."
or
"...that the AstDB handler for that topic has written..."
/trunk/tests/test_db.c
<https://reviewboard.asterisk.org/r/4490/#comment25713>
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.
- Mark Michelson
On March 30, 2015, 1:46 a.m., Matt Jordan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4490/
> -----------------------------------------------------------
>
> (Updated March 30, 2015, 1:46 a.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/20150402/6491f157/attachment-0001.html>
More information about the asterisk-dev
mailing list