On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <b...@ovn.org> wrote: > On a particular hypervisor, ovn-controller only needs to handle ports > and datapaths that have some relationship with it, that is, the > ports that actually reside on the hypervisor, plus all the other ports on > those ports' datapaths, plus all of the ports and datapaths that are > reachable from those via logical patch ports. Until now, ovn-controller > has done a poor job of limiting what it deals with to this set. This > commit improves the situation. > > This commit gets rid of the concept of a "patched_datapath" which until now > was used to represent any datapath that contained a logical patch port. > Previously, the concept of a "local_datapath" meant a datapath with a VIF > that resides on the local hypervisor. This commit extends that concept to > include any other datapath that can be reached from a VIF on the local > hypervisor, which is a simplification that makes the code easier to > understand in a few places. > > CC: Gurucharan Shetty <g...@ovn.org> > Signed-off-by: Ben Pfaff <b...@ovn.org> >
Acked-by: Mickey Spiegel <mickeys....@gmail.com> A couple of comments below. --- > ovn/controller/binding.c | 75 ++++++++++++++++++++++++++----- > ovn/controller/binding.h | 7 ++- > ovn/controller/lflow.c | 46 ++----------------- > ovn/controller/lflow.h | 1 - > ovn/controller/ovn-controller.c | 49 ++++++--------------- > ovn/controller/ovn-controller.h | 33 +++++++------- > ovn/controller/patch.c | 98 ++++++++++++++++-------------- > ----------- > ovn/controller/patch.h | 5 +-- > ovn/controller/physical.c | 30 ++----------- > ovn/controller/physical.h | 4 +- > tests/ovn-controller.at | 19 +++++++- > 11 files changed, 165 insertions(+), 202 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index fb76032..2759e23 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > <snip> @@ -293,7 +339,8 @@ consider_local_datapath(struct controller_ctx *ctx, > /* Add child logical port to the set of all local ports. */ > sset_add(all_lports, binding_rec->logical_port); > } > - add_local_datapath(local_datapaths, binding_rec); > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + false, local_datapaths); > if (iface_rec && qos_map && ctx->ovs_idl_txn) { > get_qos_params(binding_rec, qos_map); > } > @@ -328,7 +375,8 @@ consider_local_datapath(struct controller_ctx *ctx, > } > > sset_add(all_lports, binding_rec->logical_port); > - add_local_datapath(local_datapaths, binding_rec); > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + false, local_datapaths); > if (binding_rec->chassis == chassis_rec) { > return; > } > @@ -342,7 +390,8 @@ consider_local_datapath(struct controller_ctx *ctx, > const char *chassis = smap_get(&binding_rec->options, > "l3gateway-chassis"); > if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) { > Not sure if you want to cover it in this patch, but since you are making changes to add_local_datapath, I thought I would mention it. When reviewing the datapaths of interest patch, Darrell and I stumbled upon the realization that there is no reason for "&& ctx->ovnsb_idl_txn" above to be part of the condition. It is not used in the call to add_local_datapath. Similar code above for vifs and for l2gateways has no such condition on ctx->ovnsb_idl_txn. > - add_local_datapath(local_datapaths, binding_rec); > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + true, local_datapaths); > } > } else if (chassis_rec && binding_rec->chassis == chassis_rec) { > if (ctx->ovnsb_idl_txn) { > <snip> > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -157,36 +157,13 @@ static void > consider_port_binding(enum mf_field_id mff_ovn_geneve, > const struct simap *ct_zones, > struct hmap *local_datapaths, > - struct hmap *patched_datapaths, > const struct sbrec_port_binding *binding, > struct ofpbuf *ofpacts_p, > struct hmap *flow_table) > { > - /* Skip the port binding if the port is on a datapath that is neither > - * local nor with any logical patch port connected, because local > ports > - * would never need to talk to those ports. > - * > - * Even with this approach there could still be unnecessary port > - * bindings processed. A better approach would be a kind of "flood > - * fill" algorithm: > - * > - * 1. Initialize set S to the logical datapaths that have a port > - * located on the hypervisor. > - * > - * 2. For each patch port P in a logical datapath in S, add the > - * logical datapath of the remote end of P to S. Iterate > - * until S reaches a fixed point. > - * > - * This can be implemented in northd, which can generate the sets and > - * save it on each port-binding record in SB, and ovn-controller can > - * use the information directly. However, there can be update storms > - * when a pair of patch ports are added/removed to connect/disconnect > - * large lrouters and lswitches. This need to be studied further. > - */ > uint32_t dp_key = binding->datapath->tunnel_key; > uint32_t port_key = binding->tunnel_key; > - if (!get_local_datapath(local_datapaths, dp_key) > - && !get_patched_datapath(patched_datapaths, dp_key)) { > + if (!get_local_datapath(local_datapaths, dp_key)) { > return; > } I wonder why there is no similar check on consider_mc_group. Is there any reason for mc_group related physical flows to be programmed for a datapath if there are no port related physical flows or logical flows programmed for that datapath? I tried the following diff: diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 41673e5..6bca413 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -493,6 +493,11 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, struct ofpbuf *remote_ofpacts_p, struct hmap *flow_table) { + uint32_t dp_key = mc->datapath->tunnel_key; + if (!get_local_datapath(local_datapaths, dp_key)) { + return; + } + struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis); struct match match; It passed all automated tests except for one that seems to be written too narrowly at line 5408 in tests/ovn.at. The test passed when I commented out that line. Mickey _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev