[ovs-dev] [PATCH] ovsdb-idl: Fix *_is_new() IDL functions

[Dumitru Ceara]

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 <mark.d.gray at redhat.com>
> Suggested-by: Dumitru Ceara <dceara at redhat.com>
> 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.

Regards,
Dumitru

>  }
>  
>  /* Returns true if 'row' was deleted since the last change tracking reset. */
>