On Tue, May 9, 2017 at 9:17 PM, Ben Pfaff <b...@ovn.org> wrote: > Thanks. > > This is a good summary of some of what the patch does. Should it be in > the commit message? > ack I ommited the commands and outputs in the commit message.
In order to illustrate the purpose of this patch: - One logical switch sw0 with 2 ports (p1, p2) and 1 localport (lp) - Two hypervisors: HV1 and HV2 - p1 will be in HV1 (OVS port with external-id:iface-id="p1") - p2 will be in HV2 (OVS port with external-id:iface-id="p2") - lp will be in both (OVS port with external-id:iface-id="lp") - p1 should be able to reach p2 and viceversa - lp on HV1 should be able to reach p1 but not p2 - lp on HV2 should be able to reach p2 but not p1 ovn-nbctl ls-add sw0 ovn-nbctl lsp-add sw0 p1 ovn-nbctl lsp-add sw0 p2 ovn-nbctl lsp-add sw0 lp ovn-nbctl lsp-set-addresses p1 "00:00:00:aa:bb:10 10.0.1.10" ovn-nbctl lsp-set-addresses p2 "00:00:00:aa:bb:20 10.0.1.20" ovn-nbctl lsp-set-addresses lp "00:00:00:aa:bb:30 10.0.1.30" ovn-nbctl lsp-set-type lp localport add_phys_port() { name=$1 mac=$2 ip=$3 mask=$4 gw=$5 iface_id=$6 sudo ip netns add $name sudo ovs-vsctl add-port br-int $name -- set interface $name type=internal sudo ip link set $name netns $name sudo ip netns exec $name ip link set $name address $mac sudo ip netns exec $name ip addr add $ip/$mask dev $name sudo ip netns exec $name ip link set $name up sudo ip netns exec $name ip route add default via $gw sudo ovs-vsctl set Interface $name external_ids:iface-id=$iface_id } # Add p1 to HV1, p2 to HV2 and localport to both # HV1 add_phys_port p1 00:00:00:aa:bb:10 10.0.1.10 24 10.0.1.1 p1 add_phys_port lp 00:00:00:aa:bb:30 10.0.1.30 24 10.0.1.1 lp $ sudo ip netns exec p1 ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. 64 bytes from 10.0.1.20: icmp_seq=1 ttl=64 time=0.738 ms 64 bytes from 10.0.1.20: icmp_seq=2 ttl=64 time=0.502 ms --- 10.0.1.20 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1001ms rtt min/avg/max/mdev = 0.502/0.620/0.738/0.118 ms $ sudo ip netns exec lp ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. 64 bytes from 10.0.1.10: icmp_seq=1 ttl=64 time=0.187 ms 64 bytes from 10.0.1.10: icmp_seq=2 ttl=64 time=0.032 ms --- 10.0.1.10 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 999ms rtt min/avg/max/mdev = 0.032/0.109/0.187/0.078 ms $ sudo ip netns exec lp ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. --- 10.0.1.20 ping statistics --- 2 packets transmitted, 0 received, 100% packet loss, time 1000ms $ sudo ovs-ofctl dump-flows br-int | grep table=32 cookie=0x0, duration=141.939s, table=32, n_packets=2, n_bytes=196, idle_age=123, priority=150,reg14=0x3,reg15=0x2,metadata=0x7 actions=drop cookie=0x0, duration=141.939s, table=32, n_packets=2, n_bytes=196, idle_age=129, priority=100,reg15=0x2,metadata=0x7 actions=load:0x7->NXM_NX_TUN_ID[0..23],set_field:0x2->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:59 # On HV2 add_phys_port p2 00:00:00:aa:bb:20 10.0.1.20 24 10.0.1.1 p2 add_phys_port lp 00:00:00:aa:bb:30 10.0.1.30 24 10.0.1.1 lp $ sudo ip netns exec p2 ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. 64 bytes from 10.0.1.10: icmp_seq=1 ttl=64 time=0.810 ms 64 bytes from 10.0.1.10: icmp_seq=2 ttl=64 time=0.673 ms --- 10.0.1.10 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1000ms rtt min/avg/max/mdev = 0.673/0.741/0.810/0.073 ms $ sudo ip netns exec lp ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. 64 bytes from 10.0.1.20: icmp_seq=1 ttl=64 time=0.357 ms 64 bytes from 10.0.1.20: icmp_seq=2 ttl=64 time=0.062 ms --- 10.0.1.20 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1000ms rtt min/avg/max/mdev = 0.062/0.209/0.357/0.148 ms $ sudo ip netns exec lp ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. --- 10.0.1.10 ping statistics --- 2 packets transmitted, 0 received, 100% packet loss, time 999ms $ sudo ovs-ofctl dump-flows br-int | grep table=32 cookie=0x0, duration=24.169s, table=32, n_packets=2, n_bytes=196, idle_age=12, priority=150,reg14=0x3,reg15=0x1,metadata=0x7 actions=drop cookie=0x0, duration=24.169s, table=32, n_packets=2, n_bytes=196, idle_age=14, priority=100,reg15=0x1,metadata=0x7 actions=load:0x7->NXM_NX_TUN_ID[0..23],set_field:0x1->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:40 > 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_me > tadata0,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_me > tadata0,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 <b...@ovn.org> 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 <b...@ovn.org> 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 <dalva...@redhat.com> > > > > > > > > > > > > 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/3319 > 50.html > > > > > > > > > > > > Thanks!! I picked those and submitted v2 :) > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/3320 > 94.html > > > > > > > > > > > > > > > Thanks again! > > > > > > > > > > > > > > > > Thanks Ben for your great review. > > > > > > > > You're welcome, I'll try a look at v2 now. > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev