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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
