[ovs-dev] [PATCH v3] Add monitor_cond_since support

Dumitru Ceara dceara at redhat.com
Fri Nov 26 11:38:04 UTC 2021


On 11/21/21 00:12, Terry Wilson wrote:
> Add support for monitor_cond_since / update3 to python-ovs to
> allow more efficient reconnections when connecting to clustered
> OVSDB servers.
> 
> Signed-off-by: Terry Wilson <twilson at redhat.com>
> ---

Hi Terry,

Overall the changes look ok to me.  I just have a couple of comments below.

>  python/ovs/db/idl.py | 231 ++++++++++++++++++++++++++++++++++++-------
>  tests/ovsdb-idl.at   |   2 +-
>  2 files changed, 197 insertions(+), 36 deletions(-)
> 
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py

[...]

> @@ -264,8 +362,19 @@ class Idl(object):
>              msg = self._session.recv()
>              if msg is None:
>                  break
> +            is_response = msg.type in (ovs.jsonrpc.Message.T_REPLY,
> +                                       ovs.jsonrpc.Message.T_ERROR)
> +
> +            if is_response and self._request_id and self._request_id == msg.id:
> +                self._request_id = None
> +                # process_response follows
>  
>              if (msg.type == ovs.jsonrpc.Message.T_NOTIFY
> +                    and msg.method == "update3"
> +                    and len(msg.params) == 3):

Nit: To be consistent with the other ifs below we should probably add
this comment:

# Database contents changed.

> +                self.__parse_update(msg.params[2], OVSDB_UPDATE3)
> +                self.last_id = msg.params[1]
> +            elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
>                      and msg.method == "update2"
>                      and len(msg.params) == 2):
>                  # Database contents changed.
> @@ -290,11 +399,18 @@ class Idl(object):
>                  try:
>                      self.change_seqno += 1
>                      self._monitor_request_id = None
> -                    self.__clear()
> -                    if self.state == self.IDL_S_DATA_MONITOR_COND_REQUESTED:
> +                    if (self.state ==
> +                            self.IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED):
> +                        # If 'found' is false, clear table rows for new dump
> +                        if not msg.result[0]:
> +                            self.__clear()

Can msg.result[0] be 'false'?  I think so and we should call __clear()
then and I think we won't; or am I reading this wrong?

> +                        self.__parse_update(msg.result[2], OVSDB_UPDATE3)
> +                    elif self.state == self.IDL_S_DATA_MONITOR_COND_REQUESTED:
> +                        self.__clear()
>                          self.__parse_update(msg.result, OVSDB_UPDATE2)
>                      else:
>                          assert self.state == self.IDL_S_DATA_MONITOR_REQUESTED
> +                        self.__clear()
>                          self.__parse_update(msg.result, OVSDB_UPDATE)
>                      self.state = self.IDL_S_MONITORING
>  

[...]

> @@ -420,9 +571,16 @@ class Idl(object):
>  
>          if cond == []:
>              cond = [False]
> -        if table.condition != cond:
> -            table.condition = cond
> -            table.cond_changed = True
> +
> +        # Compare the new condition to the last known condition
> +        if table.condition.latest != cond:
> +            table.condition.init(cond)
> +            self.cond_changed = True
> +            p = ovs.poller.Poller()
> +            p.immediate_wake()
> +            return self.cond_seqno + 1

I'm not sure this is correct.  In the C ovsdb-cs module we do:

https://github.com/openvswitch/ovs/blob/7b8aeadd60c82f99a9d791a7df7c617254654f9d/lib/ovsdb-cs.c#L947

Which translates to expecting "self.cond_seqno + 2" if there's at least
one condition change request in flight for one of the monitor tables.

If we don't do this the application will not have a reliable way of
determining when the IDL is up to date with the most recent requested
conditions.  E.g., in ovn-controller we compare the current seqno with
the result returned by ovsdb_idl_set_condition():

https://github.com/ovn-org/ovn/blob/993d7113a61f4b5f34de727451003cc1a1a67e63/controller/ovn-controller.c#L274

And act based on the current cond_seqno:

https://github.com/ovn-org/ovn/blob/993d7113a61f4b5f34de727451003cc1a1a67e63/controller/ovn-controller.c#L827

Thanks,
Dumitru



More information about the dev mailing list