Details

    • Type: New Feature New Feature
    • Status: Closed
    • Severity: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Target Release Version/s: None
    • Component/s: Applications/app_queue
    • Labels:
      None
    • Mantis ID:
      4037
    • Regression:
      No

      Description

      This is a patch to app_queue.c that enables dynamic realtime for queues.
      Queue parameters and member list for a queue are loaded from realtime
      each time the Queue application command is called, so that updates are
      available immediately without the need for an explicit reload.

      Disclaimer has been faxed on April 15, 2005. The disclaimer concerns
      Sifira A/S, which is the company that employs me and owns the copyright
      for the code.

                • ADDITIONAL INFORMATION ******

      To enable the use of dynamic realtime for queues, first create the table
      in the database. For MySQL:

      CREATE TABLE queues_table (
      id INT(11) NOT NULL AUTO_INCREMENT, /* Optional */
      category VARCHAR(128) NOT NULL,
      var_name VARCHAR(128) NOT NULL,
      var_val VARCHAR(128) NOT NULL,
      PRIMARY KEY (id)
      );

      Ensure a proper realtime driver configuration (eg. res_mysql.conf).

      Add a suitable family in extconfig.conf:

      queue => mysql,mydb,queues_table

      Finally add the following entry to queues.conf:

      realtime_family = queue

      to tell app_queue.c to use the newly defined realtime family.

      If a queue name is defined statically in queues.conf, it will not be
      looked up in dynamic realtime.

      After this, queues can be defined / modified in the queues_table and be
      immediately available without reload. Queues are defined in the table
      with the same parameters used in the config file. The `category' column
      is the queue name, the `var_name' column is the parameter name, and the
      `var_val' column is the parameter value. For example:

      INSERT INTO queues_table VALUES (1,'my-example-queue','strategy','roundrobin');
      INSERT INTO queues_table VALUES (2,'my-example-queue','music','default');
      INSERT INTO queues_table VALUES (3,'my-example-queue','timeout','10');
      INSERT INTO queues_table VALUES (4,'my-example-queue','retry','0');
      INSERT INTO queues_table VALUES (5,'my-example-queue','maxlen','0');
      INSERT INTO queues_table VALUES (6,'my-example-queue','member','SIP/111@mysip');
      INSERT INTO queues_table VALUES (7,'my-example-queue','member','SIP/222@mysip');
      INSERT INTO queues_table VALUES (8,'my-example-queue','member','SIP/333@mysip');

      This database setup is chosen to resemble closely the one used in static
      realtime, so you should be able to use the same rows for static and
      dynamic realtime. For now I have chosen this for simplicity; another
      attractive and just-as-easy-to-implement possibility would be to have
      two tables: One `queue' table with one row per queue and one column for
      each parameter; and one `queue_member' table with one row per member and
      columns for member interface and penalty. The latter model is perhaps
      prettyer from a relatinal database perspective.

      Other than that I will add documentation to the Wiki (or another
      appropriate place if/when this is accepted into CVS.

      Comments welcome, of course.

        Activity

        No builds found.
        knielsen created issue -
        Hide
        Brian West added a comment -

        This is actually done kinda wrong. Shouldn't the column name match the var.. and the contents of the column be the value? This is pretty much automagic right?

        /b

        Show
        Brian West added a comment - This is actually done kinda wrong. Shouldn't the column name match the var.. and the contents of the column be the value? This is pretty much automagic right? /b
        Hide
        knielsen added a comment -

        bkw918, not sure if I understand you right, but I think you were expecting a database model like this:

        CREATE TABLE queue_table (
        name VARCHAR(128) PRIMARY KEY,
        strategy VARCHAR(32),
        music VARCHAR(128),
        timeout INT(11),
        retry INT(11),
        maxlen INT(11),
        ...
        );

        CREATE TABLE queue_member_table (
        queue_name varchar(128),
        interface varchar(128),
        penalty INT(11),
        PRIMARY KEY (queue_name, interface)
        );

        I do not have any strong preferences for either one, so in the end I choose the one I found was the simplest. Either model is easy to implement, so I am happy to be convinced otherwise.

        I do not get your point that this should be automagic ... ? Yes storing queues.conf in static realtime is automatic. But then changes do not take effect until the next reload, whereas the whole point of this patch is that changes in the database take effect immediately without a reload.

        Show
        knielsen added a comment - bkw918, not sure if I understand you right, but I think you were expecting a database model like this: CREATE TABLE queue_table ( name VARCHAR(128) PRIMARY KEY, strategy VARCHAR(32), music VARCHAR(128), timeout INT(11), retry INT(11), maxlen INT(11), ... ); CREATE TABLE queue_member_table ( queue_name varchar(128), interface varchar(128), penalty INT(11), PRIMARY KEY (queue_name, interface) ); I do not have any strong preferences for either one, so in the end I choose the one I found was the simplest. Either model is easy to implement, so I am happy to be convinced otherwise. I do not get your point that this should be automagic ... ? Yes storing queues.conf in static realtime is automatic. But then changes do not take effect until the next reload, whereas the whole point of this patch is that changes in the database take effect immediately without a reload.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        I think the two-table model is much preferable. It would be the first use of a multi-level structure in Asterisk realtime, but I think it's far more logical.

        If you are willing to implement it this way, I'll take a look and get it committed as quickly as possible, because I know there are people that want this

        Show
        Kevin P. Fleming (Inactive) added a comment - I think the two-table model is much preferable. It would be the first use of a multi-level structure in Asterisk realtime, but I think it's far more logical. If you are willing to implement it this way, I'll take a look and get it committed as quickly as possible, because I know there are people that want this
        Hide
        knielsen added a comment -

        Kevin, I will implement the two-level structure (I kind of prefer it myself).

        I should have a new patch ready sometimes next week.

        I am thinking that I should use two database families (to specify the two
        tables). This would make it possible to place the two tables under different
        drivers, which is kind of strange but would actually work, I think.

        Another annoyance is that with the current realtime framework I will need to
        use two separate transactions to pull the queue data from two tables. So the
        issue pops up what happens if changes happen between the two transactions. I
        think things are mostly ok if I select from queue_table first and
        queue_member_table second, though.

        Or do you want me to add a new multi-table single-transaction method to the
        realtime framework (I would be able to add it only for the MySQL driver)? I
        think we should try to avoid that...

        Show
        knielsen added a comment - Kevin, I will implement the two-level structure (I kind of prefer it myself). I should have a new patch ready sometimes next week. I am thinking that I should use two database families (to specify the two tables). This would make it possible to place the two tables under different drivers, which is kind of strange but would actually work, I think. Another annoyance is that with the current realtime framework I will need to use two separate transactions to pull the queue data from two tables. So the issue pops up what happens if changes happen between the two transactions. I think things are mostly ok if I select from queue_table first and queue_member_table second, though. Or do you want me to add a new multi-table single-transaction method to the realtime framework (I would be able to add it only for the MySQL driver)? I think we should try to avoid that...
        Hide
        Brian West added a comment -

        Well the way to do it is already there.. realtime will return an astconfig object filled out for you... no need to parse the var val pair crap outside the already existing framework.

        /b

        Show
        Brian West added a comment - Well the way to do it is already there.. realtime will return an astconfig object filled out for you... no need to parse the var val pair crap outside the already existing framework. /b
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        Except that doesn't work very well for multiple instances of the same variable name in a context, as is needed in queues.conf for "member =>" support.

        Here's anohter thought, although it's more radical (but along the lines of the current discussion about redesigning voicemail.conf). What about something like this:

        [general]
        ...

        [sales_queue]
        strategy = ...

        [member1@sales_queue]
        interface = Agent/612
        penalty = 2

        [member2@sales_queue]
        interface = SIP/phone1

        [member_foo@sales_queue]
        interface = IAX2/darkwingduck

        I'm not sure really that the "member names" here would have any real value, although I can envision some use for them in the future.

        This sort of configuration would work nicely with realtime as it never duplicates a variable name within a context, but it's a bit more wordy for non-realtime configs.

        Thoughts?

        Show
        Kevin P. Fleming (Inactive) added a comment - Except that doesn't work very well for multiple instances of the same variable name in a context, as is needed in queues.conf for "member =>" support. Here's anohter thought, although it's more radical (but along the lines of the current discussion about redesigning voicemail.conf). What about something like this: [general] ... [sales_queue] strategy = ... [member1@sales_queue] interface = Agent/612 penalty = 2 [member2@sales_queue] interface = SIP/phone1 [member_foo@sales_queue] interface = IAX2/darkwingduck I'm not sure really that the "member names" here would have any real value, although I can envision some use for them in the future. This sort of configuration would work nicely with realtime as it never duplicates a variable name within a context, but it's a bit more wordy for non-realtime configs. Thoughts?
        Hide
        knielsen added a comment -

        Kevin, what you suggest would not work for dynamic realtime (as was the point
        of this patch), as far as I can see.

        For dynamic realtime, we need a way for the database to quickly look up the
        members of a queue, as we will be doing that each time a caller enters the
        queue. With the [member@queue] syntax we would need to scan the whole config
        every time.

        What you suggest would probably work fine for static config (realtime or not),
        but that issue is not affected by the patch. It just looks that way because
        I moved some of the static config code into queue_set_param() so the code could
        be shared.

        I think for dynamic realtime we should stick with the two-table approach.

        Show
        knielsen added a comment - Kevin, what you suggest would not work for dynamic realtime (as was the point of this patch), as far as I can see. For dynamic realtime, we need a way for the database to quickly look up the members of a queue, as we will be doing that each time a caller enters the queue. With the [member@queue] syntax we would need to scan the whole config every time. What you suggest would probably work fine for static config (realtime or not), but that issue is not affected by the patch. It just looks that way because I moved some of the static config code into queue_set_param() so the code could be shared. I think for dynamic realtime we should stick with the two-table approach.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        Hmm, I see what you mean. I think I agree then, the two-table approach is the way to go, but it's possible we need to take what you create and make a new API for realtime that can collapse a two-table lookup into an ast_config object.

        Show
        Kevin P. Fleming (Inactive) added a comment - Hmm, I see what you mean. I think I agree then, the two-table approach is the way to go, but it's possible we need to take what you create and make a new API for realtime that can collapse a two-table lookup into an ast_config object.
        Hide
        knielsen added a comment -

        Ok, here is a new patch, updated to be against the newest CVS as of
        today.

        Note that this patch replaces the previous patch. Only the patch
        `rt-queue-patch2-dist.txt' needs to be applied for this feature.

        This patch uses the two-table approach discussed above. Suitable MySQL
        table definitions are the following:

        CREATE TABLE queue_table (
        name VARCHAR(128) PRIMARY KEY,
        musiconhold VARCHAR(128),
        announce VARCHAR(128),
        context VARCHAR(128),
        timeout INT(11),
        monitor_join BOOL,
        monitor_format VARCHAR(128),
        queue_youarenext VARCHAR(128),
        queue_thereare VARCHAR(128),
        queue_callswaiting VARCHAR(128),
        queue_holdtime VARCHAR(128),
        queue_minutes VARCHAR(128),
        queue_seconds VARCHAR(128),
        queue_lessthan VARCHAR(128),
        queue_thankyou VARCHAR(128),
        queue_reporthold VARCHAR(128),
        announce_frequency INT(11),
        announce_round_seconds INT(11),
        announce_holdtime VARCHAR(128),
        retry INT(11),
        wrapuptime INT(11),
        maxlen INT(11),
        servicelevel INT(11),
        strategy VARCHAR(128),
        joinempty VARCHAR(128),
        leavewhenempty VARCHAR(128),
        eventmemberstatus BOOL,
        eventwhencalled BOOL,
        reportholdtime BOOL,
        memberdelay INT(11),
        weight INT(11),
        timeoutrestart BOOL
        );

        CREATE TABLE queue_member_table (
        queue_name varchar(128),
        interface varchar(128),
        penalty INT(11),
        PRIMARY KEY (queue_name, interface)
        );

        The columns in the queue_table use the same names as parameters in the
        config file (except that I allow the use of underscore `_' instead of
        dash `-' to make life easier with SQL). All columns are optional with
        the exception of the `name' column (in the queue table) and `queue_name'
        and 'interface' (in the member table).

        The extconfig.conf needs two families, for example

        queues => mysql,asterisk,queue_table
        queue_members => mysql,asterisk,queue_member_table

        and the queues.conf needs to reference the two families:

        realtime_family = queues,queue_members

        Show
        knielsen added a comment - Ok, here is a new patch, updated to be against the newest CVS as of today. Note that this patch replaces the previous patch. Only the patch `rt-queue-patch2-dist.txt' needs to be applied for this feature. This patch uses the two-table approach discussed above. Suitable MySQL table definitions are the following: CREATE TABLE queue_table ( name VARCHAR(128) PRIMARY KEY, musiconhold VARCHAR(128), announce VARCHAR(128), context VARCHAR(128), timeout INT(11), monitor_join BOOL, monitor_format VARCHAR(128), queue_youarenext VARCHAR(128), queue_thereare VARCHAR(128), queue_callswaiting VARCHAR(128), queue_holdtime VARCHAR(128), queue_minutes VARCHAR(128), queue_seconds VARCHAR(128), queue_lessthan VARCHAR(128), queue_thankyou VARCHAR(128), queue_reporthold VARCHAR(128), announce_frequency INT(11), announce_round_seconds INT(11), announce_holdtime VARCHAR(128), retry INT(11), wrapuptime INT(11), maxlen INT(11), servicelevel INT(11), strategy VARCHAR(128), joinempty VARCHAR(128), leavewhenempty VARCHAR(128), eventmemberstatus BOOL, eventwhencalled BOOL, reportholdtime BOOL, memberdelay INT(11), weight INT(11), timeoutrestart BOOL ); CREATE TABLE queue_member_table ( queue_name varchar(128), interface varchar(128), penalty INT(11), PRIMARY KEY (queue_name, interface) ); The columns in the queue_table use the same names as parameters in the config file (except that I allow the use of underscore `_' instead of dash `-' to make life easier with SQL). All columns are optional with the exception of the `name' column (in the queue table) and `queue_name' and 'interface' (in the member table). The extconfig.conf needs two families, for example queues => mysql,asterisk,queue_table queue_members => mysql,asterisk,queue_member_table and the queues.conf needs to reference the two families: realtime_family = queues,queue_members
        Hide
        ckruetze added a comment -

        Just an idea, but what do you think of splitting it into a three table design?

        CREATE TABLE queue_table (
        name VARCHAR(128) PRIMARY KEY,
        );

        CREATE TABLE queue_option (
        queue_name VARCHAR(128),
        option_name VARCHAR(128),
        option_value VARCHAR(128),
        PRIMARY KEY (queue_name, option_name)
        );

        In the two table design, I can see two problems. a) we have fields in the table that might never get used and b) if somebody comes up with a new feature, the table needs to be changed and backwards compatibility might be a problem.

        The only problem I see might be that not all queue options are varchar(128) and that this would generate a slight overhead, but not as much as having lots of never used fields in the table.

        If a good database design is wanted, then it might even be useful to split the queue_option into queue_option_varchar and queue_option_int and maybe queue_option_bool.
        maybe queue_option_bool.

        Just my idea as I'm more a database programmer then an * programmer.

        Show
        ckruetze added a comment - Just an idea, but what do you think of splitting it into a three table design? CREATE TABLE queue_table ( name VARCHAR(128) PRIMARY KEY, ); CREATE TABLE queue_option ( queue_name VARCHAR(128), option_name VARCHAR(128), option_value VARCHAR(128), PRIMARY KEY (queue_name, option_name) ); In the two table design, I can see two problems. a) we have fields in the table that might never get used and b) if somebody comes up with a new feature, the table needs to be changed and backwards compatibility might be a problem. The only problem I see might be that not all queue options are varchar(128) and that this would generate a slight overhead, but not as much as having lots of never used fields in the table. If a good database design is wanted, then it might even be useful to split the queue_option into queue_option_varchar and queue_option_int and maybe queue_option_bool. maybe queue_option_bool. Just my idea as I'm more a database programmer then an * programmer.
        Hide
        Brian West added a comment -

        Realtime already supports doing this:

        say you have members: SIP/12;SIP/13;Zap/1;Zap/g1/5551212;Agent/1

        It would parse that correctly and return the object filled out just like if you put them all on single lines.

        /b

        Show
        Brian West added a comment - Realtime already supports doing this: say you have members: SIP/12;SIP/13;Zap/1;Zap/g1/5551212;Agent/1 It would parse that correctly and return the object filled out just like if you put them all on single lines. /b
        Hide
        Brian West added a comment -

        Also with any modern sql server you can do this with a view thus not having to hand parse the crap with your backend scripts.

        /b

        Show
        Brian West added a comment - Also with any modern sql server you can do this with a view thus not having to hand parse the crap with your backend scripts. /b
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        bkw: How do you propose handling the per-member penalty setting with them combined into a single string?

        The three-table approach is a non-starter, because it uses a different method of converting database entries into Realtime config objects and I see no reason to introduce that at this point. If you are concerned about space consumption, do the following:

        1) Allow NULLs in your columns.
        2) Don't use any fixed-length string columns.

        With those two changes to the table schema, having a bunch of extra columns carries nearly no cost at all. Adding new columns in the future is also a piece of cake, and is already the way that Realtime works.

        Code notes from a quick review:

        • Can you confirm that the compiler will optimize out a call to strncpy when the source string is a constant of length shorter than the variable? If it won't, don't use strncpy to initialize all the queue_* sound file names, since we know the constant strings will not be too long for the variables in question.
        • Minor typos: s/queue.conf/queues.conf/, s/queue-rounding-seconds/announce-round-seconds/ (actually, error messages about parameter values should just use the already-found parameter name, which will ease future changes/copying of the code)
        • Don't compare pointers to NULL (m == NULL), just use !m instead
        • In reload_queue_rt, you will call ast_strdupa every time you find a parameter with an '_' in it. This will eat up stack space very quickly, as they are not freed until the function returns. Just allocate a buffer on the stack, and keep reusing it as you need it.
        • You can avoid the 'first_field' stuff when looping through member variables by using ast_load_realtime_multientry; look at app_directory for an example.
        • I don't see a call to ast_variables_destroy for member_vars.
        • While you are in there, please change join_queue to use something like:

        if (!q)

        { ast_mutex_unlock(&qlock); return res; }

        This way all the other code doesn't need to be indented inside a block, when there are only two lines outside of it.

        • Is there some value in having the 'realtime_family' setting in queues.conf? I think it's safe to always call them 'queues' and 'queue_members', and always make the calls into ast_realtime. If the user hasn't configured them in extconfig.conf, they will be no-ops.

        Otherwise, let me say this loudly: This is a darn fine piece of work! Major kudos to you for your first patch being this complex and of such high quality.

        Show
        Kevin P. Fleming (Inactive) added a comment - bkw: How do you propose handling the per-member penalty setting with them combined into a single string? The three-table approach is a non-starter, because it uses a different method of converting database entries into Realtime config objects and I see no reason to introduce that at this point. If you are concerned about space consumption, do the following: 1) Allow NULLs in your columns. 2) Don't use any fixed-length string columns. With those two changes to the table schema, having a bunch of extra columns carries nearly no cost at all. Adding new columns in the future is also a piece of cake, and is already the way that Realtime works. Code notes from a quick review: Can you confirm that the compiler will optimize out a call to strncpy when the source string is a constant of length shorter than the variable? If it won't, don't use strncpy to initialize all the queue_* sound file names, since we know the constant strings will not be too long for the variables in question. Minor typos: s/queue.conf/queues.conf/, s/queue-rounding-seconds/announce-round-seconds/ (actually, error messages about parameter values should just use the already-found parameter name, which will ease future changes/copying of the code) Don't compare pointers to NULL (m == NULL), just use !m instead In reload_queue_rt, you will call ast_strdupa every time you find a parameter with an '_' in it. This will eat up stack space very quickly, as they are not freed until the function returns. Just allocate a buffer on the stack, and keep reusing it as you need it. You can avoid the 'first_field' stuff when looping through member variables by using ast_load_realtime_multientry; look at app_directory for an example. I don't see a call to ast_variables_destroy for member_vars. While you are in there, please change join_queue to use something like: if (!q) { ast_mutex_unlock(&qlock); return res; } This way all the other code doesn't need to be indented inside a block, when there are only two lines outside of it. Is there some value in having the 'realtime_family' setting in queues.conf? I think it's safe to always call them 'queues' and 'queue_members', and always make the calls into ast_realtime. If the user hasn't configured them in extconfig.conf, they will be no-ops. Otherwise, let me say this loudly : This is a darn fine piece of work! Major kudos to you for your first patch being this complex and of such high quality.
        Hide
        knielsen added a comment -

        Kevin, thanks for your comments, well spotted for the two memory issues!

        And thanks for the kind words .

        I implemented all of your suggestions in the new patch
        `rt-queue-patch3-dist.txt'. This is against the newest CVS as of today,
        and replaces the previous two patches (so I suggest you delete patch2
        just as you deleted patch1 earlier).

        Anything else before this could go in CVS?

        Show
        knielsen added a comment - Kevin, thanks for your comments, well spotted for the two memory issues! And thanks for the kind words . I implemented all of your suggestions in the new patch `rt-queue-patch3-dist.txt'. This is against the newest CVS as of today, and replaces the previous two patches (so I suggest you delete patch2 just as you deleted patch1 earlier). Anything else before this could go in CVS?
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        I think is ready to go... it applies and compiles cleanly, and appears that it would work properly (I don't have a database configured here, but the code is straightforward in that regard).

        I've marked this 'confirmed' so it's queued for Mark to approve.

        Show
        Kevin P. Fleming (Inactive) added a comment - I think is ready to go... it applies and compiles cleanly, and appears that it would work properly (I don't have a database configured here, but the code is straightforward in that regard). I've marked this 'confirmed' so it's queued for Mark to approve.
        Hide
        Brian West added a comment -

        Never mind.. I see it.. two tables is more sane than the var/val stuff... and does fit more in line with realtime ...

        /b

        edited on: 04-27-05 13:25

        edited on: 04-27-05 13:27

        Show
        Brian West added a comment - Never mind.. I see it.. two tables is more sane than the var/val stuff... and does fit more in line with realtime ... /b edited on: 04-27-05 13:25 edited on: 04-27-05 13:27
        Hide
        Kaj J. Niemi added a comment -

        When fetching rows from queue members, is the first part of the select statement ("... interface LIKE '%' AND ...") necessary? The table definition states that the interface column cannot be NULL. I'm just curious

        Apr 27 22:44:08 DEBUG[9551] res_config_mysql.c: MySQL RealTime: Retrieve SQL: SELECT * FROM queues WHERE name = 'my-test'
        Apr 27 22:44:08 DEBUG[9551] res_config_mysql.c: MySQL RealTime: Everything is fine.
        Apr 27 22:44:08 DEBUG[9551] res_config_mysql.c: MySQL RealTime: Retrieve SQL: SELECT * FROM queue_members WHERE interface LIKE '%' AND queue_name = 'my-test'
        ORDER BY interface
        Apr 27 22:44:08 DEBUG[9551] res_config_mysql.c: MySQL RealTime: Everything is fine.

        Show
        Kaj J. Niemi added a comment - When fetching rows from queue members, is the first part of the select statement ("... interface LIKE '%' AND ...") necessary? The table definition states that the interface column cannot be NULL. I'm just curious Apr 27 22:44:08 DEBUG [9551] res_config_mysql.c: MySQL RealTime: Retrieve SQL: SELECT * FROM queues WHERE name = 'my-test' Apr 27 22:44:08 DEBUG [9551] res_config_mysql.c: MySQL RealTime: Everything is fine. Apr 27 22:44:08 DEBUG [9551] res_config_mysql.c: MySQL RealTime: Retrieve SQL: SELECT * FROM queue_members WHERE interface LIKE '%' AND queue_name = 'my-test' ORDER BY interface Apr 27 22:44:08 DEBUG [9551] res_config_mysql.c: MySQL RealTime: Everything is fine.
        Hide
        knielsen added a comment -

        Yes, the "... interface LIKE '%' AND ..." is pretty wierd, and I do not
        like it. But it is necessary because of the way realtime is designed.

        Kevin asked me to use the ast_load_realtime_multientry() function to
        fetch members, which makes a lot of sense since we are fetching multiple
        rows. So we need to tell that function to fetch all rows that match a
        given key `queue_name', and return the rows indexed by the column
        `interface'.

        Unfortunately the API for that function is seriously broken, IMHO, in
        that you cannot explicitly specify the column `interface' on which you
        want to index the rows in the returned ast_config structure. Instead the
        index column is implicitly set from the first join condition passed to
        the function. So I have to put in an initial no-op join condition on the
        `interface' column. I choose "interface LIKE '%'", another option would
        have been "interface != ''". Look at app_directory.c, it does the same
        thing:

        rtdata = ast_load_realtime_multientry("voicemail", "mailbox LIKE", "%", "context", context, NULL);

        I am quite tempted to go in and fix this (maybe just give
        realtime_multi_mysql() and friends an extra argument specifying the
        column to index on and appropriate changes in config.c). While there I
        would also want to fix the "SQL injection" vulnerability that exists
        currently because of the lack of any use of proper SQL quoting or bind
        variable usage.

        It's just that I want my first appearance in the Asterisk development
        community to be a humble "here is a nice little feature, feel free to
        use it in Asterisk", not a crashing "you framework is badly broken, you
        need to change it like this now" .

        In any case, this is a separate issue, and if there is a consensus that
        ast_load_realtime_multientry() should be changed I will open a new bug
        for it.

        Show
        knielsen added a comment - Yes, the "... interface LIKE '%' AND ..." is pretty wierd, and I do not like it. But it is necessary because of the way realtime is designed. Kevin asked me to use the ast_load_realtime_multientry() function to fetch members, which makes a lot of sense since we are fetching multiple rows. So we need to tell that function to fetch all rows that match a given key `queue_name', and return the rows indexed by the column `interface'. Unfortunately the API for that function is seriously broken, IMHO, in that you cannot explicitly specify the column `interface' on which you want to index the rows in the returned ast_config structure. Instead the index column is implicitly set from the first join condition passed to the function. So I have to put in an initial no-op join condition on the `interface' column. I choose "interface LIKE '%'", another option would have been "interface != ''". Look at app_directory.c, it does the same thing: rtdata = ast_load_realtime_multientry("voicemail", "mailbox LIKE", "%", "context", context, NULL); I am quite tempted to go in and fix this (maybe just give realtime_multi_mysql() and friends an extra argument specifying the column to index on and appropriate changes in config.c). While there I would also want to fix the "SQL injection" vulnerability that exists currently because of the lack of any use of proper SQL quoting or bind variable usage. It's just that I want my first appearance in the Asterisk development community to be a humble "here is a nice little feature, feel free to use it in Asterisk", not a crashing "you framework is badly broken, you need to change it like this now " . In any case, this is a separate issue, and if there is a consensus that ast_load_realtime_multientry() should be changed I will open a new bug for it.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        This analysis is all spot-on; there are some definite design deficiencies in that API. However, fixing them is something that will need to happen in a separate bug, after this one has been merged.

        Show
        Kevin P. Fleming (Inactive) added a comment - This analysis is all spot-on; there are some definite design deficiencies in that API. However, fixing them is something that will need to happen in a separate bug, after this one has been merged.
        Hide
        knielsen added a comment -

        Any progress with this?

        Show
        knielsen added a comment - Any progress with this?
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        Mark: I think this is ready to go, but need your approval since it's a significant addition.

        Show
        Kevin P. Fleming (Inactive) added a comment - Mark: I think this is ready to go, but need your approval since it's a significant addition.
        Hide
        Mark Spencer added a comment -

        I definitely like this patch. I'd like some other people to sign off on the functionality test, code review and security audit, but conceptually I'm totally on board with this architecture and am ready to see it merged pending those reviews.

        Show
        Mark Spencer added a comment - I definitely like this patch. I'd like some other people to sign off on the functionality test, code review and security audit, but conceptually I'm totally on board with this architecture and am ready to see it merged pending those reviews.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        The patch from bug 3821 has made this one no longer apply cleanly, so it will need updating. While that's being done, you can replace the strncpy() calls with ast_copy_string() calls instead

        When the new patch is posted I'm ready to sign off on the code/security/formatting reviews, as long as there are no regressions from the current patch. I've posted a request to the asterisk-dev list for testers so we can get some additional functionality testing done as well.

        Show
        Kevin P. Fleming (Inactive) added a comment - The patch from bug 3821 has made this one no longer apply cleanly, so it will need updating. While that's being done, you can replace the strncpy() calls with ast_copy_string() calls instead When the new patch is posted I'm ready to sign off on the code/security/formatting reviews, as long as there are no regressions from the current patch. I've posted a request to the asterisk-dev list for testers so we can get some additional functionality testing done as well.
        Hide
        drmac added a comment -

        I will test this patch in a production environment tomorrow.

        From what I've read about this patch, this patch is step 1 of 3 to making asterisk completly load balancable across multiple servers sharing 1 database. app_queue has been keeping me from starting up a server farm load balanced by SER (or some other layer 7 switch) because its 'not' db enabled. kudos to nielsen for this.

        Show
        drmac added a comment - I will test this patch in a production environment tomorrow. From what I've read about this patch, this patch is step 1 of 3 to making asterisk completly load balancable across multiple servers sharing 1 database. app_queue has been keeping me from starting up a server farm load balanced by SER (or some other layer 7 switch) because its 'not' db enabled. kudos to nielsen for this.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        I already reported that in the bugnote above yours; did you review them?

        Show
        Kevin P. Fleming (Inactive) added a comment - I already reported that in the bugnote above yours; did you review them?
        Hide
        drmac added a comment -

        crap...(post deleted)

        Show
        drmac added a comment - crap...(post deleted)
        Hide
        outtolunc added a comment -

        uploaded http://www.dynx.net/ASTERISK/diff-patches/app_queue.c.diff.txt
        it has not been tested but it compiles cleanly. it was a 'by-hand' edit of the rt-queue-patch3-dist.txt against current cvs-head as of noon 05-16-2005 (PST)

        disclaimer on file (though this is not my code <G>)

        Show
        outtolunc added a comment - uploaded http://www.dynx.net/ASTERISK/diff-patches/app_queue.c.diff.txt it has not been tested but it compiles cleanly. it was a 'by-hand' edit of the rt-queue-patch3-dist.txt against current cvs-head as of noon 05-16-2005 (PST) disclaimer on file (though this is not my code <G>)
        Hide
        drmac added a comment -

        patch applied successfully.

        I just tried to add myself to a queue listed in database using AddQueueMember.
        Debug log shows no Realtime activity; therefor I get "queue not found" error.

        (does some investigating..)

        ok. i think i see. you can't do login/logoff of agents. all agents are static. hmm..but we can change that easily enough by adding an "active" column to the queue_memebers table. thus when an agent logs in/logs off, the "active" column is ast_realtime_update'd.

        ?? ideas on that ??

        here is my vision of this patch:
        X number of asterisk servers all sharing a common database. company ABC's queue is stored in database. agents for ABC login/logoff throughout the day.
        all X asterisk servers are load balanced. any incomming call could goto any server.
        agent 1 logs in at server A. database updated. agent 2 at B, agent 3 at C.
        person calls in and gets added to queue on server B. server B says "which agents are available?" query db. "oh. agent 1 is available. contact/bridge this call to him."
        server B knows how to contact agent 1 because agent 1 registered using realtime SIP; so we can create a full contact header.

        ?? thoughts on that ??

        Show
        drmac added a comment - patch applied successfully. I just tried to add myself to a queue listed in database using AddQueueMember. Debug log shows no Realtime activity; therefor I get "queue not found" error. (does some investigating..) ok. i think i see. you can't do login/logoff of agents. all agents are static. hmm..but we can change that easily enough by adding an "active" column to the queue_memebers table. thus when an agent logs in/logs off, the "active" column is ast_realtime_update'd. ?? ideas on that ?? here is my vision of this patch: X number of asterisk servers all sharing a common database. company ABC's queue is stored in database. agents for ABC login/logoff throughout the day. all X asterisk servers are load balanced. any incomming call could goto any server. agent 1 logs in at server A. database updated. agent 2 at B, agent 3 at C. person calls in and gets added to queue on server B. server B says "which agents are available?" query db. "oh. agent 1 is available. contact/bridge this call to him." server B knows how to contact agent 1 because agent 1 registered using realtime SIP; so we can create a full contact header. ?? thoughts on that ??
        Hide
        knielsen added a comment -

        I had some issues with outtolunc's app_queue.c.diff.txt patch, so I merged it
        into current CVS myself and will upload as rt-queue-patch4-dist.txt.

        The only difference between this and the previous patch is the change of
        strncpy() to ast_copy_string(). So there were acually no conflicts with
        bug 3821, only with the ast_copy_string() updates

        Show
        knielsen added a comment - I had some issues with outtolunc's app_queue.c.diff.txt patch, so I merged it into current CVS myself and will upload as rt-queue-patch4-dist.txt. The only difference between this and the previous patch is the change of strncpy() to ast_copy_string(). So there were acually no conflicts with bug 3821, only with the ast_copy_string() updates
        Hide
        Terry Wilson added a comment -

        Works fine for me so far (using postres w/ odbc)

        Show
        Terry Wilson added a comment - Works fine for me so far (using postres w/ odbc)
        Hide
        knielsen added a comment -

        Just a quick comment about AddQueueMember. The AddQueueMember stuff uses the embedded Berkley DB in Asterisk, not realtime. This patch doesn't try to change that, and frankly I think that should be kept as a separate issue. Once this patch is in, a new bug can be opened on getting the stuff working that the comment from drmac mentions.

        Show
        knielsen added a comment - Just a quick comment about AddQueueMember. The AddQueueMember stuff uses the embedded Berkley DB in Asterisk, not realtime. This patch doesn't try to change that, and frankly I think that should be kept as a separate issue. Once this patch is in, a new bug can be opened on getting the stuff working that the comment from drmac mentions.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        As I understand it, there are three types of queue members supported by app_queue:

        • static members
        • dynamic members (AddQueueMember)
        • agents (static members of the type Agent/<name> handled by chan_agent)

        Static members do not allow any sort of login/logout functionality. Dynamic members do not either, but they can be added/removed as needed. Agents do support login/logout, of two different varieties (callback and normal).

        This patch only addresses static members and agents, and does not affect the login/logout behavior of the agents since that is handled by chan_agent.

        In spite of that, I think we do need to allow for AddQueueMember to be called against a queue that is loaded via Realtime, since we support it for queues from queues.conf. I agree that there is no need to be able to persist them to the Realtime database at this time, they can continue to be persisted to the astdb as they are now. A future patch could extend that behavior, of course, as already commented.

        Comments? Do we need to hold this patch until Realtime queues can receive dynamic members, or merge it as it stands and hope that functionality comes along soon

        Show
        Kevin P. Fleming (Inactive) added a comment - As I understand it, there are three types of queue members supported by app_queue: static members dynamic members (AddQueueMember) agents (static members of the type Agent/<name> handled by chan_agent) Static members do not allow any sort of login/logout functionality. Dynamic members do not either, but they can be added/removed as needed. Agents do support login/logout, of two different varieties (callback and normal). This patch only addresses static members and agents, and does not affect the login/logout behavior of the agents since that is handled by chan_agent. In spite of that, I think we do need to allow for AddQueueMember to be called against a queue that is loaded via Realtime, since we support it for queues from queues.conf. I agree that there is no need to be able to persist them to the Realtime database at this time, they can continue to be persisted to the astdb as they are now. A future patch could extend that behavior, of course, as already commented. Comments? Do we need to hold this patch until Realtime queues can receive dynamic members, or merge it as it stands and hope that functionality comes along soon
        Hide
        knielsen added a comment -

        Ok, I looked deeper into the AddQueueMember issue (never used that myself).

        For a realtime queue, AddQueueMember works the same as an INSERT into the member table, and a RemoveQueueMember works the same as a DELETE from the member table.

        So to properly implement AddQueueMember/RemoveQueueMember is simple enough. In add_to_queue()/remove_from_queue(), use reload_queue_rt() to load the queue. If the queue is static do the stuff in the current code. If the queue is realtime, instead issue the proper INSERT or DELETE statement to the database (and maybe update the in-memory structure afterwards to get the change to take effect also for callers currently sitting in the queue). That should be sufficient.

        Only problem: the realtime framework does not support INSERT or DELETE (at least not the res_config_mysql.c one)

        Kevin, I think you had in mind that the AddQueueMember stuff would load the queue from realtime and then add extra dynamic members to the in-core linked lists for the queue. But that would create a synchronisation nightmare between realtime members and dynamic members in the same queue, and I cannot really see anyone wanting this functionality (certainly it's not what drmac needs). And if noone wants it, who would implement it?

        drmac, you wrote the res_mysql_config.c, would you be willing to add INSERT/DELETE to that driver (and to config.c) and to test a realtime-persistent AddQueueMember as outlined above?

        BTW, I will not be able to do any work on this patch in June or July.

        Show
        knielsen added a comment - Ok, I looked deeper into the AddQueueMember issue (never used that myself). For a realtime queue, AddQueueMember works the same as an INSERT into the member table, and a RemoveQueueMember works the same as a DELETE from the member table. So to properly implement AddQueueMember/RemoveQueueMember is simple enough. In add_to_queue()/remove_from_queue(), use reload_queue_rt() to load the queue. If the queue is static do the stuff in the current code. If the queue is realtime, instead issue the proper INSERT or DELETE statement to the database (and maybe update the in-memory structure afterwards to get the change to take effect also for callers currently sitting in the queue). That should be sufficient. Only problem: the realtime framework does not support INSERT or DELETE (at least not the res_config_mysql.c one) Kevin, I think you had in mind that the AddQueueMember stuff would load the queue from realtime and then add extra dynamic members to the in-core linked lists for the queue. But that would create a synchronisation nightmare between realtime members and dynamic members in the same queue, and I cannot really see anyone wanting this functionality (certainly it's not what drmac needs). And if noone wants it, who would implement it? drmac, you wrote the res_mysql_config.c, would you be willing to add INSERT/DELETE to that driver (and to config.c) and to test a realtime-persistent AddQueueMember as outlined above? BTW, I will not be able to do any work on this patch in June or July.
        Hide
        Terry Wilson added a comment -

        Well, as long as we are coming up with a way that we think it should be...

        Lets look at queues and queue members from the point of designing an interface for them (since they are realtime, a gui is a prime example of a good use for them). It would be nice to be able to assign members to a queue, and have them be able to log in and out. If we do it the current way for realtime that is currently done w/ AddQueueMember/RemoveQueueMember then we would be physically deleting the members so there would be no record of having them haaving existed after they logged out. Having a flag for active/not active would make it easy to define the members via realtime, and the flip the flag for loggged in or not. So as far as asterisk is concerned, the query could contain WHERE active = 1 for it to recognize the member, but you could still have them defined for the interface.

        Personally I use res_perl so I can add my own flag and own add_queue_member remove_queue_member functions that do this (as long as I can add the WHERE active = 1 clause to the SQL in the realtime queue code)... but just thought I'd give my two cents.

        Admittedly this is probably outside the scope of the original patch. I don't think that we have to wait to get the dynamic stuff in to get this submitted. We can discuss all of those considerations in a future bug.

        Show
        Terry Wilson added a comment - Well, as long as we are coming up with a way that we think it should be... Lets look at queues and queue members from the point of designing an interface for them (since they are realtime, a gui is a prime example of a good use for them). It would be nice to be able to assign members to a queue, and have them be able to log in and out. If we do it the current way for realtime that is currently done w/ AddQueueMember/RemoveQueueMember then we would be physically deleting the members so there would be no record of having them haaving existed after they logged out. Having a flag for active/not active would make it easy to define the members via realtime, and the flip the flag for loggged in or not. So as far as asterisk is concerned, the query could contain WHERE active = 1 for it to recognize the member, but you could still have them defined for the interface. Personally I use res_perl so I can add my own flag and own add_queue_member remove_queue_member functions that do this (as long as I can add the WHERE active = 1 clause to the SQL in the realtime queue code)... but just thought I'd give my two cents. Admittedly this is probably outside the scope of the original patch. I don't think that we have to wait to get the dynamic stuff in to get this submitted. We can discuss all of those considerations in a future bug.
        Hide
        outtolunc added a comment -

        i'm not sure if this is the right time or place to mention this. but as long as you guys are heading this direction.

        if you look at the relationships..

        dbget / dbput
        database get / database put
        realtime load / update

        what is wrong with that picture.. we need a a realtime put (insert)

        just my $.02

        Show
        outtolunc added a comment - i'm not sure if this is the right time or place to mention this. but as long as you guys are heading this direction. if you look at the relationships.. dbget / dbput database get / database put realtime load / update what is wrong with that picture.. we need a a realtime put (insert) just my $.02
        Hide
        drmac added a comment -

        Re: post ASTERISK-2718651

        I have no problem implementing a ast_realtime_insert and ast_realtime_delete in res_config_mysql.c. I actually have both of them already coded for my personal use. The reason I never added them publicly was because res_config_odbc did not have them.

        If we don't want to do insert/delete, we can always add another column to the queuemember table called "active" and just use ast_realtime_update to change that column from 1 to 0 and vice versa depending on Add/Remove.

        The caveat with that is you have to pre-load all members into the table which isn't the current behavior.

        Show
        drmac added a comment - Re: post ASTERISK-2718651 I have no problem implementing a ast_realtime_insert and ast_realtime_delete in res_config_mysql.c. I actually have both of them already coded for my personal use. The reason I never added them publicly was because res_config_odbc did not have them. If we don't want to do insert/delete, we can always add another column to the queuemember table called "active" and just use ast_realtime_update to change that column from 1 to 0 and vice versa depending on Add/Remove. The caveat with that is you have to pre-load all members into the table which isn't the current behavior.
        Hide
        Michael Jerris added a comment -

        markster, can we have some comment on realtime insert\delete?

        Show
        Michael Jerris added a comment - markster, can we have some comment on realtime insert\delete?
        Hide
        Michael Jerris added a comment -

        Also lets get this in and move implementing realtime updates and delete to another bug.

        Show
        Michael Jerris added a comment - Also lets get this in and move implementing realtime updates and delete to another bug.
        Hide
        knielsen added a comment -

        I think we should finish this issue one way or the other, add to CVS or not, but let's finish it.

        I looked once more into the AddQueueMember stuff, and I still think the only sane way is to map them to realtime INSERT/DELETE, once that is added to realtime. The comment from outtolunc seems to agree.

        Or perhaps use drmac's realtime UPDATE proposal, but that also requires realtime additions: the current realtime API does not allow to update on a composite key, and realtime queue need the key (queue, member).

        Both twilson and MikeJ seemed to think this should be added to CVS and the dynamic stuff moved to another bug.

        Or if you prefer, I will just have to maintain this locally for our own purpose.

        Show
        knielsen added a comment - I think we should finish this issue one way or the other, add to CVS or not, but let's finish it. I looked once more into the AddQueueMember stuff, and I still think the only sane way is to map them to realtime INSERT/DELETE, once that is added to realtime. The comment from outtolunc seems to agree. Or perhaps use drmac's realtime UPDATE proposal, but that also requires realtime additions: the current realtime API does not allow to update on a composite key, and realtime queue need the key (queue, member). Both twilson and MikeJ seemed to think this should be added to CVS and the dynamic stuff moved to another bug. Or if you prefer, I will just have to maintain this locally for our own purpose.
        Hide
        Kevin P. Fleming (Inactive) added a comment -

        Committed to CVS HEAD, thanks!

        Show
        Kevin P. Fleming (Inactive) added a comment - Committed to CVS HEAD, thanks!
        Hide
        Digium Subversion added a comment -

        Repository: asterisk
        Revision: 5821

        U trunk/apps/app_queue.c
        U trunk/configs/extconfig.conf.sample

        ------------------------------------------------------------------------
        r5821 | kpfleming | 2008-01-15 15:36:50 -0600 (Tue, 15 Jan 2008) | 2 lines

        add realtime support to app_queue for static members/agents (bug ASTERISK-3943)

        ------------------------------------------------------------------------

        http://svn.digium.com/view/asterisk?view=rev&revision=5821

        Show
        Digium Subversion added a comment - Repository: asterisk Revision: 5821 U trunk/apps/app_queue.c U trunk/configs/extconfig.conf.sample ------------------------------------------------------------------------ r5821 | kpfleming | 2008-01-15 15:36:50 -0600 (Tue, 15 Jan 2008) | 2 lines add realtime support to app_queue for static members/agents (bug ASTERISK-3943 ) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5821
        Erin Spiceland made changes -
        Field Original Value New Value
        issue.field.mantisimportkey 4037 27711
        Erin Spiceland made changes -
        Workflow jira [ 69531 ] Subtask and Courtesy Workflow [ 108299 ]
        Erin Spiceland made changes -
        Assignee Kevin P. Fleming [ kpfleming ]

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development