On 11/10/20 11:31 AM, Dumitru Ceara wrote: > 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 <dce...@redhat.com> >>> --- >>> 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.
Maybe a little pinch of magic will help? :) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index a06cae3cc..8bc331e95 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -236,16 +236,24 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, } } -out: - sbrec_port_binding_set_condition(ovnsb_idl, &pb); - sbrec_logical_flow_set_condition(ovnsb_idl, &lf); - sbrec_mac_binding_set_condition(ovnsb_idl, &mb); - sbrec_multicast_group_set_condition(ovnsb_idl, &mg); - sbrec_dns_set_condition(ovnsb_idl, &dns); - sbrec_controller_event_set_condition(ovnsb_idl, &ce); - sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast); - sbrec_igmp_group_set_condition(ovnsb_idl, &igmp); - sbrec_chassis_private_set_condition(ovnsb_idl, &chprv); +out:; + unsigned int cond_seqnos[] = { + sbrec_port_binding_set_condition(ovnsb_idl, &pb), + sbrec_logical_flow_set_condition(ovnsb_idl, &lf), + sbrec_mac_binding_set_condition(ovnsb_idl, &mb), + sbrec_multicast_group_set_condition(ovnsb_idl, &mg), + sbrec_dns_set_condition(ovnsb_idl, &dns), + sbrec_controller_event_set_condition(ovnsb_idl, &ce), + sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast), + sbrec_igmp_group_set_condition(ovnsb_idl, &igmp), + }; + /* We'll need to wait for a maximum expected sequence number to be sure + * that all conditions accepted by the server. */ + unsigned int expected_cond_seqno = 0; + for (size_t i = 0; i < ARRAY_SIZE(cond_seqnos); i++) { + expected_cond_seqno = MAX(expected_cond_seqno, cond_seqnos[i]); + } + ovsdb_idl_condition_destroy(&pb); ovsdb_idl_condition_destroy(&lf); ovsdb_idl_condition_destroy(&mb); @@ -255,6 +263,8 @@ out: ovsdb_idl_condition_destroy(&ip_mcast); ovsdb_idl_condition_destroy(&igmp); ovsdb_idl_condition_destroy(&chprv); + + return expected_cond_seqno; } static const char * --- Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev