bond: Always revalidate unbalanced bonds when active member changes.

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
Acked-by: Eelco Chaudron <[email protected]>
Signed-off-by: Mike Pattrick <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>

Author: mkp-rh

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,6 +550,7 @@ 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;
@@ -558,6 +560,9 @@ bond_active_member_changed(struct bond *bond)
         bond->active_member_mac = eth_addr_zero;
     }
     bond->active_member_changed = true;
+    if (!bond_is_balanced(bond)) {
+        bond->bond_revalidate = true;
+    }
     seq_change(connectivity_seq_get());
 }
 
@@ -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);

tests/ofproto-dpif.at

@@ -351,6 +351,49 @@ 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(src=50:54:00:00:00:0a,dst=50:54:00:00:00:09),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,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(src=50:54:00:00:00:0a,dst=50:54:00:00:00:09),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,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([ovs-appctl bond/show | grep -q 'active-backup primary: p4'])
+
+AT_CHECK([ovs-appctl revalidator/wait])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -q "actions:p[[13]]"], [1])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -q "actions:p[[24]]"], [0])
+
+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.