[ovs-dev] [PATCH] OVN localport type support
Daniel Alvarez Sanchez
dalvarez at redhat.com
Wed May 10 08:37:00 UTC 2017
On Tue, May 9, 2017 at 9:17 PM, Ben Pfaff <blp at 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 <blp at 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 <blp at 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 <dalvarez at 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.
> > >
>
More information about the dev
mailing list