ovsdb-idl: Add partial map updates functionality.

In the current implementation, every time an element of either a map or set
column has to be modified, the entire content of the column is sent to the
server to be updated. This is not a major problem if the information contained
in the column for the corresponding row is small, but there are cases where
these columns can have a significant amount of elements per row, or these
values are updated frequently, therefore the cost of the modifications becomes
high in terms of time and bandwidth.

In this solution, the ovsdb-idl code is modified to use the RFC 7047 'mutate'
operation, to allow sending partial modifications on map columns to the server.
The functionality is exposed to clients in the vswitch idl. This was
implemented through map operations.

A map operation is defined as an insertion, update or deletion of a key-value
pair inside a map. The idea is to minimize the amount of map operations
that are send to the OVSDB server when a transaction is committed.

In order to keep track of the requested map operations, structs map_op and
map_op_list were defined with accompanying functions to manipulate them. These
functions make sure that only one operation is send to the server for each
key-value that wants to be modified, so multiple operation on a key value are
collapsed into a single operation.

As an example, if a client using the IDL updates several times the value for
the same key, the functions will ensure that only the last value is send to
the server, instead of multiple updates. Or, if the client inserts a key-value,
and later on deletes the key before committing the transaction, then both
actions cancel out and no map operation is send for that key.

To keep track of the desired map operations on each transaction, a list of map
operations (struct map_op_list) is created for every column on the row on which
a map operation is performed. When a new map operation is requested on the same
column, the corresponding map_op_list is checked to verify if a previous
operations was performed on the same key, on the same transaction. If there is
no previous operation, then the new operation is just added into the list. But
if there was a previous operation on the same key, then the previous operation
is collapsed with the new operation into a single operation that preserves the
final result if both operations were to be performed sequentially. This design
keep a small memory footprint during transactions.

When a transaction is committed, the map operations lists are checked and
all map operations that belong to the same map are grouped together into a
single JSON RPC "mutate" operation, in which each map_op is transformed into
the necessary "insert" or "delete" mutators. Then the "mutate" operation is
added to the operations that will be send to the server.

Once the transaction is finished, all map operation lists are cleared and
deleted, so the next transaction starts with a clean board for map operations.

Using different structures and logic to handle map operations, instead of
trying to force the current structures (like 'old' and 'new' datums in the row)
to handle then, ensures that map operations won't mess up with the current
logic to generate JSON messages for other operations, avoids duplicating the
whole map for just a few changes, and is faster for insert and delete
operations, because there is no need to maintain the invariants in the 'new'
datum.

Signed-off-by: Edward Aymerich <edward.aymerich@hpe.com>
Signed-off-by: Arnoldo Lutz <arnoldo.lutz.guevara@hpe.com>
Co-authored-by: Arnoldo Lutz <arnoldo.lutz.guevara@hpe.com>
[blp@ovn.org made style changes and factored out error checking]
Signed-off-by: Ben Pfaff <blp@ovn.org>

lib/automake.mk

@@ -176,6 +176,8 @@ lib_libopenvswitch_la_SOURCES = \
 	lib/ovsdb-idl-provider.h \
 	lib/ovsdb-idl.c \
 	lib/ovsdb-idl.h \
+	lib/ovsdb-map-op.c \
+	lib/ovsdb-map-op.h \
 	lib/ovsdb-parser.c \
 	lib/ovsdb-parser.h \
 	lib/ovsdb-types.c \
@@ -508,4 +510,3 @@ lib-install-data-local:
 	$(MKDIR_P) $(DESTDIR)$(PKIDIR)
 	$(MKDIR_P) $(DESTDIR)$(LOGDIR)
 	$(MKDIR_P) $(DESTDIR)$(DBDIR)
-

lib/ovsdb-idl-provider.h

@@ -19,6 +19,7 @@
 #include "hmap.h"
 #include "openvswitch/list.h"
 #include "ovsdb-idl.h"
+#include "ovsdb-map-op.h"
 #include "ovsdb-types.h"
 #include "shash.h"
 #include "uuid.h"
@@ -36,6 +37,8 @@ struct ovsdb_idl_row {
     unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
     unsigned long int *written; /* Bitmap of columns from "new" to write. */
     struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
+    unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
+    struct map_op_list **map_op_lists; /* Per-column map operations. */
 
     /* Tracking data */
     unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX];

lib/ovsdb-idl.c

