On 9/30/20 11:21 AM, Dumitru Ceara wrote: > On 9/29/20 8:34 PM, Mark Gray wrote: >> Currently all functions of the type *_is_new() always return >> 'false'. This patch resolves this issue by using the >> 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the >> 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row >> is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT' >> 'change_seqno' on clear. >> >> Further to this, the code is also updated to match this >> behaviour: >> >> When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT' >> 'change_seqno' is updated to match the new database >> change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' >> is not set for inserted rows (only for updated rows). >> >> At the end of a run, ovsdb_idl_db_track_clear() should be >> called to clear all tracking information, this includes >> resetting all row 'change_seqno' to zero. This will ensure >> that subsequent runs will not see a previously 'new' row. >> >> add_tracked_change_for_references() is updated to only >> track rows that reference the current row. >> >> Signed-off-by: Mark Gray <[email protected]> >> Suggested-by: Dumitru Ceara <[email protected]> >> Reported-at: https://bugzilla.redhat.com/1883562 > > Hi Mark, > > Thanks for working on this, it definitely looks like a nasty bug to me! > > We were lucky to not hit it in ovn-controller before. That's just > because all the "update" operations were handled there as "delete" + > "insert" but that is not mandatory and might change. > >> --- >> lib/ovsdb-idl.c | 39 +++++++++++++++++++++++++++------------ >> ovsdb/ovsdb-idlc.in | 2 +- >> 2 files changed, 28 insertions(+), 13 deletions(-) >> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >> index d8f221ca6..3cfd73871 100644 >> --- a/lib/ovsdb-idl.c >> +++ b/lib/ovsdb-idl.c >> @@ -1959,6 +1959,11 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) >> free(row->updated); >> row->updated = NULL; >> } >> + >> + row->change_seqno[OVSDB_IDL_CHANGE_INSERT] = >> + row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] = >> + row->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; >> + >> ovs_list_remove(&row->track_node); >> ovs_list_init(&row->track_node); >> if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) >> { >> @@ -2684,24 +2689,27 @@ ovsdb_idl_process_update2(struct ovsdb_idl_table >> *table, >> return OVSDB_IDL_UPDATE_DB_CHANGED; >> } >> >> -/* Recursively add rows to tracked change lists for current row >> - * and the rows that reference this row. */ >> +/* Recursively add rows to tracked change lists for all rows that reference >> + 'row'. */ >> static void >> add_tracked_change_for_references(struct ovsdb_idl_row *row) >> { >> - if (ovs_list_is_empty(&row->track_node) && >> - ovsdb_idl_track_is_set(row->table)) { >> - ovs_list_push_back(&row->table->track_list, >> - &row->track_node); >> - row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> - = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> - = row->table->db->change_seqno + 1; >> - >> const struct ovsdb_idl_arc *arc; >> LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) { >> - add_tracked_change_for_references(arc->src); >> + struct ovsdb_idl_row *ref = arc->src; >> + >> + if (ovs_list_is_empty(&ref->track_node) && >> + ovsdb_idl_track_is_set(ref->table)) { >> + ovs_list_push_back(&ref->table->track_list, >> + &ref->track_node); >> + >> + ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> + = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> + = ref->table->db->change_seqno + 1; >> + >> + add_tracked_change_for_references(ref); >> + } >> } >> - } >> } >> >> >> @@ -2767,7 +2775,14 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, >> const struct json *row_json, >> row->change_seqno[change] >> = row->table->change_seqno[change] >> = row->table->db->change_seqno + 1; >> + >> if (table->modes[column_idx] & OVSDB_IDL_TRACK) { >> + if (ovs_list_is_empty(&row->track_node) && >> + ovsdb_idl_track_is_set(row->table)) { >> + ovs_list_push_back(&row->table->track_list, >> + &row->track_node); >> + } >> + >> add_tracked_change_for_references(row); >> if (!row->updated) { >> row->updated = >> bitmap_allocate(class->n_columns); >> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in >> index 698fe25f3..89c3eb682 100755 >> --- a/ovsdb/ovsdb-idlc.in >> +++ b/ovsdb/ovsdb-idlc.in >> @@ -282,7 +282,7 @@ const struct %(s)s *%(s)s_table_track_get_first(const >> struct %(s)s_table *); >> /* Returns true if 'row' was inserted since the last change tracking reset. >> */ >> static inline bool %(s)s_is_new(const struct %(s)s *row) >> { >> - return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; >> + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0; > > This causes some issues with records created by the IDL client (e.g., > ovn-controller). Those are created with ovsdb_idl_txn_insert() and we > don't set the OVSDB_IDL_CHANGE_INSERT change_seqno in that case. > > Essentially *_is_new() will never return true for rows inserted locally. >
It would also be nice if we could add some OVS self tests in ovsdb-idl.at for all these changes. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
