[ovs-dev] [PATCH] ovsdb-idl.c: Clear conditions when clearing IDL.

Han Zhou hzhou at ovn.org
Tue Mar 3 06:43:54 UTC 2020


On Mon, Mar 2, 2020 at 7:55 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 2/29/20 12:00 AM, Dumitru Ceara wrote:
> > If the ovsdb-server reply to "monitor_cond_since" requests has
> > "found" == false then ovsdb_idl_db_parse_monitor_reply() calls
> > ovsdb_idl_db_clear() which iterates through all tables and
> > unconditionally sets table->cond_changed to false.
> >
> > However, if the client had already set a new condition for some of the
> > tables, this new condition request will never be sent to ovsdb-server
> > until the condition is reset to a different value. This is due to the
> > check in ovsdb_idl_db_set_condition().
> >
> > In order to fix this we now also call ovsdb_idl_condition_clear() for
> > each table condition in ovsdb_idl_db_clear(). This ensures that
> > resetting the condition to the same value as before the
> > "monitor_cond_since" reply will trigger a new request.
> >
> > One way to replicate the issue is described in the bugzilla reporting
> > the bug, when ovn-controller is configured to use "ovn-monitor-all":
> > https://bugzilla.redhat.com/show_bug.cgi?id=1808125#c6
> >
> > Reported-by: Dan Williams <dcbw at redhat.com>
> > Reported-at: https://bugzilla.redhat.com/1808125
> > CC: Han Zhou <hzhou at ovn.org>
> > Fixes: 403a6a0cb003 ("ovsdb-idl: Fast resync from server when
connection reset.")
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>
> Hi Han,
>
> I was wondering if you could review this change because ovn-kubernetes
> is trying to enable ovn-monitor-all for scalability reasons but their CI
> fails due to the bug fixed by the patch below.
>
> Thanks,
> Dumitru
>
> > ---
> >  lib/ovsdb-idl.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index 190143f..64721ca 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -610,7 +610,11 @@ ovsdb_idl_db_clear(struct ovsdb_idl_db *db)
> >          struct ovsdb_idl_table *table = &db->tables[i];
> >          struct ovsdb_idl_row *row, *next_row;
> >
> > -        table->cond_changed = false;
> > +        if (table->cond_changed) {
> > +            table->cond_changed = false;
> > +            ovsdb_idl_condition_clear(&table->condition);
> > +        }
> > +
> >          if (hmap_is_empty(&table->rows)) {
> >              continue;
> >          }
> >
>

Hi Dumitru,

If I understand the problem, it is when the client updated monitor
condition and at the same time it received monitor_cond_since reply from
server for it's previous conditional monitor request, and the handling of
the reply overwrite the flag table->cond_changed from true to false, and
because of this, the new condition is not sent to server.

However, I don't understand the fix. It seems that the fix should be "don't
overwrite the flag", instead of clearing the conditions as well. Also, by
simply looking at this code I don't understand why the flag needs to be set
to false when clearing data in IDL. What would be the problem if we keep it
as what it is? If you understand the details, could you explain more about
the fix?

In addition, I didn't change this logic when implementing
monitor_cond_since in 403a6a0cb003 ("ovsdb-idl: Fast resync from server
when connection reset."). In that commit it only reduced the chance of
calling ovsdb_idl_db_clear() as an optimization. Before that commit,
ovsdb_idl_db_parse_monitor_reply() always calls ovsdb_idl_db_clear() and it
would result in same problem.

Thanks,
Han


More information about the dev mailing list