@@ -181,6 +181,7 @@ static struct ovsdb_idl_row *ovsdb_idl_row_create(struct ovsdb_idl_table *,
                                                   const struct uuid *);
 static void ovsdb_idl_row_destroy(struct ovsdb_idl_row *);
 static void ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl *);
+static void ovsdb_idl_destroy_all_map_op_lists(struct ovsdb_idl_row *);
 
 static void ovsdb_idl_row_parse(struct ovsdb_idl_row *);
 static void ovsdb_idl_row_unparse(struct ovsdb_idl_row *);
@@ -191,6 +192,12 @@ static void ovsdb_idl_row_clear_arcs(struct ovsdb_idl_row *, bool destroy_dsts);
 static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *);
 static bool ovsdb_idl_txn_process_reply(struct ovsdb_idl *,
                                         const struct jsonrpc_msg *msg);
+static bool ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *,
+                                            struct json *);
+static void ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *,
+                                     const struct ovsdb_idl_column *,
+                                     struct ovsdb_datum *,
+                                     enum map_op_type);
 
 static void ovsdb_idl_send_lock_request(struct ovsdb_idl *);
 static void ovsdb_idl_send_unlock_request(struct ovsdb_idl *);
@@ -1596,6 +1603,8 @@ ovsdb_idl_row_create(struct ovsdb_idl_table *table, const struct uuid *uuid)
     hmap_insert(&table->rows, &row->hmap_node, uuid_hash(uuid));
     row->uuid = *uuid;
     row->table = table;
+    row->map_op_written = NULL;
+    row->map_op_lists = NULL;
     return row;
 }
 
@@ -1605,6 +1614,7 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
     if (row) {
         ovsdb_idl_row_clear_old(row);
         hmap_remove(&row->table->rows, &row->hmap_node);
+        ovsdb_idl_destroy_all_map_op_lists(row);
         if (ovsdb_idl_track_is_set(row->table)) {
             row->change_seqno[OVSDB_IDL_CHANGE_DELETE]
                 = row->table->change_seqno[OVSDB_IDL_CHANGE_DELETE]
@@ -1617,6 +1627,27 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
     }
 }
 
