Message ID | CAKFX60cywiwW9mvqtEA9UTP9tajC+V4GaGpOWxtRC4Tq-rZsVQ@mail.gmail.com |
---|---|
State | Superseded |
Headers | show |
On Sun, Dec 18, 2016 at 06:17:15PM -0800, Mickey Spiegel wrote: > On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <blp@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. > Acked-by: Mickey Spiegel <mickeys.dev@gmail.com> Thanks for the review! > > @@ -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. Thanks for noticing. I removed this condition. > > 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? You're right, we can add this check here too. Thanks, done. > 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. I have a fix for that in a later patch. I've now moved it to this one. Thanks for noticing.
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;