[ovs-dev] [RFC 00/14] ovn-controller incremental processing.

[Mark Michelson]

Hi Han,

I've given this patchset a look, and I was following along pretty well 
until I got to about patch 11. From that point on, I had to re-read the 
code more times than I care to admit before I finally understood what 
was going on :)

What you have is a structure (lflow_ref_list_node) that is 
simultaneously in two lists. These two lists are each the data in 
separate hmap nodes. The hmap nodes are in two separate hmaps. One hmap 
uses the reference type and name as a key, and the other uses the lflow 
UUID as a key. This way given an address set name, you can find the 
associated logical flow UUIDs in the ref_lflow_table. Or given a logical 
flow UUID, you can find address sets.

I'm wondering if this can be simplified somehow.

Right now, if logical flows change, the change handler has to update the 
ref_lflow_table so that address sets no longer reference that logical 
flow. If address sets change, then the lflow_ref_table is updated. In 
both cases consider_logical_flow() gets called and realigns the tables 
as appropriate.

The problem with this is that it reeks of cross-cutting concerns, and it 
seems like it wouldn't scale well (consider a 3- or 4-chain of 
dependencies). Ideally, the dependency chain would make sure that the 
change handler for logical flows only deals with logical flows, and the 
change handler for address sets only deals with address sets.

If we generalize things a bit, there are likely to be two ways 
dependencies manifest in the database. In this particular case, text in 
one row expands to data of a separate database row. The other case would 
be where a database row contains the UUID (or list of UUIDs) of other 
database rows.

For the textual case, I think the easiest way to handle this is to 
replace the text with what it expands to earlier than when we currently 
do it. Consider that a logical flow references address set $foo. 
Currently, the logical flow in the southbound database has the text 
"$foo" in it. If $foo were replaced with the actual addresses from the 
address set, then when an address set changes, the text of the logical 
flow would change as well, thus resulting in a direct change of the 
logical flow. A less disruptive version of this might be to use some 
reserved character automatically in the logical flow match followed by a 
sequence number. So for instance, if a logical flow were set up to 
reference address set $foo, then the actual logical flow might be 
something like $foo?1. Then if northbound address set foo changes, the 
logical flow could be updated to $foo?2 by ovn-northd. Again, the 
textual change in the logical flow would result in triggering the change 
tracker.

For the database referencing case, it would be nice if the IDL change 
tracking code could automatically do this for us. This way if record foo 
has a column that references row bar, then if bar changes, we would be 
told that foo also changed. This strikes me as difficult to implement 
and could result in some interesting dependency graphs within the IDL 
code though.

What do you think?

On 07/24/2018 08:59 PM, Han Zhou wrote:
> This patch series is the rebase of previous patch series [1] on top of master
> where a series of changes that avoided passing ovsdb IDL (eaa4ead5) has been
> merged. The major concern of the previous patches are the maintainability
> of the dependencies. This patch series, thanks for the removal of passing
> IDL directly, eliminates the access to any tables within any engine node
> processing, and all dependencies are naturally exposed because otherwise
> the code will either not pass compile or abort in the very first iteration,
> except for one case - accessing data of other tables through references.
> 
> For exposing the dependencies introduced by reference access, it is a big
> TODO item and it is the major reason this patch series is RFC only.
> 
> Other than this, there is another problem found while using ovsdb IDL index
> to query chassis: current ovsdb IDL index is not updated for the changes made
> in current transaction. It would be better if we fix the index implementation,
> although this patch series worked around this problem by postpone some
> processing to next iterations in such circumstances.
> 
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/347808.html
> 
> Han Zhou (14):
>    ovsdb-idlc.in: Support more interfaces for passing pointers of
>      individual tables.
>    ovn-controller: Incremental processing engine
>    ovn-controller: Track OVSDB changes
>    ovn-controller: Initial use of incremental engine.
>    ovn-controller: Incremental logical flow processing
>    ovn-controller: runtime_data change handler for SB port-binding
>    ovn-controller: port-binding incremental processing for physical flows
>    ovn-controller: incremental processing for multicast group changes
>    ovsdb-idl: Tracking - preserve data for deleted rows.
>    ovn-controller: Split addr_sets from runtime_data.
>    ovn-controller: Maintain resource references for logical flows.
>    ovn-controller: Incremental processing for address-set changes.
>    ovn-controller: Split port_groups from runtime_data.
>    ovn-controller: Incremental processing for port-group changes.
> 
>   include/ovn/actions.h           |    3 +
>   include/ovn/expr.h              |    5 +-
>   lib/ovsdb-idl-provider.h        |    2 +
>   lib/ovsdb-idl.c                 |   36 +-
>   ovn/controller/bfd.c            |    4 +-
>   ovn/controller/binding.c        |  108 ++-
>   ovn/controller/binding.h        |    7 +
>   ovn/controller/encaps.c         |   12 +-
>   ovn/controller/lflow.c          |  428 +++++++++++-
>   ovn/controller/lflow.h          |  100 ++-
>   ovn/controller/ofctrl.c         |  262 +++++---
>   ovn/controller/ofctrl.h         |   32 +-
>   ovn/controller/ovn-controller.c | 1398 +++++++++++++++++++++++++++++++++------
>   ovn/controller/physical.c       |  232 +++++--
>   ovn/controller/physical.h       |   20 +-
>   ovn/lib/actions.c               |    8 +-
>   ovn/lib/automake.mk             |    4 +-
>   ovn/lib/expr.c                  |   21 +-
>   ovn/lib/extend-table.c          |   60 +-
>   ovn/lib/extend-table.h          |   16 +-
>   ovn/lib/inc-proc-eng.c          |  201 ++++++
>   ovn/lib/inc-proc-eng.h          |  240 +++++++
>   ovn/utilities/ovn-trace.c       |    2 +-
>   ovsdb/ovsdb-idlc.in             |   25 +
>   tests/ovn.at                    |   75 +++
>   tests/test-ovn.c                |    7 +-
>   26 files changed, 2877 insertions(+), 431 deletions(-)
>   create mode 100644 ovn/lib/inc-proc-eng.c
>   create mode 100644 ovn/lib/inc-proc-eng.h
>