There was a patch in OVS 3.1 that added support to the IDL code for specifying the permanent UUID of a row when inserting [1]. There are both C and Python implementations. Initially, I was adding support to ovsdbapp for this feature and noticed that the Python tests for this feature passed `new_uuid` to `insert()` as a string:
elif name == "insert_uuid": ... s = txn.insert(idl.tables["simple"], new_uuid=args[0], persist_uuid=True) when the existing code in `insert()` treats `new_uuid` as a `uuid.UUID` or None. def insert(self, table, new_uuid=None, persist_uuid=False): ... if new_uuid is None: new_uuid = uuid.uuid4() row = Row(self.idl, table, new_uuid, None, persist_uuid=persist_uuid) table.rows[row.uuid] = row self._txn_rows[row.uuid] = row return row It creates a `Row` with `row.uuid` of type string in this case instead of a `uuid.UUID` and then stores it in `table.rows` under a string key. This `Row` is fairly short-lived in that it gets deleted once we send the request to the ovsdb-server and call `Transaction.__disassemble()`. Then, when we get the OVSDB update notification, the `Row` is created normally and stored in `table.rows`. So at this point there's no real problem except that from an interface perspective, new_uuid is the wrong type. In client code, this can probably be problem having the returned `Row` having a string for `uuid`. This starts to get interesting when you try to simply fix this type issue. For the case where `new_uuid` already exists in the DB, If you pass `new_uuid=uuid.uuid4()` and fix the code in commit op["op"] = "insert" if row._persist_uuid: op["uuid"] = row.uuid to convert `row.uuid` to a string for the JSONRPC request, the code in insert that does `table.rows[row.uuid] = row` will then *overwrite* the existing row, then when the transaction is disassembled, that row will be deleted. Since the row exists in the server, the txn will fail. So your insert() call ends up deleting the row instead of adding a new one. The existing test case will catch this issue. The tests currently depend on getting a response from ovsdb-server regarding the duplicate UUID. So fixing it just in the Python implementation by checking locally `insert()` if the UUID was already stored in `table.rows` would be a behavior difference between the two. Trying to just pass the old row to the transaction, since `Row._data` would no longer be `None` would convert the insert request to an update. Knowing that the C IDL and Python IDL are very similar, I went to compare what they were doing. It was essentially the same. The C code does take a `const struct uuid *` as an argument to `ovsdb_idl_txn_insert()` like one would expect, and insert the new row into `table->rows`. The difference is, that `table->rows` is an hmap and hmaps allow duplicate keys. So the C IDL version is storing two copies of the same row in the DB after `ovsdb_idl_txn_insert()` and the one that is inserted, having been inserted into `txn->txn_rows` is looked up in `ovsdb_idl_txn_disassemble` and deleted directly with `hmap_remove()`. So things, perhaps accidentally, end up working fine in practice there. One caveat would be that any operation that tried to find the row while the transaction was active with something like `HMAP_FOR_EACH_WITH_HASH()` which `ovsdb_idl_get_row()`, will get the first one that is iterated over based on the hmap implementation. So my questions are: Do we really mean to be storing two identical rows in the C version? If not, should we also do local checks for the row client-side in both versions of the IDL code and fail early? It would be an API change to either assert or return NULL/None in the insert methods. One possible issue with this is that it would enable a race condition: Imagine two requests for deleting and then recreating an object with a persistent UUID and they are routed to different worker IDLs. The recreate could fail if it doesn't receive the update notification from ovsdb-server with the delete in time, and one of the reasons we want to use this feature is to avoid this kind of issue. If we actually intend to store multiple rows with the same UUID temporarily in the in-memory tables, then how do we want to do that in the Python code? table.rows technically isn't a dict, it's a custom class that subclasses dict, so if we absolutely needed to we could override `__setitem__`, `__delitem__`, and `__getitem__` to handle duplicates by storing/retrieving them from a list. Ultimately, it seems like we need to a) always send the insert transactions for persistent uuids (as the current code does) and b) do that without passing the wrong type in the Python code or inadvertently causing issues with blindly storing multiple rows with the same key in the table.rows hmap in the C code. Ideas? [1] https://github.com/openvswitch/ovs/commit/55b9507e6824b935ffa0205fc7c7bebfe4e54279 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev