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 <[email protected]> > ---
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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
