[ovs-dev] [RFC 00/14] ovn-controller incremental processing.
mmichels at redhat.com
Tue Jul 31 13:47:48 UTC 2018
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
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
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
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
What do you think?
On 07/24/2018 08:59 PM, Han Zhou wrote:
> This patch series is the rebase of previous patch series  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.
>  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
More information about the dev