Thanks. This is a good summary of some of what the patch does. Should it be in the commit message?
On Tue, May 09, 2017 at 05:09:43PM +0200, Daniel Alvarez Sanchez wrote: > Hi, > > I've submitted a new patch v3 where I removed the external-id > "ovn-localport" > from the Interface which I used to identify a port as localport in > physical.c. > > Instead, I have passed another parameter "local_lports" to physical_run. > When > inserting flows in table 32, I'm inserting higher priority drop flows for > every > local localport. > > In order to illustrate this: > > LS1 switch1 with 3 ports: 2 ports and 1 localport > $ ovn-nbctl show switch1 > switch c2a81271-b77f-4019-b877-6428c8b647d7 (switch1) > port p2 > addresses: ["00:00:00:01:01:11 20.0.0.11"] > port lp1 > type: localport > addresses: ["00:00:00:01:01:05 20.0.0.5"] > port p1 > addresses: ["00:00:00:01:01:10 20.0.0.10"] > > Two HVs: hv1 and hv2. > > p1 (tunnel_key = 2) is in hv1 > p2 (tunnel_key = 4) is in hv2 > lp1 (tunnel_key = 1) is in both hv1 and hv2 > > (When i say that a port is in a certain hv i'm saying that there's an OVS > port > with external-id:iface-id=<port>") > > Table 32 in hv2 looks like: > > cookie=0x0, duration=2077.204s, table=32, n_packets=2, n_bytes=196, > idle_age=114, priority=150,reg14=0x1,reg15=0x2,metadata=0x5 actions=drop > cookie=0x0, duration=2077.214s, table=32, n_packets=0, n_bytes=0, > idle_age=2077, priority=100,reg15=0x2,metadata=0x5 > actions=load:0x5->NXM_NX_TUN_ID[0..23],set_field:0x2->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:34 > > Which means that if a packet is directed to p1 (reg15=0x2) send it through > the tunnel unless it comes from lp1 (reg14=0x1) when it'll be dropped. > > > Table 32 in hv1 looks like: > > cookie=0x0, duration=15.193s, table=32, n_packets=0, n_bytes=0, > idle_age=15, priority=150,reg14=0x1,reg15=0x4,metadata=0x5 actions=drop > cookie=0x0, duration=15.193s, table=32, n_packets=0, n_bytes=0, > idle_age=15, priority=100,reg15=0x4,metadata=0x5 > actions=load:0x5->NXM_NX_TUN_ID[0..23],set_field:0x4->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:28 > > Which means that if a packet is directed to p1 (reg15=0x4) send it through > the tunnel unless it comes from lp1 (reg14=0x1) when it'll be dropped. > > > > > On Fri, May 5, 2017 at 5:51 PM, Ben Pfaff <[email protected]> wrote: > > > [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
