On 11/10/20 11:07 AM, Ilya Maximets wrote: > On 11/9/20 12:09 PM, Dumitru Ceara wrote: >> IDL clients had no way of checking whether monitor_cond_change requests >> were pending (either local or in flight). This commit introduces a new >> API to check for the state of the conditional monitoring clauses. >> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> lib/ovsdb-idl.c | 40 ++++++++++++++++++++++++++++++++-------- >> lib/ovsdb-idl.h | 1 + >> 2 files changed, 33 insertions(+), 8 deletions(-) >> > > <snip> > >> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h >> index a1a5776..3f19d40 100644 >> --- a/lib/ovsdb-idl.h >> +++ b/lib/ovsdb-idl.h >> @@ -421,6 +421,7 @@ unsigned int ovsdb_idl_set_condition(struct ovsdb_idl *, >> const struct ovsdb_idl_condition *); >> >> unsigned int ovsdb_idl_get_condition_seqno(const struct ovsdb_idl *); >> +bool ovsdb_idl_monitor_condition_pending(struct ovsdb_idl *); > > I don't think that we need a new api for this. Condition sequence number, > I believe, exists for exactly this case. There is an issue, however, that > db bindings doesn't return result of ovsdb_idl_set_condition() throwing it > away without giving a chance for an idl user to actually use it. That could > be fixed by something like this: > > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > index 698fe25f3..d319adb03 100755 > --- a/ovsdb/ovsdb-idlc.in > +++ b/ovsdb/ovsdb-idlc.in > @@ -1416,10 +1416,10 @@ struct %(s)s * > print("\nstruct ovsdb_idl_column %s_columns[%s_N_COLUMNS];" % ( > structName, structName.upper())) > print(""" > -void > +unsigned int > %(s)s_set_condition(struct ovsdb_idl *idl, struct ovsdb_idl_condition > *condition) > { > - ovsdb_idl_set_condition(idl, &%(p)stable_%(tl)s, condition); > + return ovsdb_idl_set_condition(idl, &%(p)stable_%(tl)s, condition); > }""" % {'p': prefix, > 's': structName, > 'tl': tableName.lower()}) > --- > > I think, I had this patch somewhere already, but it seems that I never > actually send it.
Thanks for looking into this, Ilya! Yes, returning the seqno would probably be nice to have in *_set_condition(..). > > With this change ovn-controller will need to store the returned value > and wait until current condition seqno != expected to be sure that > condition was successfully set. > > What do you think? > I thought about this too before proposing the new API but it seemed to me that in that case the code that sets the conditions in ovn-controller might get unnecessarily complicated: https://github.com/ovn-org/ovn/blob/c108f23e1c10910031f9409b79001d001aae0c8f/controller/ovn-controller.c#L240 I guess it can be rewritten as something like this: expected_cond_seqno = 0; expected_cond_seqno = MAX(sbrec_port_binding_set_condition(ovnsb_idl, &pb), expected_cond_seqno); [...] expected_cond_seqno = MAX(sbrec_chassis_private_set_condition(ovnsb_idl, &chprv), expected_cond_seqno); [...] return expected_cond_seqno; I know it's not really OVS's problem but the set_condition() API doesn't seem to make it easy to write simple code to use it properly. However, if you think a new API would be redundant, we'll have to just find a way to properly explain (comments I guess) why we need to do things like this in ovn-controller. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
