[ovs-dev] [PATCH] ovsdb-idl: Add ovsdb_idl_monitor_condition_pending().

[Ilya Maximets]

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 at 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.