[oops, adding back the list]
On Fri, May 05, 2017 at 08:51:01AM -0700, Ben Pfaff wrote: > On Fri, May 05, 2017 at 02:58:45PM +0200, Daniel Alvarez Sanchez wrote: > > Thanks a lot Ben for taking the time to review the patch and submit > > the 3 patch series. > > > > On Wed, May 3, 2017 at 11:54 PM, Ben Pfaff <[email protected]> wrote: > > > > > On Tue, Apr 25, 2017 at 11:05:28AM +0000, Daniel Alvarez wrote: > > > > This patch introduces a new type of OVN ports called "localport". > > > > These ports will be present in every hypervisor and may have the > > > > same IP/MAC addresses. They are not bound to any chassis and traffic > > > > to these ports will never go through a tunnel. > > > > > > > > Its main use case is the OpenStack metadata API support which relies > > > > on a local agent running on every hypervisor and serving metadata to > > > > VM's locally. This service is described in detail at [0]. > > > > > > > > Signed-off-by: Daniel Alvarez <[email protected]> > > > > > > In consider_local_datapath(), the ovn-localport-port logic only fires if > > > ovn-localport-port is completely unset. Usually, it's better to make > > > sure that everything that ovn-controller sets happens every time, > > > because this fixes up damage if any occurs. So, for example, in this > > > case, we would tend to always set ovn-localport-port. (Sometimes, this > > > can be expensive, and so we come up with ways to avoid it, but I don't > > > have a reason to believe that it is expensive in this case.) > > > > > Ack. However, now that I've made previous change (only setting it on > > the Interface, I think we can keep it like this because that > > "ovn-localport-port logic" is simply setting the external-id if > > unset. Or you would just prefer to set it anyways? There's no more > > logic apart from this now that I removed setting it also in the Port > > table. > > I'm not sure I understand. I'll see when I read the next version, no > problem. > > > > The documentation for external-ids:ovn-localport-port should say what > > > the key's value is. > > > > > > Actually, the purpose of external-ids:ovn-localport-port is not entirely > > > clear to me. The other documented keys in this category have the > > > following uses: > > > > > > external-ids:ovn-localnet-port > > > external-ids:ovn-l2gateway-port > > > external-ids:ovn-l3gateway-port > > > > > > ovn-controller creates these ports itself and it adds these > > > keys to them to indicate that it owns them; otherwise that > > > would be ambiguous and therefore it would be risky for > > > ovn-controller to later delete them. > > > > > > external-ids:ovn-logical-patch-port > > > > > > ovn-controller doesn't use these anymore; the documentation > > > is obsolete and should be removed. > > > > > > But it looks to me that ovn-controller doesn't create this new kind of > > > port (and doesn't delete it either). It seems that, so far, > > > ovn-controller only uses this key to internally communicate with two > > > pieces of itself, which doesn't seem like a great approach to me. > > > > > I see your point. Right now, I'm setting this key in binding.c so that, > > when in physical.c we're iterating over ports to set the flows, I can > > have the corresponding ofport for every localport and, for each, > > output flow insert a drop if the traffic comes from a localport and > > tries to go through a tunnel. > > > > Other thing I could do at consider_port_binding in physical.c at [0] is: > > > > 1. SBREC_PORT_BINDING_FOR_EACH(entry): > > 2. Insert a drop flow if entry->type == "localport" matching > > by logical inport > > > > What do you think about this? I think current implementation is more > > efficient > > since we don't have to iterate over all sbrec_port_binding entries every > > time > > for every flow we want to insert in table 32 but we'd get rid of the > > ovn-localport-port external_id. > > Usually I'd suggest a shared data structure. The "binding" code already > constructs data structures to keep track of logical ports, for example. > > > > Also, I noticed that this patch re-set physical_map_changed based on > > > update_ofports(), although it should have honored an existing 'true' > > > value there. > > > > > > > I might be missing something but what I just wanted to set > > physical_map_changed to true if an update on 'localport_to_ofport' > > happened. Otherwise, leave it as it was before. What am I missing? > > > > /* Capture changed or removed openflow ports. */ > > physical_map_changed |= update_ofports(&localvif_to_ofport, > > > > &new_localvif_to_ofport); > > physical_map_changed |= update_ofports(&localport_to_ofport, > > > > &new_localport_to_ofport); > > Sure, that would be fine, but the patch actually used = instead of |=: > + physical_map_changed = simap_sync(&localvif_to_ofport, > + &new_localvif_to_ofport); > + /* Do the same for all localports. */ > + physical_map_changed |= simap_sync(&localport_to_ofport, > + &new_localport_to_ofport); > > > Finally, this patch reuses some code from physical_run() that was more > > > complicated than necessary. Not your fault--it was natural to reuse > > > it--but I decided to rewrite and simplify it. So, I sent this out as a > > > 3-patch series with that simplification as the first 2 patches. Would > > > you mind picking it up from that series? It's posted starting here: > > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/331950.html > > > > > > Thanks!! I picked those and submitted v2 :) > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332094.html > > > > > > Thanks again! > > > > > > > Thanks Ben for your great review. > > You're welcome, I'll try a look at v2 now. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
