On 11/10/20 12:59 PM, Dumitru Ceara wrote: > On 11/10/20 12:50 PM, Ilya Maximets wrote: >> 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 <[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. >> >> Maybe a little pinch of magic will help? :) >> > > I think it does. :) > >> 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:; > > I'll see if I can get rid of this label all together. > >> + 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 * > > Will you be sending a patch for ovsdb-idlc.in to expose the expected seqno?
Sent: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