+static void
+ovsdb_idl_destroy_all_map_op_lists(struct ovsdb_idl_row *row)
+{
+    if (row->map_op_written) {
+        /* Clear Map Operation Lists */
+        size_t idx, n_columns;
+        const struct ovsdb_idl_column *columns;
+        const struct ovsdb_type *type;
+        n_columns = row->table->class->n_columns;
+        columns = row->table->class->columns;
+        BITMAP_FOR_EACH_1 (idx, n_columns, row->map_op_written) {
+            type = &columns[idx].type;
+            map_op_list_destroy(row->map_op_lists[idx], type);
+        }
+        free(row->map_op_lists);
+        bitmap_free(row->map_op_written);
+        row->map_op_lists = NULL;
+        row->map_op_written = NULL;
+    }
+}
+
 static void
 ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl *idl)
 {
@@ -2144,6 +2175,7 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
     txn->idl->txn = NULL;
 
     HMAP_FOR_EACH_SAFE (row, next, txn_node, &txn->txn_rows) {
+        ovsdb_idl_destroy_all_map_op_lists(row);
         if (row->old) {
             if (row->written) {
                 ovsdb_idl_row_unparse(row);
@@ -2172,6 +2204,116 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
     hmap_init(&txn->txn_rows);
 }
 
+static bool
+ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *row,
+                                struct json *mutations)
+{
+    const struct ovsdb_idl_table_class *class = row->table->class;
+    size_t idx;
+    bool any_mutations = false;
+
+    BITMAP_FOR_EACH_1(idx, class->n_columns, row->map_op_written) {
+        struct map_op_list *map_op_list;
+        const struct ovsdb_idl_column *column;
+        const struct ovsdb_datum *old_datum;
+        enum ovsdb_atomic_type key_type, value_type;
+        struct json *mutation, *map, *col_name, *mutator;
+        struct json *del_set, *ins_map;
+        bool any_del, any_ins;
+
+        map_op_list = row->map_op_lists[idx];
+        column = &class->columns[idx];
+        key_type = column->type.key.type;
+        value_type = column->type.value.type;
+        old_datum = ovsdb_idl_read(row, column);
+
+        del_set = json_array_create_empty();
+        ins_map = json_array_create_empty();
+        any_del = false;
+        any_ins = false;
+
+        for (struct map_op *map_op = map_op_list_first(map_op_list); map_op;
+             map_op = map_op_list_next(map_op_list, map_op)) {
+
+            if (map_op_type(map_op) == MAP_OP_UPDATE) {
+                /* Find out if value really changed. */
+                struct ovsdb_datum *new_datum;
+                unsigned int pos;
+                new_datum = map_op_datum(map_op);
+                pos = ovsdb_datum_find_key(old_datum,
+                                           &new_datum->keys[0],
+                                           key_type);
+                if (ovsdb_atom_equals(&new_datum->values[0],
+                                      &old_datum->values[pos],
+                                      value_type)) {
+                    /* No change in value. Move on to next update. */
+                    continue;
+                }
+            } else if (map_op_type(map_op) == MAP_OP_DELETE){
+                /* Verify that there is a key to delete. */
+                unsigned int pos;
+                pos = ovsdb_datum_find_key(old_datum,
+                                           &map_op_datum(map_op)->keys[0],
+                                           key_type);
+                if (pos == UINT_MAX) {
+                    /* No key to delete.  Move on to next update. */
+                    VLOG_WARN("Trying to delete a key that doesn't "
+                              "exist in the map.");
+                    continue;
+                }
+            }
+
+            if (map_op_type(map_op) == MAP_OP_INSERT) {
+                map = json_array_create_2(
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                       key_type),
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->values[0],
+                                       value_type));
+                json_array_add(ins_map, map);
+                any_ins = true;
+            } else { /* MAP_OP_UPDATE or MAP_OP_DELETE */
+                map = ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                         key_type);
+                json_array_add(del_set, map);
+                any_del = true;
+            }
+
+            /* Generate an additional insert mutate for updates. */
+            if (map_op_type(map_op) == MAP_OP_UPDATE) {
+                map = json_array_create_2(
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                       key_type),
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->values[0],
+                                       value_type));
+                json_array_add(ins_map, map);
+                any_ins = true;
+            }
+        }
+
+        if (any_del) {
+            col_name = json_string_create(column->name);
+            mutator = json_string_create("delete");
+            map = json_array_create_2(json_string_create("set"), del_set);
+            mutation = json_array_create_3(col_name, mutator, map);
+            json_array_add(mutations, mutation);
+            any_mutations = true;
+        } else {
+            json_destroy(del_set);
+        }
+        if (any_ins) {
+            col_name = json_string_create(column->name);
+            mutator = json_string_create("insert");
+            map = json_array_create_2(json_string_create("map"), ins_map);
+            mutation = json_array_create_3(col_name, mutator, map);
+            json_array_add(mutations, mutation);
+            any_mutations = true;
+        } else {
+            json_destroy(ins_map);
+        }
+    }
+    return any_mutations;
+}
+
 /* Attempts to commit 'txn'.  Returns the status of the commit operation, one
  * of the following TXN_* constants:
  *
@@ -2358,6 +2500,28 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
                 json_destroy(op);
             }
         }
+
+        /* Add mutate operation, for partial map updates. */
+        if (row->map_op_written) {
+            struct json *op, *mutations;
+            bool any_mutations;
+
+            op = json_object_create();
+            json_object_put_string(op, "op", "mutate");
+            json_object_put_string(op, "table", class->name);
+            json_object_put(op, "where", where_uuid_equals(&row->uuid));
+            mutations = json_array_create_empty();
+            any_mutations = ovsdb_idl_txn_extract_mutations(row, mutations);
+            json_object_put(op, "mutations", mutations);
+
+            if (any_mutations) {
+                op = substitute_uuids(op, txn);
+                json_array_add(operations, op);
+                any_updates = true;
+            } else {
+                json_destroy(op);
+            }
+        }
     }
 
     /* Add increment. */
@@ -3166,6 +3330,118 @@ ovsdb_idl_parse_lock_notify(struct ovsdb_idl *idl,
     }
 }
 
+/* Inserts a new Map Operation into current transaction. */
+static void
+ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *row,
+                         const struct ovsdb_idl_column *column,
+                         struct ovsdb_datum *datum,
+                         enum map_op_type op_type)
+{
+    const struct ovsdb_idl_table_class *class;
+    size_t column_idx;
+    struct map_op *map_op;
+
+    class = row->table->class;
+    column_idx = column - class->columns;
+
+    /* Check if a map operation list exists for this column. */
+    if (!row->map_op_written) {
+        row->map_op_written = bitmap_allocate(class->n_columns);
+        row->map_op_lists = xzalloc(class->n_columns *
+                                    sizeof *row->map_op_lists);
+    }
+    if (!row->map_op_lists[column_idx]) {
+        row->map_op_lists[column_idx] = map_op_list_create();
+    }
+
+    /* Add a map operation to the corresponding list. */
+    map_op = map_op_create(datum, op_type);
+    bitmap_set1(row->map_op_written, column_idx);
+    map_op_list_add(row->map_op_lists[column_idx], map_op, &column->type);
+
+    /* Add this row to transaction's list of rows. */
+    if (hmap_node_is_null(&row->txn_node)) {
+        hmap_insert(&row->table->idl->txn->txn_rows, &row->txn_node,
+                    uuid_hash(&row->uuid));
+    }
+}
+
+static bool
+is_valid_partial_update(const struct ovsdb_idl_row *row,
+                        const struct ovsdb_idl_column *column,
+                        struct ovsdb_datum *datum)
+{
+    /* Verify that this column is being monitored. */
+    unsigned int column_idx = column - row->table->class->columns;
+    if (!(row->table->modes[column_idx] & OVSDB_IDL_MONITOR)) {
+        VLOG_WARN("cannot partially update non-monitored column");
+        return false;
+    }
+
+    /* Verify that the update affects a single element. */
+    if (datum->n != 1) {
+        VLOG_WARN("invalid datum for partial update");
+        return false;
+    }
+
+    return true;
+}
+
+/* Inserts the key-value specified in 'datum' into the map in 'column' in
+ * 'row_'. If the key already exist in 'column', then it's value is updated
+ * with the value in 'datum'. The key-value in 'datum' must be of the same type
+ * as the keys-values in 'column'. This function takes ownership of 'datum'.
+ *
+ * Usually this function is used indirectly through one of the "update"
+ * functions generated by vswitch-idl. */
+void
+ovsdb_idl_txn_write_partial_map(const struct ovsdb_idl_row *row_,
+                                const struct ovsdb_idl_column *column,
+                                struct ovsdb_datum *datum)
+{
+    struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
+    enum ovsdb_atomic_type key_type;
+    enum map_op_type op_type;
+    unsigned int pos;
+    const struct ovsdb_datum *old_datum;
+
+    if (!is_valid_partial_update(row, column, datum)) {
+        ovsdb_datum_destroy(datum, &column->type);
+        return;
+    }
+
+    /* Find out if this is an insert or an update. */
+    key_type = column->type.key.type;
+    old_datum = ovsdb_idl_read(row, column);
+    pos = ovsdb_datum_find_key(old_datum, &datum->keys[0], key_type);
+    op_type = pos == UINT_MAX ? MAP_OP_INSERT : MAP_OP_UPDATE;
+
+    ovsdb_idl_txn_add_map_op(row, column, datum, op_type);
+}
+
+/* Deletes the key specified in 'datum' from the map in 'column' in 'row_'.
+ * The key in 'datum' must be of the same type as the keys in 'column'.
+ * The value in 'datum' must be NULL. This function takes ownership of
+ * 'datum'.
+ *
+ * Usually this function is used indirectly through one of the "update"
+ * functions generated by vswitch-idl. */
+void
+ovsdb_idl_txn_delete_partial_map(const struct ovsdb_idl_row *row_,
+                                 const struct ovsdb_idl_column *column,
+                                 struct ovsdb_datum *datum)
+{
+    struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
+
+    if (!is_valid_partial_update(row, column, datum)) {
+        struct ovsdb_type type_ = column->type;
+        type_.value.type = OVSDB_TYPE_VOID;
+        ovsdb_datum_destroy(datum, &type_);
+        return;
+    }
+    ovsdb_idl_txn_add_map_op(row, column, datum, MAP_OP_DELETE);
+}
+
 void
 ovsdb_idl_loop_destroy(struct ovsdb_idl_loop *loop)
 {

lib/ovsdb-idl.h

@@ -265,6 +265,12 @@ void ovsdb_idl_txn_write(const struct ovsdb_idl_row *,
 void ovsdb_idl_txn_write_clone(const struct ovsdb_idl_row *,
                                const struct ovsdb_idl_column *,
                                const struct ovsdb_datum *);
+void ovsdb_idl_txn_write_partial_map(const struct ovsdb_idl_row *,
+                                     const struct ovsdb_idl_column *,
+                                     struct ovsdb_datum *);
+void ovsdb_idl_txn_delete_partial_map(const struct ovsdb_idl_row *,
+                                      const struct ovsdb_idl_column *,
+                                      struct ovsdb_datum *);
 void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *);
 const struct ovsdb_idl_row *ovsdb_idl_txn_insert(
     struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *,