[ovs-dev,mointor2,8/9] lib: add monitor2 support in ovsdb-idl.
diff mbox

Message ID 1445489131-21483-8-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Oct. 22, 2015, 4:45 a.m. UTC
Add support for monitor2. When idl starts to run, monitor2 will be
attempted first. In case the server is an older version that does
not recognize monitor2.  IDL will then fall back to use "monitor"
method.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 lib/ovsdb-idl.c    | 319 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 tests/ovsdb-idl.at |  10 +-
 2 files changed, 306 insertions(+), 23 deletions(-)

Comments

Ben Pfaff Nov. 10, 2015, 5:45 p.m. UTC | #1
On Wed, Oct 21, 2015 at 09:45:30PM -0700, Andy Zhou wrote:
> Add support for monitor2. When idl starts to run, monitor2 will be
> attempted first. In case the server is an older version that does
> not recognize monitor2.  IDL will then fall back to use "monitor"
> method.
> 
> Signed-off-by: Andy Zhou <azhou@nicira.com>

This is pretty clean, thanks!

In the error case here, it could be worthwhile to check that the
msg->id matches idl->request_id:

        } else if (msg->type == JSONRPC_ERROR
                   && idl->state == IDL_S_MONITOR2_REQUESTED) {


At a glance, it looks like there's a lot of code duplication between the
new and the old update implementation.  Can any of it be cleanly
factored out?

This adds an extra blank line above ovsdb_idl_update_has_lock().

Thanks,

Ben.
Andy Zhou Nov. 24, 2015, 10:27 a.m. UTC | #2
On Tue, Nov 10, 2015 at 9:45 AM, Ben Pfaff <blp@ovn.org> wrote:
> On Wed, Oct 21, 2015 at 09:45:30PM -0700, Andy Zhou wrote:
>> Add support for monitor2. When idl starts to run, monitor2 will be
>> attempted first. In case the server is an older version that does
>> not recognize monitor2.  IDL will then fall back to use "monitor"
>> method.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>
> This is pretty clean, thanks!
>
> In the error case here, it could be worthwhile to check that the
> msg->id matches idl->request_id:
>
>         } else if (msg->type == JSONRPC_ERROR
>                    && idl->state == IDL_S_MONITOR2_REQUESTED) {
>
>
Thanks for the suggestion. Folded this into V2.

> At a glance, it looks like there's a lot of code duplication between the
> new and the old update implementation.  Can any of it be cleanly
> factored out?
>
O.K. I merged ovsdb_idl_parse_upate__() and ovsdb_idl_parse_update2__() into
a single function to reduce code duplication. Please let me know if v2
looks better.

> This adds an extra blank line above ovsdb_idl_update_has_lock().
Removed

Patch
diff mbox

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 00b900d..1a30ad4 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -78,7 +78,9 @@  struct ovsdb_idl_arc {
 enum ovsdb_idl_state {
     IDL_S_SCHEMA_REQUESTED,
     IDL_S_MONITOR_REQUESTED,
-    IDL_S_MONITORING
+    IDL_S_MONITORING,
+    IDL_S_MONITOR2_REQUESTED,
+    IDL_S_MONITORING2
 };
 
 struct ovsdb_idl {
@@ -93,6 +95,7 @@  struct ovsdb_idl {
     unsigned int state_seqno;
     enum ovsdb_idl_state state;
     struct json *request_id;
+    struct json *schema;
 
     /* Database locking. */
     char *lock_name;            /* Name of lock we need, NULL if none. */
@@ -138,18 +141,27 @@  static struct vlog_rate_limit semantic_rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
 static void ovsdb_idl_clear(struct ovsdb_idl *);
 static void ovsdb_idl_send_schema_request(struct ovsdb_idl *);
-static void ovsdb_idl_send_monitor_request(struct ovsdb_idl *,
-                                           const struct json *schema_json);
+static void ovsdb_idl_send_monitor_request(struct ovsdb_idl *);
+static void ovsdb_idl_send_monitor2_request(struct ovsdb_idl *);
 static void ovsdb_idl_parse_update(struct ovsdb_idl *, const struct json *);
+static void ovsdb_idl_parse_update2(struct ovsdb_idl *, const struct json *);
 static struct ovsdb_error *ovsdb_idl_parse_update__(struct ovsdb_idl *,
                                                     const struct json *);
+static struct ovsdb_error *ovsdb_idl_parse_update2__(struct ovsdb_idl *,
+                                                     const struct json *);
 static bool ovsdb_idl_process_update(struct ovsdb_idl_table *,
                                      const struct uuid *,
                                      const struct json *old,
                                      const struct json *new);
+static bool ovsdb_idl_process_update2(struct ovsdb_idl_table *,
+                                      const struct uuid *,
+                                      const char *operation,
+                                      const struct json *row);
 static void ovsdb_idl_insert_row(struct ovsdb_idl_row *, const struct json *);
 static void ovsdb_idl_delete_row(struct ovsdb_idl_row *);
 static bool ovsdb_idl_modify_row(struct ovsdb_idl_row *, const struct json *);
+static bool ovsdb_idl_modify_row_by_diff(struct ovsdb_idl_row *,
+                                         const struct json *);
 
 static bool ovsdb_idl_row_is_orphan(const struct ovsdb_idl_row *);
 static struct ovsdb_idl_row *ovsdb_idl_row_create__(
@@ -232,6 +244,7 @@  ovsdb_idl_create(const char *remote, const struct ovsdb_idl_class *class,
 
     idl->state_seqno = UINT_MAX;
     idl->request_id = NULL;
+    idl->schema = NULL;
 
     hmap_init(&idl->outstanding_txns);
 
@@ -260,6 +273,7 @@  ovsdb_idl_destroy(struct ovsdb_idl *idl)
         json_destroy(idl->request_id);
         free(idl->lock_name);
         json_destroy(idl->lock_request_id);
+        json_destroy(idl->schema);
         hmap_destroy(&idl->outstanding_txns);
         free(idl);
     }
@@ -335,12 +349,12 @@  ovsdb_idl_run(struct ovsdb_idl *idl)
         }
 
         if (msg->type == JSONRPC_NOTIFY
-            && !strcmp(msg->method, "update")
+            && !strcmp(msg->method, "update2")
             && msg->params->type == JSON_ARRAY
             && msg->params->u.array.n == 2
             && msg->params->u.array.elems[0]->type == JSON_NULL) {
             /* Database contents changed. */
-            ovsdb_idl_parse_update(idl, msg->params->u.array.elems[1]);
+            ovsdb_idl_parse_update2(idl, msg->params->u.array.elems[1]);
         } else if (msg->type == JSONRPC_REPLY
                    && idl->request_id
                    && json_equal(idl->request_id, msg->id)) {
@@ -349,24 +363,44 @@  ovsdb_idl_run(struct ovsdb_idl *idl)
                 /* Reply to our "get_schema" request. */
                 json_destroy(idl->request_id);
                 idl->request_id = NULL;
-                ovsdb_idl_send_monitor_request(idl, msg->result);
-                idl->state = IDL_S_MONITOR_REQUESTED;
+                idl->schema = json_clone(msg->result);
+                ovsdb_idl_send_monitor2_request(idl);
+                idl->state = IDL_S_MONITOR2_REQUESTED;
                 break;
 
             case IDL_S_MONITOR_REQUESTED:
-                /* Reply to our "monitor" request. */
+            case IDL_S_MONITOR2_REQUESTED:
+                /* Reply to our "monitor" or "monitor2" request. */
                 idl->change_seqno++;
                 json_destroy(idl->request_id);
                 idl->request_id = NULL;
-                idl->state = IDL_S_MONITORING;
                 ovsdb_idl_clear(idl);
-                ovsdb_idl_parse_update(idl, msg->result);
+                if (idl->state == IDL_S_MONITOR_REQUESTED) {
+                    idl->state = IDL_S_MONITORING;
+                    ovsdb_idl_parse_update(idl, msg->result);
+                } else { /* IDL_S_MONITOR2_REQUESTED. */
+                    idl->state = IDL_S_MONITORING2;
+                    ovsdb_idl_parse_update2(idl, msg->result);
+                }
+
+                /* Schema is not useful after monitor request is accepted
+                 * by the server.  */
+                json_destroy(idl->schema);
+                idl->schema = NULL;
                 break;
 
             case IDL_S_MONITORING:
+            case IDL_S_MONITORING2:
             default:
                 OVS_NOT_REACHED();
             }
+        } else if (msg->type == JSONRPC_NOTIFY
+            && !strcmp(msg->method, "update")
+            && msg->params->type == JSON_ARRAY
+            && msg->params->u.array.n == 2
+            && msg->params->u.array.elems[0]->type == JSON_NULL) {
+            /* Database contents changed. */
+            ovsdb_idl_parse_update(idl, msg->params->u.array.elems[1]);
         } else if (msg->type == JSONRPC_REPLY
                    && idl->lock_request_id
                    && json_equal(idl->lock_request_id, msg->id)) {
@@ -380,6 +414,16 @@  ovsdb_idl_run(struct ovsdb_idl *idl)
                    && !strcmp(msg->method, "stolen")) {
             /* Someone else stole our lock. */
             ovsdb_idl_parse_lock_notify(idl, msg->params, false);
+        } else if (msg->type == JSONRPC_ERROR
+                   && idl->state == IDL_S_MONITOR2_REQUESTED) {
+            if (msg->error && !strcmp(json_string(msg->error),
+                                      "unknown method")) {
+                /* Fall back to using "monitor" method.  */
+                json_destroy(idl->request_id);
+                idl->request_id = NULL;
+                ovsdb_idl_send_monitor_request(idl);
+                idl->state = IDL_S_MONITOR_REQUESTED;
+            }
         } else if ((msg->type == JSONRPC_ERROR
                     || msg->type == JSONRPC_REPLY)
                    && ovsdb_idl_txn_process_reply(idl, msg)) {
@@ -690,14 +734,15 @@  parse_schema(const struct json *schema_json)
 }
 
 static void
-ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl,
-                               const struct json *schema_json)
+ovsdb_idl_send_monitor_request__(struct ovsdb_idl *idl,
+                                 const char *method)
 {
-    struct shash *schema = parse_schema(schema_json);
+    struct shash *schema;
     struct json *monitor_requests;
     struct jsonrpc_msg *msg;
     size_t i;
 
+    schema = parse_schema(idl->schema);
     monitor_requests = json_object_create();
     for (i = 0; i < idl->class->n_tables; i++) {
         const struct ovsdb_idl_table *table = &idl->tables[i];
@@ -747,7 +792,7 @@  ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl,
 
     json_destroy(idl->request_id);
     msg = jsonrpc_create_request(
-        "monitor",
+        method,
         json_array_create_3(json_string_create(idl->class->database),
                             json_null_create(), monitor_requests),
         &idl->request_id);
@@ -755,16 +800,124 @@  ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl,
 }
 
 static void
-ovsdb_idl_parse_update(struct ovsdb_idl *idl, const struct json *table_updates)
+ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl)
+{
+    ovsdb_idl_send_monitor_request__(idl, "monitor");
+}
+
+static void
+log_parse_update_error(struct ovsdb_error *error)
 {
-    struct ovsdb_error *error = ovsdb_idl_parse_update__(idl, table_updates);
-    if (error) {
         if (!VLOG_DROP_WARN(&syntax_rl)) {
             char *s = ovsdb_error_to_string(error);
             VLOG_WARN_RL(&syntax_rl, "%s", s);
             free(s);
         }
         ovsdb_error_destroy(error);
+}
+
+static void
+ovsdb_idl_send_monitor2_request(struct ovsdb_idl *idl)
+{
+    ovsdb_idl_send_monitor_request__(idl, "monitor2");
+}
+
+static void
+ovsdb_idl_parse_update2(struct ovsdb_idl *idl,
+                        const struct json *table_updates)
+{
+    struct ovsdb_error *error = ovsdb_idl_parse_update2__(idl, table_updates);
+    if (error) {
+        log_parse_update_error(error);
+    }
+}
+
+static struct ovsdb_error *
+ovsdb_idl_parse_update2__(struct ovsdb_idl *idl,
+                          const struct json *table_updates)
+{
+    const struct shash_node *tables_node;
+
+    if (table_updates->type != JSON_OBJECT) {
+        return ovsdb_syntax_error(table_updates, NULL,
+                                  "<table-updates> is not an object");
+    }
+
+    SHASH_FOR_EACH (tables_node, json_object(table_updates)) {
+        const struct json *table_update2 = tables_node->data;
+        const struct shash_node *table_node;
+        struct ovsdb_idl_table *table;
+
+        table = shash_find_data(&idl->table_by_name, tables_node->name);
+        if (!table) {
+            return ovsdb_syntax_error(
+                table_updates, NULL,
+                "<table-updates> includes unknown table \"%s\"",
+                tables_node->name);
+        }
+
+        if (table_update2->type != JSON_OBJECT) {
+            return ovsdb_syntax_error(table_update2, NULL,
+                                      "<table-update> for table \"%s\" is "
+                                      "not an object", table->class->name);
+        }
+        SHASH_FOR_EACH (table_node, json_object(table_update2)) {
+            const struct json *row_update2 = table_node->data;
+            struct uuid uuid;
+
+            if (!uuid_from_string(&uuid, table_node->name)) {
+                return ovsdb_syntax_error(table_update2, NULL,
+                                          "<table-update2> for table \"%s\" "
+                                          "contains bad UUID "
+                                          "\"%s\" as member name",
+                                          table->class->name,
+                                          table_node->name);
+            }
+            if (row_update2->type != JSON_OBJECT) {
+                return ovsdb_syntax_error(row_update2, NULL,
+                                          "<table-update2> for table \"%s\" "
+                                          "contains <row-update2> for %s that "
+                                          "is not an object",
+                                          table->class->name,
+                                          table_node->name);
+            }
+
+            const char *ops[] = {"modify", "insert", "delete", "initial"};
+            const char *operation;
+            const struct json *row;
+            int i;
+
+            for (i = 0; i < ARRAY_SIZE(ops); i++) {
+                operation = ops[i];
+                row = shash_find_data(json_object(row_update2), operation);
+
+                if (row)  {
+                    if (ovsdb_idl_process_update2(table, &uuid, operation,
+                                                  row)) {
+                        idl->change_seqno++;
+                    }
+                    break;
+                }
+            }
+
+            /* row_update2 should contain one of the objects */
+            if (i == ARRAY_SIZE(ops)) {
+                return ovsdb_syntax_error(row_update2, NULL,
+                                          "<row_update2> includes unknown "
+                                          "object");
+            }
+        }
+    }
+
+    return NULL;
+}
+
+static void
+ovsdb_idl_parse_update(struct ovsdb_idl *idl, const struct json *table_updates)
+{
+    struct ovsdb_error *error = ovsdb_idl_parse_update__(idl, table_updates);
+    if (error) {
+        log_parse_update_error(error);
     }
 }
 
@@ -916,6 +1069,120 @@  ovsdb_idl_process_update(struct ovsdb_idl_table *table,
 /* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
  * otherwise. */
 static bool
+ovsdb_idl_process_update2(struct ovsdb_idl_table *table,
+                          const struct uuid *uuid,
+                          const char *operation,
+                          const struct json *json_row)
+{
+    struct ovsdb_idl_row *row;
+
+    row = ovsdb_idl_get_row(table, uuid);
+    if (!strcmp(operation, "delete")) {
+        /* Delete row. */
+        if (row && !ovsdb_idl_row_is_orphan(row)) {
+            ovsdb_idl_delete_row(row);
+        } else {
+            VLOG_WARN_RL(&semantic_rl, "cannot delete missing row "UUID_FMT" "
+                         "from table %s",
+                         UUID_ARGS(uuid), table->class->name);
+            return false;
+        }
+    } else if (!strcmp(operation, "insert") || !strcmp(operation, "initial")) {
+        /* Insert row. */
+        if (!row) {
+            ovsdb_idl_insert_row(ovsdb_idl_row_create(table, uuid), json_row);
+        } else if (ovsdb_idl_row_is_orphan(row)) {
+            ovsdb_idl_insert_row(row, json_row);
+        } else {
+            VLOG_WARN_RL(&semantic_rl, "cannot add existing row "UUID_FMT" to "
+                         "table %s", UUID_ARGS(uuid), table->class->name);
+            ovsdb_idl_delete_row(row);
+            ovsdb_idl_insert_row(row, json_row);
+        }
+    } else if (!strcmp(operation, "modify")) {
+        /* Modify row. */
+        if (row) {
+            if (!ovsdb_idl_row_is_orphan(row)) {
+                return ovsdb_idl_modify_row_by_diff(row, json_row);
+            } else {
+                VLOG_WARN_RL(&semantic_rl, "cannot modify missing but "
+                             "referenced row "UUID_FMT" in table %s",
+                             UUID_ARGS(uuid), table->class->name);
+                return false;
+            }
+        } else {
+            VLOG_WARN_RL(&semantic_rl, "cannot modify missing row "UUID_FMT" "
+                         "in table %s", UUID_ARGS(uuid), table->class->name);
+            return false;
+        }
+    } else {
+            VLOG_WARN_RL(&semantic_rl, "unknown operation %s to "
+                         "table %s", operation, table->class->name);
+            return false;
+    }
+
+    return true;
+}
+
+/* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
+ * otherwise. */
+static bool
+ovsdb_idl_row_apply_diff(struct ovsdb_idl_row *row,
+                         const struct json *diff_json)
+{
+    struct ovsdb_idl_table *table = row->table;
+    struct shash_node *node;
+    bool changed = false;
+
+    SHASH_FOR_EACH (node, json_object(diff_json)) {
+        const char *column_name = node->name;
+        const struct ovsdb_idl_column *column;
+        struct ovsdb_datum diff;
+        struct ovsdb_error *error;
+
+        column = shash_find_data(&table->columns, column_name);
+        if (!column) {
+            VLOG_WARN_RL(&syntax_rl, "unknown column %s updating row "UUID_FMT,
+                         column_name, UUID_ARGS(&row->uuid));
+            continue;
+        }
+
+        error = ovsdb_transient_datum_from_json(&diff, &column->type,
+                                                node->data);
+        if (!error) {
+            unsigned int column_idx = column - table->class->columns;
+            struct ovsdb_datum *old = &row->old[column_idx];
+            struct ovsdb_datum *new;
+
+            new = ovsdb_datum_apply_diff(old, &diff, &column->type);
+            if (!new) {
+                VLOG_WARN_RL(&syntax_rl, "update2 failed to modify column "
+                             "%s row "UUID_FMT, column_name,
+                             UUID_ARGS(&row->uuid));
+            } else {
+                ovsdb_datum_swap(old, new);
+                ovsdb_datum_destroy(new, &column->type);
+                free(new);
+                if (table->modes[column_idx] & OVSDB_IDL_ALERT) {
+                    changed = true;
+                }
+            }
+            ovsdb_datum_destroy(&diff, &column->type);
+        } else {
+            char *s = ovsdb_error_to_string(error);
+            VLOG_WARN_RL(&syntax_rl, "error parsing column %s in row "UUID_FMT
+                         " in table %s: %s", column_name,
+                         UUID_ARGS(&row->uuid), table->class->name, s);
+            free(s);
+            ovsdb_error_destroy(error);
+        }
+    }
+    return changed;
+}
+
+/* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
+ * otherwise. */
+static bool
 ovsdb_idl_row_update(struct ovsdb_idl_row *row, const struct json *row_json)
 {
     struct ovsdb_idl_table *table = row->table;
@@ -1182,6 +1449,20 @@  ovsdb_idl_modify_row(struct ovsdb_idl_row *row, const struct json *row_json)
 }
 
 static bool
+ovsdb_idl_modify_row_by_diff(struct ovsdb_idl_row *row,
+                             const struct json *diff_json)
+{
+    bool changed;
+
+    ovsdb_idl_row_unparse(row);
+    ovsdb_idl_row_clear_arcs(row, true);
+    changed = ovsdb_idl_row_apply_diff(row, diff_json);
+    ovsdb_idl_row_parse(row);
+
+    return changed;
+}
+
+static bool
 may_add_arc(const struct ovsdb_idl_row *src, const struct ovsdb_idl_row *dst)
 {
     const struct ovsdb_idl_arc *arc;
@@ -2559,11 +2840,13 @@  ovsdb_idl_is_lock_contended(const struct ovsdb_idl *idl)
     return idl->is_lock_contended;
 }
 
+
 static void
 ovsdb_idl_update_has_lock(struct ovsdb_idl *idl, bool new_has_lock)
 {
     if (new_has_lock && !idl->has_lock) {
-        if (idl->state == IDL_S_MONITORING) {
+        if (idl->state == IDL_S_MONITORING ||
+            idl->state == IDL_S_MONITORING2) {
             idl->change_seqno++;
         } else {
             /* We're setting up a session, so don't signal that the database
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index d3d2aeb..5ede75f 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -588,13 +588,13 @@  test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column (database n
 # Check that ovsdb-idl sent on "monitor" request and that it didn't
 # mention that table or column, and (for paranoia) that it did mention another
 # table and column.
-AT_CHECK([grep -c '"monitor"' stderr], [0], [1
+AT_CHECK([grep -c '"monitor\|monitor2"' stderr], [0], [1
 ])
-AT_CHECK([grep '"monitor"' stderr | grep link2], [1])
-AT_CHECK([grep '"monitor"' stderr | grep l2], [1])
-AT_CHECK([grep '"monitor"' stderr | grep -c '"link1"'], [0], [1
+AT_CHECK([grep '"monitor\|monitor2"' stderr | grep link2], [1])
+AT_CHECK([grep '"monitor\|monitor2"' stderr | grep l2], [1])
+AT_CHECK([grep '"monitor\|monitor2"' stderr | grep -c '"link1"'], [0], [1
 ])
-AT_CHECK([grep '"monitor"' stderr | grep -c '"ua"'], [0], [1
+AT_CHECK([grep '"monitor\|monitor2"' stderr | grep -c '"ua"'], [0], [1
 ])
 OVSDB_SERVER_SHUTDOWN
 AT_CLEANUP