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

[Dumitru Ceara]

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 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? :)
> 

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