On 1/8/21 2:59 PM, Dumitru Ceara wrote:
> On 12/19/20 3:44 AM, Ben Pfaff wrote:
>> This change breaks the IDL into two layers: the IDL proper, whose
>> interface to its client is unchanged, and a low-level library called
>> the OVSDB "client synchronization" (CS) library. There are two
>> reasons for this change. First, the IDL is big and complicated and
>> I think that this change factors out some of that complication into
>> a simpler lower layer. Second, the OVN northd implementation based
>> on DDlog can benefit from the client synchronization library even
>> though it would actually be made increasingly complicated by the IDL.
>>
>> Signed-off-by: Ben Pfaff <b...@ovn.org>
>> ---
>
> Hi Ben,
>
> Overall this looks OK to me.
>
> OVS & OVN unit tests pass with this series applied.
>
> There are however a couple memory leaks and I have another small comment
> inline.
>
>> lib/ovsdb-cs.c | 1955 ++++++++++++++++++++++++++++++++++
>> lib/ovsdb-cs.h | 141 ++-
>> lib/ovsdb-idl-provider.h | 8 +-
>> lib/ovsdb-idl.c | 2161 +++++++++-----------------------------
>> 4 files changed, 2563 insertions(+), 1702 deletions(-)
<snip>
>> +
>> +static void
>> +ovsdb_cs_db_destroy_tables(struct ovsdb_cs_db *db)
>> +{
>> + struct ovsdb_cs_db_table *table, *next;
>> + HMAP_FOR_EACH_SAFE (table, next, hmap_node, &db->tables) {
>> + json_destroy(table->ack_cond);
>> + json_destroy(table->req_cond);
>> + json_destroy(table->new_cond);
>
> We leak both 'table' and 'table->name' because we don't free them here.
Caught these leaks too while running idl tests with AddressSanitizer.
>
>> + hmap_remove(&db->tables, &table->hmap_node);
>> + }
>> + hmap_destroy(&db->tables);
>> +}
>> +
>> +static unsigned int
>> +ovsdb_cs_db_set_condition(struct ovsdb_cs_db *db, const char *table,
>> + const struct json *condition)
>> +{
>> + /* Compare the new condition to the last known condition which can be
>> + * either "new" (not sent yet), "requested" or "acked", in this order.
>> */
>> + struct ovsdb_cs_db_table *t = ovsdb_cs_db_get_table(db, table);
>> + const struct json *table_cond = (t->new_cond ? t->new_cond
>> + : t->req_cond ? t->req_cond
>> + : t->ack_cond);
>> + if (!condition_equal(condition, table_cond)) {
>> + json_destroy(t->new_cond);
>> + t->new_cond = condition_clone(condition);
>> + db->cond_changed = true;
>> + poll_immediate_wake();
>> + }
>> +
>> + /* Conditions will be up to date when we receive replies for already
>> + * requested and new conditions, if any. */
>> + return db->cond_seqno + (t->new_cond ? 1 : 0) + (t->req_cond ? 1 : 0);
>> +}
>> +
>> +/* Sets the replication condition for 'tc' in 'cs' to 'condition' and
>> arranges
>> + * to send the new condition to the database server.
>> + *
>> + * Return the next conditional update sequence number. When this value and
>> + * ovsdb_cs_get_condition_seqno() matches, 'cs' contains rows that match the
>> + * 'condition'. */
>> +unsigned int
>> +ovsdb_cs_set_condition(struct ovsdb_cs *cs, const char *table,
>> + const struct json *condition)
>> +{
>> + return ovsdb_cs_db_set_condition(&cs->data, table, condition);
>> +}
>> +
>> +/* Returns a "sequence number" that represents the number of conditional
>> + * monitoring updates successfully received by the OVSDB server of a CS
>> + * connection.
>> + *
>> + * ovsdb_cs_set_condition() sets a new condition that is different from the
>> + * current condtion, the next expected "sequence number" is returned.
>> + *
>> + * Whenever ovsdb_cs_get_condition_seqno() returns a value that matches the
>> + * return value of ovsdb_cs_set_condition(), the client is assured that:
>> + *
>> + * - The ovsdb_cs_set_condition() changes has been acknowledged by the
>> OVSDB
>> + * server.
>> + *
>> + * - 'cs' now contains the content matches the new conditions. */
>> +unsigned int
>> +ovsdb_cs_get_condition_seqno(const struct ovsdb_cs *cs)
>> +{
>> + return cs->data.cond_seqno;
>> +}
>> +
>> +static struct json *
>> +ovsdb_cs_create_cond_change_req(const struct json *cond)
>> +{
>> + struct json *monitor_cond_change_request = json_object_create();
>> + json_object_put(monitor_cond_change_request, "where", json_clone(cond));
>> + return monitor_cond_change_request;
>> +}
>> +
>> +static struct jsonrpc_msg *
>> +ovsdb_cs_db_compose_cond_change(struct ovsdb_cs_db *db)
>> +{
>> + if (!db->cond_changed) {
>> + return NULL;
>> + }
>> +
>> + struct json *monitor_cond_change_requests = NULL;
>> + struct ovsdb_cs_db_table *table;
>> + HMAP_FOR_EACH (table, hmap_node, &db->tables) {
>> + /* Always use the most recent conditions set by the CS client when
>> + * requesting monitor_cond_change, i.e., table->new_cond.
>> + */
>> + if (table->new_cond) {
>> + struct json *req =
>> + ovsdb_cs_create_cond_change_req(table->new_cond);
>> + if (req) {
>> + if (!monitor_cond_change_requests) {
>> + monitor_cond_change_requests = json_object_create();
>> + }
>> + json_object_put(monitor_cond_change_requests,
>> + table->name,
>> + json_array_create_1(req));
>> + }
>> + /* Mark the new condition as requested by moving it to req_cond.
>> + * If there's already requested condition that's a bug.
>> + */
>> + ovs_assert(table->req_cond == NULL);
>> + table->req_cond = table->new_cond;
>> + table->new_cond = NULL;
>> + }
>> + }
>> +
>> + if (!monitor_cond_change_requests) {
>> + return NULL;
>> + }
>> +
>> + db->cond_changed = false;
>> + struct json *params = json_array_create_3(json_clone(db->monitor_id),
>> + json_clone(db->monitor_id),
>> + monitor_cond_change_requests);
>> + return jsonrpc_create_request("monitor_cond_change", params, NULL);
>> +}
>> +
>> +/* Marks all requested table conditions in 'db' as acked by the server.
>> + * It should be called when the server replies to monitor_cond_change
>> + * requests.
>> + */
>> +static void
>> +ovsdb_cs_db_ack_condition(struct ovsdb_cs_db *db)
>> +{
>> + struct ovsdb_cs_db_table *table;
>> + HMAP_FOR_EACH (table, hmap_node, &db->tables) {
>> + if (table->req_cond) {
>> + json_destroy(table->ack_cond);
>> + table->ack_cond = table->req_cond;
>> + table->req_cond = NULL;
>> + }
>> + }
>> +}
>> +
>> +/* Should be called when the CS fsm is restarted and resyncs table
>> conditions
>> + * based on the state the DB is in:
>> + * - if a non-zero last_id is available for the DB then upon reconnect
>> + * the CS should first request acked conditions to avoid missing updates
>> + * about records that were added before the transaction with
>> + * txn-id == last_id. If there were requested condition changes in flight
>> + * (i.e., req_cond not NULL) and the CS client didn't set new conditions
>> + * (i.e., new_cond is NULL) then move req_cond to new_cond to trigger a
>> + * follow up monitor_cond_change request.
>> + * - if there's no last_id available for the DB then it's safe to use the
>> + * latest conditions set by the CS client even if they weren't acked yet.
>> + */
>> +static void
>> +ovsdb_cs_db_sync_condition(struct ovsdb_cs_db *db)
>> +{
>> + bool ack_all = uuid_is_zero(&db->last_id);
>> + if (ack_all) {
>> + db->cond_changed = false;
>> + }
>> +
>> + struct ovsdb_cs_db_table *table;
>> + HMAP_FOR_EACH (table, hmap_node, &db->tables) {
>> + /* When monitor_cond_since requests will be issued, the
>> + * table->ack_cond condition will be added to the "where" clause".
>> + * Follow up monitor_cond_change requests will use table->new_cond.
>> + */
>> + if (ack_all) {
>> + if (table->new_cond) {
>> + json_destroy(table->req_cond);
>> + table->req_cond = table->new_cond;
>> + table->new_cond = NULL;
>> + }
>> +
>> + if (table->req_cond) {
>> + json_destroy(table->ack_cond);
>> + table->ack_cond = table->req_cond;
>> + table->req_cond = NULL;
>> + }
>> + } else {
>> + /* If there was no "unsent" condition but instead a
>> + * monitor_cond_change request was in flight, move
>> table->req_cond
>> + * to table->new_cond and set db->cond_changed to trigger a new
>> + * monitor_cond_change request.
>> + *
>> + * However, if a new condition has been set by the CS client,
>> + * monitor_cond_change will be sent anyway and will use the most
>> + * recent table->new_cond so there's no need to update it here.
>> + */
>> + if (table->req_cond) {
>> + if (table->new_cond) {
>> + json_destroy(table->req_cond);
>> + } else {
>> + table->new_cond = table->req_cond;
>> + }
>> + table->req_cond = NULL;
>> + db->cond_changed = true;
>> + }
>
> This used to be:
>
> if (table->req_cond && !table->new_cond) {
> /* Move "req_cond" to "new_cond". */
> ovsdb_idl_condition_move(&table->new_cond, &table->req_cond);
> db->cond_changed = true;
> }
>
> Which is what the comment above tried to explain.
>
> Is there a case that was missed in the old code? If so, can we factor
> out the fix in a separate patch to make it easier to track?
I suppose that this additional check here is to clear 'req_cond', so
we will not assert inside ovsdb_cs_db_compose_cond_change().
Previously we had ovsdb_idl_condition_move() function that took care
of destroying the destination, but now we have no such function and
have an assert to be sure that we're not leaking the 'req_cond'.
This should not actually change the logic. But I think that we need
to rephrase part of the comment that starts with 'However' as it
doesn't reflect the code anymore.
Otherwise, the patch looks ok to me.
On the side note: don't we need to increase 'cond_seqno' here since
we're practically dropping one of the condition requests? User might
wait for this sequence number, but 'req_cond' will be cleared and
never be acked.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
