[ovs-dev] [PATCH v5 3/3] ovsdb-idl: Force IDL retry when missing updates encountered.

Dumitru Ceara dceara at redhat.com
Thu May 28 08:24:23 UTC 2020


On 5/28/20 9:08 AM, Han Zhou wrote:
> Hi Dumitru, please see my comments below.
> 
> On Thu, May 7, 2020 at 4:21 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>>
>> Adds a generic recovery mechanism which triggers an IDL retry with fast
>> resync disabled in case the IDL has detected that it ended up in an
>> inconsistent state due to other bugs in the ovsdb-server/ovsdb-idl
>> implementation.
>>
>> CC: Andy Zhou <azhou at ovn.org <mailto:azhou at ovn.org>>
>> Fixes: db2b5757328c ("lib: add monitor2 support in ovsdb-idl.")
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>>
>> ---
>>  lib/ovsdb-idl.c |   37 +++++++++++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
>> index 557f61c..076f0a1 100644
>> --- a/lib/ovsdb-idl.c
>> +++ b/lib/ovsdb-idl.c
>> @@ -328,7 +328,8 @@ static bool ovsdb_idl_process_update(struct
> ovsdb_idl_table *,
>>  static bool ovsdb_idl_process_update2(struct ovsdb_idl_table *,
>>                                        const struct uuid *,
>>                                        const char *operation,
>> -                                      const struct json *row);
>> +                                      const struct json *row,
>> +                                      bool *inconsistent);
>>  static void ovsdb_idl_insert_row(struct ovsdb_idl_row *, const struct
> json *);
>>  static void ovsdb_idl_delete_row(struct ovsdb_idl_row *);
>>  static bool ovsdb_idl_modify_row(struct ovsdb_idl_row *, const struct
> json *);
>> @@ -2420,10 +2421,26 @@ ovsdb_idl_db_parse_update__(struct
> ovsdb_idl_db *db,
>>                      row = shash_find_data(json_object(row_update),
> operation);
>>
>>                      if (row)  {
>> +                        bool inconsistent = false;
>> +
>>                          if (ovsdb_idl_process_update2(table, &uuid,
> operation,
>> -                                                      row)) {
>> +                                                      row,
> &inconsistent)) {
>>                              db->change_seqno++;
>>                          }
>> +
>> +                        /* If the db is in an inconsistent state,
> clear the
>> +                         * db->last_id and retry to get the complete
> view of
>> +                         * the database.
>> +                         */
>> +                        if (inconsistent) {
>> +                            memset(&db->last_id, 0, sizeof db->last_id);
>> +                            ovsdb_idl_retry(db->idl);
> 
> Just to confirm, we want to retry regardless of the monitor version,
> right? I.e. even if we are not using monitor_cond_since (e.g. the server
> doesn't support it), but if there is an inconsistency, retry would still
> help. Right?
> 

Right, even if we're just using monitor_cond, if we detect an
inconsistency we need to retry. In that case last_id would've already
been 0 but it doesn't hurt to reset it.

>> +                            return ovsdb_error(NULL,
>> +                                               "<row_update2>
> received for "
>> +                                               "inconsistent IDL: "
>> +                                               "reconnecting IDL with "
>> +                                               "fast resync disabled");
> 
> The message could be revised to "... reconnecting IDL and resync all
> data.", because I feel "with fast resync disabled" sounds like we are
> not using monitor_cond_since.

Sounds better indeed, I'll change it.

Thanks,
Dumitru

> 
> Thanks,
> Han
> 
>> +                        }
>>                          break;
>>                      }
>>                  }
>> @@ -2547,16 +2564,26 @@ ovsdb_idl_process_update(struct
> ovsdb_idl_table *table,
>>  }
>>
>>  /* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
>> - * otherwise. */
>> + * otherwise.
>> + *
>> + * NOTE: When processing the "modify" updates, the IDL can determine that
>> + * previous updates were missed (e.g., due to bugs) and that rows
> that don't
>> + * exist locally should be updated. This indicates that the
>> + * IDL is in an inconsistent state and, unlike in
> ovsdb_idl_process_update(),
>> + * the missing rows cannot be inserted. If this is the case,
> 'inconsistent'
>> + * is set to true to indicate the catastrophic failure.
>> + */
>>  static bool
>>  ovsdb_idl_process_update2(struct ovsdb_idl_table *table,
>>                            const struct uuid *uuid,
>>                            const char *operation,
>> -                          const struct json *json_row)
>> +                          const struct json *json_row,
>> +                          bool *inconsistent)
>>  {
>>      struct ovsdb_idl_row *row;
>>
>>      row = ovsdb_idl_get_row(table, uuid);
>> +    *inconsistent = false;
>>      if (!strcmp(operation, "delete")) {
>>          /* Delete row. */
>>          if (row && !ovsdb_idl_row_is_orphan(row)) {
>> @@ -2588,11 +2615,13 @@ ovsdb_idl_process_update2(struct
> ovsdb_idl_table *table,
>>                  VLOG_WARN_RL(&semantic_rl, "cannot modify missing but "
>>                               "referenced row "UUID_FMT" in table %s",
>>                               UUID_ARGS(uuid), table->class_->name);
>> +                *inconsistent = true;
>>                  return false;
>>              }
>>          } else {
>>              VLOG_WARN_RL(&semantic_rl, "cannot modify missing row
> "UUID_FMT" "
>>                           "in table %s", UUID_ARGS(uuid),
> table->class_->name);
>> +            *inconsistent = true;
>>              return false;
>>          }
>>      } else {
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org <mailto:dev at openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list