On Thu, Oct 3, 2024 at 5:02 PM Mike Pattrick <[email protected]> wrote: > > Currently a bond will not always revalidate when an active member > changes. This can result in counter-intuitive behaviors like the fact > that using ovs-appctl bond/set-active-member will cause the bond to > revalidate but changing other_config:bond-primary will not trigger a > revalidate in the bond. > > When revalidation is not set but the active member changes in an > unbalanced bond, OVS may send traffic out of previously active member > instead of the new active member. > > This change will always mark unbalanced bonds for revalidation if the > active member changes. > > Reported-at: https://issues.redhat.com/browse/FDP-845 > Signed-off-by: Mike Pattrick <[email protected]> > --- > v2: Added a test > v3: Made the test more reliable > v4: Made test much more reliable > v5: Improved test performance > v6: Improved system test by removing waits on ping. > v7: Added a unit test. > --- > ofproto/bond.c | 8 +++++-- > tests/ofproto-dpif.at | 40 ++++++++++++++++++++++++++++++++ > tests/system-traffic.at | 51 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 97 insertions(+), 2 deletions(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 0858de374..b9e2282f0 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond > *, bool force) > static bool bond_is_falling_back_to_ab(const struct bond *); > static void bond_add_lb_output_buckets(const struct bond *); > static void bond_del_lb_output_buckets(const struct bond *); > +static bool bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock); > > > /* Attempts to parse 's' as the name of a bond balancing mode. If > successful, > @@ -549,11 +550,15 @@ bond_find_member_by_mac(const struct bond *bond, const > struct eth_addr mac) > > static void > bond_active_member_changed(struct bond *bond) > + OVS_REQ_WRLOCK(rwlock) > { > if (bond->active_member) { > struct eth_addr mac; > netdev_get_etheraddr(bond->active_member->netdev, &mac); > bond->active_member_mac = mac; > + if (!bond_is_balanced(bond)) { > + bond->bond_revalidate = true; > + } > } else { > bond->active_member_mac = eth_addr_zero; > } > @@ -1121,7 +1126,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond, > uint32_t *recirc_id, > /* Rebalancing. */ > > static bool > -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock) > +bond_is_balanced(const struct bond *bond) > { > return bond->rebalance_interval > && (bond->balance == BM_SLB || bond->balance == BM_TCP) > @@ -1725,7 +1730,6 @@ bond_unixctl_set_active_member(struct unixctl_conn > *conn, > } > > if (bond->active_member != member) { > - bond->bond_revalidate = true; > bond->active_member = member; > VLOG_INFO("bond %s: active member is now %s", > bond->name, member->name); > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 12cb7f7a6..7a916de54 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -351,6 +351,46 @@ > recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=ff: > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - active-backup bonding set primary]) > + > +OVS_VSWITCHD_START( > + [add-bond br0 bond0 p1 p2 bond_mode=active-backup \ > + other_config:bond-primary=p1 -- \ > + set bridge br0 other-config:hwaddr=aa:66:aa:66:aa:00 -- \ > + 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 -- \ > + add-port br0 p7 -- set interface p7 ofport_request=7 type=dummy -- \ > + add-br br1 -- \ > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > + set bridge br1 datapath-type=dummy -- \ > + add-bond br1 bond1 p3 p4 bond_mode=active-backup \ > + other_config:bond-primary=p3 -- \ > + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock > ofport_request=3 -- \ > + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock > ofport_request=4 -- \ > + add-port br1 p8 -- set interface p8 ofport_request=8 type=dummy]) > + > +WAIT_FOR_DUMMY_PORTS([p3], [p4]) > + > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl add-flow br1 action=normal]) > + > +dnl Create datapath flow with bidirectional traffic. > +AT_CHECK([ovs-appctl netdev-dummy/receive p8 > 'in_port(8),eth(dst=50:54:00:00:00:09,src=50:54:00:00:00:0a),eth_type(0x0800),ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p7 > 'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p8 > 'in_port(8),eth(dst=50:54:00:00:00:09,src=50:54:00:00:00:0a),eth_type(0x0800),ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p7 > 'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) > + > +dnl Set p2 and p4 as primary. > +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=p2 -- \ > + set port bond1 other_config:bond-primary=p4]) > + > +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup > primary: p4'`"]) > + > +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "actions:p[[13]]"], > [1], [])
I find that netdev-dummy/receive causes the relevant flows to revalidate immediately, whereas non-dummy datapaths don't have this behaviour. However, this test should fail/succeed appropriately depending on if setting bond-primary triggers revalidation. -M > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - balance-slb bonding]) > # Create br0 with members bond0(p1, p2, p3) and p7, > # and br1 with members p4, p5, p6 and p8. > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 724b25fa9..64f5400ad 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -291,6 +291,57 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -W > 2 10.1.1.2 | FORMAT_PING > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([datapath - bond active-backup set primary]) > +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) > + > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > +AT_CHECK([ovs-ofctl add-flow br1 "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24") > +on_exit 'ip link del link0a' > +on_exit 'ip link del link0b' > +AT_CHECK([ip link add link0a type veth peer name link1a]) > +AT_CHECK([ip link add link0b type veth peer name link1b]) > + > +AT_CHECK([ip link set dev link0a up]) > +AT_CHECK([ip link set dev link1a up]) > +AT_CHECK([ip link set dev link0b up]) > +AT_CHECK([ip link set dev link1b up]) > + > +AT_CHECK([ovs-vsctl add-bond br0 bond0 link0a link0b > bond_mode=active-backup]) > +AT_CHECK([ovs-vsctl add-bond br1 bond1 link1a link1b > bond_mode=active-backup]) > + > +dnl Set primary bond member. > +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0a -- \ > + set port bond1 other_config:bond-primary=link1a]) > + > +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup > primary: link0a'`"]) > + > +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2]) > + > +dnl Keep traffic active in the background. > +NETNS_DAEMONIZE([at_ns0], [ping -q 10.1.1.2], [ping.pid]) > + > +dnl Check correct port is used. > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq "actions:link[[01]]a"], > [0]) > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq "actions:link[[01]]b"], > [1]) > + > +dnl Change primary bond member. > +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0b -- \ > + set port bond1 other_config:bond-primary=link1b]) > + > +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup > primary: link0b'`"]) > + > +dnl Check correct port is used. > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq "actions:link[[01]]a"], > [1]) > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq "actions:link[[01]]b"], > [0]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([datapath - ping over vxlan tunnel]) > AT_SKIP_IF([test $HAVE_NC = no]) > OVS_CHECK_VXLAN() > -- > 2.43.5 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
