| Message ID | 1604920181-23904-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Add ovsdb_idl_monitor_condition_pending(). | expand |
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 <dceara@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. 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? Best regards, Ilya Maximets.
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 <dceara@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. Regards, Dumitru
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 <dceara@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.
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 <dceara@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? :) > 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? Thanks, Dumitru
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 <dceara@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? :) >> > > 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/20201110121811.2205350-1-i.maximets@ovn.org/ Best regards, Ilya Maximets.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index fdb9d85..8c0f86c 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -188,6 +188,17 @@ enum ovsdb_idl_monitoring { outstanding. */ }; +enum ovsdb_idl_monitor_cond_state { + OVSDB_IDL_MONITOR_COND_ACKED, /* Local conditional monitoring clauses + * have been acked by the server. */ + OVSDB_IDL_MONITOR_COND_LOCAL, /* Local conditional monitoring clause + * changes have not yet been sent to the + * server. */ + OVSDB_IDL_MONITOR_COND_REQUESTED, /* Local conditional monitoring clause + * changes have been sent to the server + * but have not yet been acked. */ +}; + struct ovsdb_idl_db { struct ovsdb_idl *idl; @@ -203,8 +214,8 @@ struct ovsdb_idl_db { struct json *schema; enum ovsdb_idl_monitoring monitoring; - /* True if any of the tables' monitoring conditions has changed. */ - bool cond_changed; + /* Current state of the conditional monitoring clauses. */ + enum ovsdb_idl_monitor_cond_state cond_state; unsigned int cond_seqno; /* Keep track of condition clauses changes over a single conditional monitoring session. @@ -1580,7 +1591,7 @@ ovsdb_idl_db_set_condition(struct ovsdb_idl_db *db, if (!ovsdb_idl_condition_equals(condition, table_cond)) { ovsdb_idl_condition_clone(&table->new_cond, condition); - db->cond_changed = true; + db->cond_state = OVSDB_IDL_MONITOR_COND_LOCAL; poll_immediate_wake(); return seqno + 1; } @@ -1638,7 +1649,7 @@ ovsdb_idl_create_cond_change_req(const struct ovsdb_idl_condition *cond) static struct jsonrpc_msg * ovsdb_idl_db_compose_cond_change(struct ovsdb_idl_db *db) { - if (!db->cond_changed) { + if (db->cond_state != OVSDB_IDL_MONITOR_COND_LOCAL) { return NULL; } @@ -1672,7 +1683,7 @@ ovsdb_idl_db_compose_cond_change(struct ovsdb_idl_db *db) return NULL; } - db->cond_changed = false; + db->cond_state = OVSDB_IDL_MONITOR_COND_REQUESTED; struct json *params = json_array_create_3(json_clone(db->monitor_id), json_clone(db->monitor_id), monitor_cond_change_requests); @@ -1693,6 +1704,19 @@ ovsdb_idl_db_ack_condition(struct ovsdb_idl_db *db) ovsdb_idl_condition_move(&table->ack_cond, &table->req_cond); } } + + /* Ack the last monitor condition change if no local changes happened in + * the meantime. + */ + if (db->cond_state == OVSDB_IDL_MONITOR_COND_REQUESTED) { + db->cond_state = OVSDB_IDL_MONITOR_COND_ACKED; + } +} + +bool +ovsdb_idl_monitor_condition_pending(struct ovsdb_idl *idl) +{ + return idl->data.cond_state != OVSDB_IDL_MONITOR_COND_ACKED; } /* Should be called when the IDL fsm is restarted and resyncs table conditions @@ -1712,7 +1736,7 @@ ovsdb_idl_db_sync_condition(struct ovsdb_idl_db *db) { bool ack_all = uuid_is_zero(&db->last_id); - db->cond_changed = false; + db->cond_state = OVSDB_IDL_MONITOR_COND_ACKED; for (size_t i = 0; i < db->class_->n_tables; i++) { struct ovsdb_idl_table *table = &db->tables[i]; @@ -1731,7 +1755,7 @@ ovsdb_idl_db_sync_condition(struct ovsdb_idl_db *db) } else { /* If there was no "unsent" condition but instead a * monitor_cond_change request was in flight, move table->req_cond - * to table->new_cond and set db->cond_changed to trigger a new + * to table->new_cond and set db->cond_state to trigger a new * monitor_cond_change request. * * However, if a new condition has been set by the IDL client, @@ -1740,7 +1764,7 @@ ovsdb_idl_db_sync_condition(struct ovsdb_idl_db *db) */ if (table->req_cond && !table->new_cond) { ovsdb_idl_condition_move(&table->new_cond, &table->req_cond); - db->cond_changed = true; + db->cond_state = OVSDB_IDL_MONITOR_COND_LOCAL; } } } 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 *); /* Indexes over one or more columns in the IDL, to retrieve rows matching * particular search criteria and to iterate over a subset of rows in a defined
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 <dceara@redhat.com> --- lib/ovsdb-idl.c | 40 ++++++++++++++++++++++++++++++++-------- lib/ovsdb-idl.h | 1 + 2 files changed, 33 insertions(+), 8 deletions(-)