On Sun, Jul 03, 2016 at 10:35:27AM -0500, Ryan Moats wrote:
> Ensure that ovn flow tables are persisted so that changes to
> them chan be applied incrementally - this is a prereq for
> making lflow_run and physical_run incremental.
>
> Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
This time I think I've figured out properly what I'm concerned about.
Before, I think I was distracted enough by 'is_new' and the remaining
patches that I hadn't formulated it correctly yet.
ofctrl needs to support the following operations on physical flows:
1. Add flow.
2. Update flow.
3. Remove flow.
lflow generates physical flows from logical flows in a one-to-many
fashion. That is, a single logical flow can yield zero, one, or
multiple physical flows. Other sources can yield physical flows too,
and ofctrl needs those sources to pretend that they are generated from
something similar enough to a logical flow that it can be uniquely
identified with a UUID. All that makes sense.
Case #1, "add flow", for physical flows is implemented via
ofctrl_add_flow() with is_new == true. This is straightforward enough.
Add the new flow to the flow table, add it to the list of physical flows
associated with the UUID passed in, suppress duplicates.
Case #3, "remove flow", for physical flows is implemented via
ofctrl_remove_flow(). This removes all the physical flows associated
with a UUID. This is almost as straightforward. The implementation
here does appear to mean that, if there is a bug in the logical flow
table such that two different logical flows generate physical flows with
the same match, then there is a behavioral change versus the previous
implementation: previously, one of the flows to be added in the main
loop would end up in the flow table, but after this change the removal
will not reveal an underlying flow but just delete it, at least until
something happens to fully refresh flows instead of just incrementally
adding and removing them. That is an edge case; it might be necessary
to fix it but it is not the top priority.
Case #2, "update flow", is implemented via ofctrl_add_flow() with is_new
== false. This is the one that I'm worried about and where I do not
understand the strategy. What I'd expect is that the client would be
able to say, one way or another, "Here is the new set of physical flows
F2 associated with uuid U." If some of the flows in F2 coincide with
the set of physical flows F1 that had previously been associated with U,
then it would make sense that nothing actually changes in the physical
flow table. But there are many more possibilities. For example, if F1
contains more flows than F2, then there needs to be a way to indicate
that some of the physical flows associated with U should be removed.
There is code in ovn_flow_lookup_by_uuid() that tries to tolerate some
kind of changes to flow matches from F1 to F2 (match fields that appear
or disappear), but I don't know a reason to believe that only those
changes can happen from F1 to F2; even after explanation, they look like
magic to me.
Maybe there is a belief here that a given Logical_Flow has some kind of
consistency, e.g. that its match and action, etc. do not change after
the logical flow is added. That might be a useful assumption, and we
could enforce it if we made some (all?) Logical_Flow columns immutable.
But it is not currently guaranteed to be true: I can use ovn-sbctl (or
whatever) to modify all of the columns in a Logical_Flow row in
arbitrary ways. This means that F1 and F2 might also have nothing in
common for a given Logical_Flow U.
Now let's go back to the edge case concern I expressed about case #3.
Thinking harder, I don't think it's so much of an edge case, at least
not to the extent that exhibiting it requires a buggy logical flow
table. Suppose that a single transaction from ovn-northd deletes a
Logical_Flow with uuid U1 and adds a new Logical_Flow with uuid U2.
This will yield a call to ofctrl_remove_flow(U1) and (except in
pathological cases) one or more calls to ofctrl_add_flow(U2). Suppose
that the sets of physical flows for U1 and U2 overlap in terms of their
key fields (table_id, pipeline, match, ...). Then I believe that the
result will currently depend on the order of the calls to
ofctrl_remove_flow() and ofctrl_add_flow():
* If the removal precedes all the adds, all is well.
* If the removal follows any of the adds, the remove will
falsely delete all the flows that should be added.
Does any of this ring a bell?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
