| Message ID | 20241003210002.261576-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v7] bond: Always revalidate unbalanced bonds when active member changes. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Thu, Oct 3, 2024 at 5:02 PM Mike Pattrick <mkp@redhat.com> 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 <mkp@redhat.com> > --- > 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 >
On 3 Oct 2024, at 23:00, Mike Pattrick 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 <mkp@redhat.com> Thanks for the updated revision. The patch looks good to me. I'm not sure if we still need the DP test now that we have the unit test. I'll leave it to Ilya to decide, as he originally requested it. //Eelco Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > 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], []) > + > +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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/4/24 16:27, Eelco Chaudron wrote: > > > On 3 Oct 2024, at 23:00, Mike Pattrick 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 <mkp@redhat.com> > > Thanks for the updated revision. The patch looks good to me. I'm not sure if > we still need the DP test now that we have the unit test. I'll leave it to > Ilya to decide, as he originally requested it. I think we should remove the system test as it is very similar to the unit test. See some more comments below. > > //Eelco > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> --- >> 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; >> + } Why we revalidate only if there is an active member? If all members end up disabled we should also revalidate to start dropping traffic directed to this bond. >> } 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)']) >> + I'd suggest to not swap places for src and dst, otherwise it's really hard to tell if these packets are different or not. >> +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'`"]) There is no need for 'test', just grep -q would be enough. And the apperance of p4 in the bond/show output doesn't mean the revalidation is done, so there is a race here. We need to wait for revalidators: AT_CHECK([ovs-appctl revalidator/wait]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "actions:p[[13]]"], [1], []) The [] at the end is not needed. Also, we need to check that flows didn't just expire, i.e. that there are new updated flows in there. Best regards, Ilya Maximets.
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], []) + +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()
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 <mkp@redhat.com> --- 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(-)