[ovs-dev] [PATCH v3 07/16] ofproto-dpif: Bonding with in_port that isn't present in the datapath

Andy Zhou azhou at nicira.com
Wed Apr 30 09:22:24 UTC 2014 

On Wed, Apr 30, 2014 at 1:35 AM, Simon Horman <horms at verge.net.au> wrote:
> On Wed, Apr 30, 2014 at 01:10:02AM -0700, Andy Zhou wrote:
>> Simon,
>>
>> Thanks for explaining. It is now clear what the test is about.
>>
>> I spent some time today to study this.  The root cause seems to be in
>> how packet out are
>> implemented, not with recirculation or bond.   A reasonable solution
>> may be treating patch ports as
>> LOCAL ports for packet out. I have prototyped this solution and passed
>> the test.  Any thoughts?
>
> Will this also handle other ports that are not present in the datapath
> for packet_out? In particular the CONTROLLER port?

This is a good idea, The patch I sent out will only handle patch ports. Let me
send a v2 to address CONTROLLER as well.

>
>> I will clean it up and send it out soon.
>
> Thanks.
>
>>
>> Andy
>>
>> On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman <horms at verge.net.au> wrote:
>> > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote:
>> >> If p7 and p37 are now connected together as patch ports, would any
>> >> traffic injected from p7 bypass (in the user space) the bond
>> >> interfaces all-together?
>> >
>> > I believe what happens is that the normal action forwards
>> > the packet to all ports that are connected to br0 to other than p7.
>> > That is the packet is forwarded to bond0.
>> >
>> > Without this patch, the packet is then executed in the datapath
>> > and an upcall is made as a result of a recirculation action.
>> > In handle_upcalls() the call to xlate_receive() fails because
>> > the in_port does not exist in the datapath.
>> >
>> >
>> > To be honest I'm not particularly concerned if patch-ports work in
>> > conjunction with bonding or recirculation in general. And indeed they don't
>> > even with this series applied. The purpose of this patch was to illustrate
>> > the problem that without other patches in this series recirculation doesn't
>> > work in cases where the in_port doesn't exist in the datapath. And in the
>> > case of bonding the only example I could come up with involved a
>> > patch-port.
>> >
>> > In the case of recirculation for MPLS, which is also added by this series,
>> > a more realistic example can be constructed using packet_out with
>> > CONTROLLER as the in_port. This case I am inclined to care about.
>> > Particularly as it is used extensively by Ryu's test-suite which can be run
>> > using Open vSwitch's make check-ryu target.
>> >
>> > I did not use CONTROLLER as the in_port for the bonding test as for reasons
>> > I don't fully understand packets with the CONTROLLER as the in_port do not
>> > seem to be able to be handled by a normal action.  And it it my
>> > understanding that a normal action is needed to forward packets to a bond.
>> >
>> >>
>> >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman <horms at verge.net.au> wrote:
>> >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote:
>> >> >> Simon,  I thought this test exposed a bug that is fixed with this
>> >> >> patch series.  However, when I apply this patch alone against current
>> >> >> master, it passed fine. (I tried many times).   So I must
>> >> >> misunderstand the intention of the test, or why current master failed
>> >> >> to address
>> >> >> packet out.  Would you please shed some light on this?
>> >> >
>> >> > Hi Andy,
>> >> >
>> >> > thanks for pointing that out. My intention was, as you suggest,
>> >> > to provide a test that fails without previous patches in the series.
>> >> > Unfortunately I managed to create a test that passes regardless
>> >> > of the presence of other patches in the series :(
>> >> >
>> >> > Could you please take a look at the updated test below?
>> >> > It should fail without earlier patches in the series.
>> >> >
>> >> > From: Simon Horman <horms at verge.net.au>
>> >> >
>> >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the datapath
>> >> >
>> >> > This tests exercises execution of actions in ovs-vswitchd
>> >> > in the case where a packet is processed due to a packet out message
>> >> > with an in_port that doesn't exist in the datapath and translation
>> >> > results in recirc actions due to bonding.
>> >> >
>> >> > Signed-off-by: Simon Horman <horms at verge.net.au>
>> >> >
>> >> > ---
>> >> > v2
>> >> > * First Post
>> >> >
>> >> > v2.1
>> >> > * Use a patch-port instead of NONE as the in_port for packet_out.
>> >> >   This has the intended effect of causing a packet with an in_port
>> >> >   that does not exist in the datapath to be output to a bonding interface.
>> >> > ---
>> >> >  tests/ofproto-dpif.at | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 90 insertions(+)
>> >> >
>> >> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> >> > index 89c8ad7..2113464 100644
>> >> > --- a/tests/ofproto-dpif.at
>> >> > +++ b/tests/ofproto-dpif.at
>> >> > @@ -197,6 +197,96 @@ AT_CHECK([test `grep in_port.6 br1_flows.txt |wc -l` -gt 7])
>> >> >  OVS_VSWITCHD_STOP()
>> >> >  AT_CLEANUP
>> >> >
>> >> > +AT_SETUP([ofproto-dpif, balance-tcp bonding, packet-out])
>> >> > +# Create br0 with interfaces bond0(p1, p2, p3), bond1(p4,p5,p6) and p7, and
>> >> > +#        br1 with interfaces bond10(p11, p12, p13) and bond11(p14,p15,p16), and
>> >> > +#        br2 with interfaces bond20(p21, p22, p23) and bond21(p24,p25,p26)
>> >> > +#        br3 with interface p37
>> >> > +#    bond0 <-> bond10 (unix socket backed interfaces)
>> >> > +#    bond1 <-> bond20 (unix socket backed interfaces)
>> >> > +#    p7 <-> p37 (patch interface)
>> >> > +# Send some traffic, make sure the traffic are spread based on L4 headers.
>> >> > +OVS_VSWITCHD_START(
>> >> > +  [add-bond br0 bond0 p1 p2 p3 bond_mode=balance-tcp lacp=active \
>> >> > +        other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --\
>> >> > +   set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \
>> >> > +   set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock ofport_request=2 -- \
>> >> > +   set interface p3 type=dummy options:pstream=punix:$OVS_RUNDIR/p3.sock ofport_request=3 -- \
>> >> > +   add-bond br0 bond1 p4 p5 p6 bond_mode=balance-tcp lacp=active \
>> >> > +        other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --\
>> >> > +   set interface p4 type=dummy options:pstream=punix:$OVS_RUNDIR/p4.sock ofport_request=4 -- \
>> >> > +   set interface p5 type=dummy options:pstream=punix:$OVS_RUNDIR/p5.sock ofport_request=5 -- \
>> >> > +   set interface p6 type=dummy options:pstream=punix:$OVS_RUNDIR/p6.sock ofport_request=6 -- \
>> >> > +   add-port br0 p7 -- \
>> >> > +   set interface p7 type=patch options:peer=p37 ofport_request=7 -- \
>> >> > +   add-br br1 -- \
>> >> > +   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>> >> > +   set bridge br1 datapath-type=dummy other-config:datapath-id=1234 fail-mode=secure -- \
>> >> > +   add-bond br1 bond10 p11 p12 p13 bond_mode=balance-tcp lacp=active \
>> >> > +        other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --\
>> >> > +   set interface p11 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \
>> >> > +   set interface p12 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock ofport_request=3 -- \
>> >> > +   set interface p13 type=dummy options:stream=unix:$OVS_RUNDIR/p3.sock ofport_request=2 -- \
>> >> > +   add-bond br1 bond11 p14 p15 p16 bond_mode=balance-tcp lacp=active \
>> >> > +        other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --\
>> >> > +   set interface p14 type=dummy options:stream=unix:$OVS_RUNDIR/p7.sock ofport_request=4 -- \
>> >> > +   set interface p15 type=dummy options:stream=unix:$OVS_RUNDIR/p8.sock ofport_request=5 -- \
>> >> > +   set interface p16 type=dummy options:stream=unix:$OVS_RUNDIR/p9.sock ofport_request=6 -- \
>> >> > +   add-br br2 -- \
>> >> > +   set bridge br2 other-config:hwaddr=aa:66:aa:66:00:02 -- \
>> >> > +   set bridge br2 datapath-type=dummy other-config:datapath-id=1235 fail-mode=secure -- \
>> >> > +   add-bond br2 bond20 p21 p22 p23 bond_mode=balance-tcp lacp=active \
>> >> > +        other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --\
>> >> > +   set interface p21 type=dummy options:stream=unix:$OVS_RUNDIR/p4.sock ofport_request=1 -- \
>> >> > +   set interface p22 type=dummy options:stream=unix:$OVS_RUNDIR/p5.sock ofport_request=2 -- \
>> >> > +   set interface p23 type=dummy options:stream=unix:$OVS_RUNDIR/p6.sock ofport_request=3 -- \
>> >> > +   add-br br3 -- \
>> >> > +   set bridge br3 other-config:hwaddr=aa:66:aa:66:00:03 -- \
>> >> > +   set bridge br3 datapath-type=dummy other-config:datapath-id=1236 fail-mode=secure -- \
>> >> > +   add-port br3 p37 -- \
>> >> > +   set interface p37 type=patch options:peer=p7 ofport_request=37 -- ])
>> >> > +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
>> >> > +])
>> >> > +AT_CHECK([ovs-ofctl add-flow br1 action=normal])
>> >> > +AT_CHECK([ovs-ofctl add-flow br2 action=normal])
>> >> > +AT_CHECK([ovs-appctl upcall/disable-megaflows], [0], [megaflows disabled
>> >> > +], [])
>> >> > +sleep 1;
>> >> > +ovs-appctl time/stop
>> >> > +ovs-appctl time/warp 100
>> >> > +ovs-appctl lacp/show > lacp.txt
>> >> > +ovs-appctl bond/show > bond.txt
>> >> > +
>> >> > +ovs-appctl vlog/set dpif,file,dbg
>> >> > +#ovs-appctl vlog/set ofproto,file,dbg
>> >> > +
>> >> > +dnl The input is a TCP/IP frame which tcpdump -vve shows as:
>> >> > +dnl 60:66:66:66:00:01 > 50:54:00:00:00:07, ethertype IPv4 (0x0800), length 58: (tos 0x0, ttl 255, id 0, offset 0, flags [none], proto TCP (6), length 44)
>> >> > +dnl     192.168.0.1.80 > 192.168.0.2.$port: Flags [none], cksum 0x7744 (correct), seq 42:46, win 10000, length 4
>> >> > +(
>> >> > +for i in `seq 0 255` ;
>> >> > +    do
>> >> > +    port=$(printf "%02x" $i)
>> >> > +    ovs-ofctl -O OpenFlow13 packet-out br0 7 normal "50 54 00 00 00 07 60 66 66 66 00 01 08 00 45 00 00 2c 00 00 00 00 ff 06 3a 78 c0 a8 00 01 c0 a8 00 02 00 50 00 $port 00 00 00 2a 00 00 00 2a 50 00 27 10 77 44 00 00 48 4f 47 45"
>> >> > +done
>> >> > +)
>> >> > +
>> >> > +ovs-appctl time/warp 100
>> >> > +ovs-appctl time/warp 100
>> >> > +ovs-appctl time/warp 100
>> >> > +AT_CHECK([ovs-appctl dpif/dump-flows br1 |grep tcp > br1_flows.txt])
>> >> > +AT_CHECK([ovs-appctl dpif/dump-flows br2 |grep tcp > br2_flows.txt])
>> >> > +# Make sure there is resonable distribution to all three ports.
>> >> > +# We don't want to make this check precise, in case hash function changes.
>> >> > +AT_CHECK([test `grep in_port.11 br1_flows.txt |wc -l` -gt 7])
>> >> > +AT_CHECK([test `grep in_port.12 br1_flows.txt |wc -l` -gt 7])
>> >> > +AT_CHECK([test `grep in_port.13 br1_flows.txt |wc -l` -gt 7])
>> >> > +AT_CHECK([test `grep in_port.21 br2_flows.txt |wc -l` -gt 7])
>> >> > +AT_CHECK([test `grep in_port.22 br2_flows.txt |wc -l` -gt 7])
>> >> > +AT_CHECK([test `grep in_port.23 br2_flows.txt |wc -l` -gt 7])
>> >> > +OVS_VSWITCHD_STOP()
>> >> > +AT_CLEANUP
>> >> > +
>> >> >  AT_SETUP([ofproto-dpif - resubmit])
>> >> >  OVS_VSWITCHD_START
>> >> >  ADD_OF_PORTS([br0], [1], [10], [11], [12], [13], [14], [15],
>> >> > --
>> >> > 1.8.5.2
>> >> >
>> >>
>>

More information about the dev mailing list