[ovs-dev] [PATCH v2] [python] Add monitor_cond_since support

[Ilya Maximets]

On 11/4/21 14:07, Terry Wilson wrote:
> 
> 
> On Wed, Nov 3, 2021, 11:22 AM Ilya Maximets <i.maximets at ovn.org <mailto:i.maximets at ovn.org>> wrote:
> 
>     On 11/3/21 01:36, 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 <mailto:twilson at redhat.com>>
>     > ---
>     >  python/ovs/db/idl.py | 55 +++++++++++++++++++++++++++++++++++---------
>     >  1 file changed, 44 insertions(+), 11 deletions(-)
>     >
> 
>     Hi, Terry.  Thanks for working on this!
> 
> 
> Thank you so much for the review!
> 
>     This patch is missing some unit tests.  At least, you need to enable
>     tests that we have for C implementation, so they will run with the
>     python implementation too.  E.g:
> 
>     diff --git a/tests/ovsdb-idl.at <http://ovsdb-idl.at> b/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>     index 0f229b2f9..1de7f4e67 100644
>     --- a/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>     +++ b/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>     @@ -2293,7 +2293,7 @@ m4_define([OVSDB_CHECK_CLUSTER_IDL],
> 
>      # Checks that monitor_cond_since works fine when disconnects happen
>      # with cond_change requests in flight (i.e., IDL is properly updated).
>     -OVSDB_CHECK_CLUSTER_IDL_C([simple idl, monitor_cond_since, cluster disconnect],
>     +OVSDB_CHECK_CLUSTER_IDL([simple idl, monitor_cond_since, cluster disconnect],
>        3,
>        [['["idltest",
>             {"op": "insert",
>     ---
> 
>     I didn't check, but I guess that this test will actually fail for
>     python idl.  The reason is that I don't see any extra handling of
>     monitor conditions in the patch.  The sequence of events that test
>     above is testing is following:
> 
> 
> That test does fail. I'll work on adding the support for handling interrupted cond_change to the patch. It's a bit more of a pain without the clearly defined state machine of ovsdb-cs, but I'm close to getting it finished.
> 
> It's increasing my resolve to do a refactor of idl.py to be a bit more ovsdb-cs-ish and a bit more Pythonic in places. Is this something the community would like to see and would have time to review?

That would be great!  In general, I think, the idl/cs split makes a lot
of sense for the C code and it would be good to have similar logical
split in the python code as well.  As we generally need to develop/fix
the same things for both C and Python versions of a code, it would
be good to have them similar.  That said, "a bit more Pythonic" should
be a good thing.  OTOH, if it's a complete re-write that will make Python
code very different from C, that might become a problem for the maintenance.

For me it's a bit hard to review python code as I'm not working with it
on a regular basis.  Especially some "more Pythonic" stuff. :)
But I'll try to find some time for review once this change will hit the
mailing list.  Maybe we'll be able to get someone else's attention too.

Best regards, Ilya Maximets.

> 
> Terry
> 
>     1. create a monitor
>     2. set conditions
>     3. change conditions and disconnect before receiving reply from
>        a server.
>     4. re-connect.
> 
>     In order to have correctly updated information with monitor_cond_since,
>     in this scenario, on re-connect, idl should send 'old' conditions first
>     in order to receive all the current updates and then send 'new' conditions
>     in order to receive data for condition update.
> 
>     If the 'new' conditions will be sent right away, idl will never receive
>     data that matches 'new' conditions, but doesn't match 'old' ones for
>     transactions that happened before the 'last_id'.
> 
>     See the corresponding C code here:
>       https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-cs.c#L1058 <https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-cs.c#L1058>
> 
>     Best regards, Ilya Maximets.
>