Re: [ovs-dev] [PATCH v3] Add monitor_cond_since support
On 12/1/21 17:55, Terry Wilson wrote: > On Fri, Nov 26, 2021 at 5:38 AM Dumitru Ceara wrote: >> Nit: To be consistent with the other ifs below we should probably add >> this comment: >> >> # Database contents changed. > > Will do. > Thanks. >>> +# 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? > > if not msg.result[0] is equivalent (mostly) to if msg.result[0] == > False, in which case we would call self.__clear(). > Ah, my bad, I has misread the initial code, sorry for the noise. >>> @@ -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() > > The poller lines here were wrong due to differences in how python-ovs > and the C code do polling. Instead, I'll add some code to Idl.wait() > to wake up if self.cond_changed. > Sounds good. >>> +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. > > Ah, good point. I'll fix this. > > Thanks for the review! > Terry > Thanks! Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] Add monitor_cond_since support
On Fri, Nov 26, 2021 at 5:38 AM Dumitru Ceara wrote: > Nit: To be consistent with the other ifs below we should probably add > this comment: > > # Database contents changed. Will do. > > +# 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? if not msg.result[0] is equivalent (mostly) to if msg.result[0] == False, in which case we would call self.__clear(). > > @@ -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() The poller lines here were wrong due to differences in how python-ovs and the C code do polling. Instead, I'll add some code to Idl.wait() to wake up if self.cond_changed. > > +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. Ah, good point. I'll fix this. Thanks for the review! Terry ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] Add monitor_cond_since support
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 > --- 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] Add monitor_cond_since support
Terry Wilson writes:
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 87ee06cde..c5d3ccdba 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
[...]
> @@ -45,6 +47,60 @@ Notice = collections.namedtuple('Notice', ('event', 'row',
> 'updates'))
> Notice.__new__.__defaults__ = (None,) # default updates=None
>
>
> +class Monitor(enum.IntEnum):
[...]
> +def init(self, cond):
> +"""Signal that a a condition change is being initiated"""
Typo: 'a a'
--
James
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] Add monitor_cond_since support
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
---
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
index 87ee06cde..c5d3ccdba 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -13,6 +13,7 @@
# limitations under the License.
import collections
+import enum
import functools
import uuid
@@ -36,6 +37,7 @@ ROW_DELETE = "delete"
OVSDB_UPDATE = 0
OVSDB_UPDATE2 = 1
+OVSDB_UPDATE3 = 2
CLUSTERED = "clustered"
RELAY = "relay"
@@ -45,6 +47,60 @@ Notice = collections.namedtuple('Notice', ('event', 'row',
'updates'))
Notice.__new__.__defaults__ = (None,) # default updates=None
+class Monitor(enum.IntEnum):
+monitor = OVSDB_UPDATE
+monitor_cond = OVSDB_UPDATE2
+monitor_cond_since = OVSDB_UPDATE3
+
+
+class ConditionState(object):
+def __init__(self):
+self._ack_cond = None
+self._req_cond = None
+self._new_cond = [True]
+
+def __iter__(self):
+return iter([self._new_cond, self._req_cond, self._ack_cond])
+
+@property
+def new(self):
+"""The latest freshly initialized condition change"""
+return self._new_cond
+
+@property
+def acked(self):
+"""The last condition change that has been accepted by the server"""
+return self._ack_cond
+
+@property
+def latest(self):
+"""The most recent condition change"""
+return next(cond for cond in self if cond is not None)
+
+@staticmethod
+def is_true(condition):
+return condition == [True]
+
+def init(self, cond):
+"""Signal that a a condition change is being initiated"""
+self._new_cond = cond
+
+def ack(self):
+"""Signal that a condition change has been acked"""
+if self._req_cond is not None:
+self._ack_cond, self._req_cond = (self._req_cond, None)
+
+def request(self):
+"""Signal that a condition change has been requested"""
+if self._new_cond is not None:
+self._req_cond, self._new_cond = (self._new_cond, None)
+
+def reset(self):
+"""Reset a requested condition change back to new"""
+if self._req_cond is not None and self._new_cond is None:
+self._new_cond, self._req_cond = (self._req_cond, None)
+
+
class Idl(object):
"""Open vSwitch Database Interface Definition Language (OVSDB IDL).
@@ -102,7 +158,13 @@ class Idl(object):
IDL_S_SERVER_MONITOR_REQUESTED = 2
IDL_S_DATA_MONITOR_REQUESTED = 3
IDL_S_DATA_MONITOR_COND_REQUESTED = 4
-IDL_S_MONITORING = 5
+IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED = 5
+IDL_S_MONITORING = 6
+
+monitor_map = {
+Monitor.monitor: IDL_S_SERVER_MONITOR_REQUESTED,
+Monitor.monitor_cond: IDL_S_DATA_MONITOR_COND_REQUESTED,
+Monitor.monitor_cond_since: IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED}
def __init__(self, remote, schema_helper, probe_interval=None,
leader_only=True):
@@ -146,10 +208,12 @@ class Idl(object):
remotes = self._parse_remotes(remote)
self._session = ovs.jsonrpc.Session.open_multiple(remotes,
probe_interval=probe_interval)
+self._request_id = None
self._monitor_request_id = None
self._last_seqno = None
self.change_seqno = 0
self.uuid = uuid.uuid1()
+self.last_id = str(uuid.UUID(int=0))
# Server monitor.
self._server_schema_request_id = None
@@ -176,6 +240,9 @@ class Idl(object):
self.txn = None
self._outstanding_txns = {}
+self.cond_changed = False
+self.cond_seqno = 0
+
for table in schema.tables.values():
for column in table.columns.values():
if not hasattr(column, 'alert'):
@@ -183,8 +250,7 @@ class Idl(object):
table.need_table = False
table.rows = custom_index.IndexedRows(table)
table.idl = self
-table.condition = [True]
-table.cond_changed = False
+table.condition = ConditionState()
def _parse_remotes(self, remote):
# If remote is -
@@ -222,6 +288,38 @@ class Idl(object):
update."""
self._session.close()
+def ack_conditions(self):
+"""Mark all requested table conditions as acked"""
+for table in self.tables.values():
+table.condition.ack()
+
+def sync_conditions(self):
+"""Synchronize condition state when the FSM is restarted
+
+If a non-zero last_id is available for the DB, then upon reconnect
+the IDL should first request acked conditions to avoid missing updates
+about records that were added before the transaction with
+t
