bridge: Keep bond active slave selection across OVS restart

Whenever OVS restarts, it pseudo-randomly picks an interface
of a bond port to be the active slave. This can cause traffic
disruption in case the upstream switch does not support LACP, or
in case of multi-chassis switches that do not support mLACP.

This patch helps the situation by always record the last active
slave into ovsdb. When OVS restarts, the stored last active slave
has the highest priority to be selected again. In case this interface
is available, due to configuration changes or being offline, OVS then
consider other interfaces with the bond as it does today.

In a nutshell, this patch makes the active slave selection stickier
across OVS restart.

VMware-BZ:  1332235

Signed-off-by: Andy Zhou <azhou@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>

diff --git a/NEWS b/NEWS
index 230e7cf..a785a62 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,7 @@ v2.3.1 - xx xxx xxxx
    - ovs-pki: Changed message digest algorithm from MD5 to SHA-1 because
      MD5 is no longer secure and some operating systems have started to disable
      it in OpenSSL.
+   - Keep active bond slave selection across OVS restart.
 
 v2.3.0 - 14 Aug 2014
 ---------------------

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 803408b..44e9df1 100644 (file)
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -132,6 +132,15 @@ struct bond {
     uint32_t recirc_id;          /* Non zero if recirculation can be used.*/
     struct hmap pr_rule_ops;     /* Helps to maintain post recirculation rules.*/
 
+    /* Store active slave to OVSDB. */
+    bool active_slave_changed; /* Set to true whenever the bond changes
+                                   active slave. It will be reset to false
+                                   after it is stored into OVSDB */
+
+    /* Interface name may not be persistent across an OS reboot, use
+     * MAC address for identifing the active slave */
+    uint8_t active_slave_mac[ETH_ADDR_LEN];
+                               /* The MAC address of the active interface. */
     /* Legacy compatibility. */
     long long int next_fake_iface_update; /* LLONG_MAX if disabled. */
     bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */
@@ -459,10 +468,47 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
         bond_entry_reset(bond);
     }
 
+    memcpy(bond->active_slave_mac, s->active_slave_mac,
+           sizeof s->active_slave_mac);
+
+    bond->active_slave_changed = false;
+
     ovs_rwlock_unlock(&rwlock);
     return revalidate;
 }
 
+static struct bond_slave *
+bond_find_slave_by_mac(const struct bond *bond, const uint8_t mac[6])
+{
+    struct bond_slave *slave;
+
+    /* Find the last active slave */
+    HMAP_FOR_EACH(slave, hmap_node, &bond->slaves) {
+        uint8_t slave_mac[6];
+
+        if (netdev_get_etheraddr(slave->netdev, slave_mac)) {
+            continue;
+        }
+
+        if (!memcmp(slave_mac, mac, sizeof(slave_mac))) {
+            return slave;
+        }
+    }
+
+    return NULL;
+}
+
+static void
+bond_active_slave_changed(struct bond *bond)
+{
+    uint8_t mac[6];
+
+    netdev_get_etheraddr(bond->active_slave->netdev, mac);
+    memcpy(bond->active_slave_mac, mac, sizeof bond->active_slave_mac);
+    bond->active_slave_changed = true;
+    seq_change(connectivity_seq_get());
+}
+
 static void
 bond_slave_set_netdev__(struct bond_slave *slave, struct netdev *netdev)
     OVS_REQ_WRLOCK(rwlock)
@@ -1293,6 +1339,11 @@ bond_print_details(struct ds *ds, const struct bond *bond)
         break;
     }
 
+    ds_put_cstr(ds, "active slave mac: ");
+    ds_put_format(ds, ETH_ADDR_FMT, ETH_ADDR_ARGS(bond->active_slave_mac));
+    slave = bond_find_slave_by_mac(bond, bond->active_slave_mac);
+    ds_put_format(ds,"(%s)\n", slave ? slave->name : "none");
+
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         shash_add(&slave_shash, slave->name, slave);
     }
@@ -1463,6 +1514,7 @@ bond_unixctl_set_active_slave(struct unixctl_conn *conn,
                   bond->name, slave->name);
         bond->send_learning_packets = true;
         unixctl_command_reply(conn, "done");
+        bond_active_slave_changed(bond);
     } else {
         unixctl_command_reply(conn, "no change");
     }
@@ -1770,6 +1822,12 @@ bond_choose_slave(const struct bond *bond)
 {
     struct bond_slave *slave, *best;
 
+    /* Find the last active slave. */
+    slave = bond_find_slave_by_mac(bond, bond->active_slave_mac);
+    if (slave && slave->enabled) {
+        return slave;
+    }
+
     /* Find an enabled slave. */
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         if (slave->enabled) {
@@ -1810,6 +1868,10 @@ bond_choose_active_slave(struct bond *bond)
         }
 
         bond->send_learning_packets = true;
+
+        if (bond->active_slave != old_active_slave) {
+            bond_active_slave_changed(bond);
+        }
     } else if (old_active_slave) {
         VLOG_INFO_RL(&rl, "bond %s: all interfaces disabled", bond->name);
     }
@@ -1851,3 +1913,27 @@ bond_update_fake_slave_stats(struct bond *bond)
         netdev_close(bond_dev);
     }
 }
+
+/*
+ * Return true if bond has unstored active slave change.
+ * If return true, 'mac' will store the bond's current active slave's
+ * MAC address.  */
+bool
+bond_get_changed_active_slave(const char *name, uint8_t* mac, bool force)
+{
+    struct bond *bond;
+
+    ovs_rwlock_wrlock(&rwlock);
+    bond = bond_find(name);
+    if (bond) {
+        if (bond->active_slave_changed || force) {
+            memcpy(mac, bond->active_slave_mac, ETH_ADDR_LEN);
+            bond->active_slave_changed = false;
+            ovs_rwlock_unlock(&rwlock);
+            return true;
+        }
+    }
+    ovs_rwlock_unlock(&rwlock);
+
+    return false;
+}

diff --git a/ofproto/bond.h b/ofproto/bond.h
index 0d9a67a..d7cb715 100644 (file)
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -55,6 +55,10 @@ struct bond_settings {
     /* Legacy compatibility. */
     bool fake_iface;            /* Update fake stats for netdev 'name'? */
     bool lacp_fallback_ab_cfg;  /* Fallback to active-backup on LACP failure. */
+
+    uint8_t active_slave_mac[6];/* The MAC address of the interface
+                                   that was active during the last
+                                   ovs run. */
 };
 
 /* Program startup. */
@@ -81,6 +85,8 @@ bool bond_should_send_learning_packets(struct bond *);
 struct ofpbuf *bond_compose_learning_packet(struct bond *,
                                             const uint8_t eth_src[ETH_ADDR_LEN],
                                             uint16_t vlan, void **port_aux);
+bool bond_get_changed_active_slave(const char *name, uint8_t mac[6],
+                                        bool force);
 
 /* Packet processing. */
 enum bond_verdict {

diff --git a/tests/lacp.at b/tests/lacp.at
index 0db2077..49f1394 100644 (file)
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -5,6 +5,11 @@ m4_define([STRIP_RECIRC_ID], [[sed '
     s/Recirc-ID.*$/<del>/
 ' ]])
 
+# Strips out active slave mac address since it may change over time.
+m4_define([STRIP_ACTIVE_SLAVE_MAC], [[sed '
+    s/active slave mac.*$/<active slave mac del>/
+' ]])
+
 AT_SETUP([lacp - config])
 OVS_VSWITCHD_START([\
         add-port br0 p1 --\
@@ -123,6 +128,7 @@ bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
 lacp_status: negotiated
+active slave mac: 00:00:00:00:00:00(none)
 
 slave p1: disabled
        may_enable: false
@@ -188,8 +194,8 @@ done
 AT_CHECK(
   [ovs-appctl lacp/show bond0
 ovs-appctl lacp/show bond1
-ovs-appctl bond/show bond0 | STRIP_RECIRC_ID
-ovs-appctl bond/show bond1 | STRIP_RECIRC_ID ], [0], [stdout])
+ovs-appctl bond/show bond0 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC
+ovs-appctl bond/show bond1 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC ], [0], [stdout])
 AT_CHECK([sed '/active slave/d' stdout], [0], [dnl
 ---- bond0 ----
        status: active negotiated
@@ -308,7 +314,7 @@ slave p3: enabled
        may_enable: true
 
 ])
-AT_CHECK([grep 'active slave' stdout], [0], [dnl
+AT_CHECK([grep 'active slave$' stdout], [0], [dnl
        active slave
        active slave
 ])
@@ -324,8 +330,8 @@ for i in `seq 0 40`; do ovs-appctl time/warp 100; done
 AT_CHECK(
   [ovs-appctl lacp/show bond0
 ovs-appctl lacp/show bond1
-ovs-appctl bond/show bond0 | STRIP_RECIRC_ID
-ovs-appctl bond/show bond1 | STRIP_RECIRC_ID ], [0], [dnl
+ovs-appctl bond/show bond0 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC
+ovs-appctl bond/show bond1 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC ], [0], [dnl
 ---- bond0 ----
        status: active negotiated
        sys_id: aa:55:aa:55:00:00
@@ -421,6 +427,7 @@ bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
 lacp_status: negotiated
+<active slave mac del>
 
 slave p0: disabled
        may_enable: false
@@ -436,6 +443,7 @@ bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
 lacp_status: negotiated
+<active slave mac del>
 
 slave p2: disabled
        may_enable: false
@@ -452,8 +460,8 @@ for i in `seq 0 40`; do ovs-appctl time/warp 100; done
 AT_CHECK(
   [ovs-appctl lacp/show bond0
 ovs-appctl lacp/show bond1
-ovs-appctl bond/show bond0 | STRIP_RECIRC_ID
-ovs-appctl bond/show bond1 | STRIP_RECIRC_ID ], [0], [dnl
+ovs-appctl bond/show bond0 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC
+ovs-appctl bond/show bond1 | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC ], [0], [dnl
 ---- bond0 ----
        status: active negotiated
        sys_id: aa:55:aa:55:00:00
@@ -549,6 +557,7 @@ bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
 lacp_status: negotiated
+<active slave mac del>
 
 slave p0: disabled
        may_enable: false
@@ -564,6 +573,7 @@ bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
 lacp_status: negotiated
+<active slave mac del>
 
 slave p2: disabled
        may_enable: false

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d2643a0..822b471 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -60,6 +60,7 @@
 #include "vlog.h"
 #include "sflow_api.h"
 #include "vlan-bitmap.h"
+#include "packets.h"
 
 VLOG_DEFINE_THIS_MODULE(bridge);
 
@@ -385,6 +386,7 @@ bridge_init(const char *remote)
 
     ovsdb_idl_omit_alert(idl, &ovsrec_port_col_status);
     ovsdb_idl_omit_alert(idl, &ovsrec_port_col_statistics);
+    ovsdb_idl_omit_alert(idl, &ovsrec_port_col_bond_active_slave);
     ovsdb_idl_omit(idl, &ovsrec_port_col_external_ids);
 
     ovsdb_idl_omit_alert(idl, &ovsrec_interface_col_admin_state);
@@ -2140,6 +2142,26 @@ port_refresh_stp_stats(struct port *port)
                                ARRAY_SIZE(int_values));
 }
 
+static void
+port_refresh_bond_status(struct port *port, bool force_update)
+{
+    uint8_t mac[6];
+
+    /* Return if port is not a bond */
+    if (list_is_singleton(&port->ifaces)) {
+        return;
+    }
+
+    if (bond_get_changed_active_slave(port->name, mac, force_update)) {
+        struct ds mac_s;
+
+        ds_init(&mac_s);
+        ds_put_format(&mac_s, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
+        ovsrec_port_set_bond_active_slave(port->cfg, ds_cstr(&mac_s));
+        ds_destroy(&mac_s);
+    }
+}
+
 static bool
 enable_system_stats(const struct ovsrec_open_vswitch *cfg)
 {
@@ -2238,7 +2260,7 @@ refresh_controller_status(void)
 
     ofproto_free_ofproto_controller_info(&info);
 }
-\f
+
 static void
 bridge_run__(void)
 {
@@ -2439,6 +2461,7 @@ bridge_run(void)
                     struct iface *iface;
 
                     port_refresh_stp_status(port);
+                    port_refresh_bond_status(port, force_status_commit);
                     LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
                         iface_refresh_netdev_status(iface);
                         iface_refresh_ofproto_status(iface);
@@ -3362,6 +3385,7 @@ port_configure_bond(struct port *port, struct bond_settings *s)
 {
     const char *detect_s;
     struct iface *iface;
+    const char *mac_s;
     int miimon_interval;
 
     s->name = port->name;
@@ -3420,6 +3444,13 @@ port_configure_bond(struct port *port, struct bond_settings *s)
     LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
         netdev_set_miimon_interval(iface->netdev, miimon_interval);
     }
+
+    mac_s = port->cfg->bond_active_slave;
+    if (!mac_s || !ovs_scan(mac_s, ETH_ADDR_SCAN_FMT,
+                            ETH_ADDR_SCAN_ARGS(s->active_slave_mac))) {
+        /* OVSDB did not store the last active interface */
+        memset(s->active_slave_mac, 0, sizeof(s->active_slave_mac));
+    }
 }
 
 /* Returns true if 'port' is synthetic, that is, if we constructed it locally

diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 006ed23..08c9b20 100644 (file)
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
 {"name": "Open_vSwitch",
- "version": "7.6.0",
- "cksum": "1731605290 20602",
+ "version": "7.6.1",
+ "cksum": "1587823839 20754",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -153,6 +153,10 @@
          "type": "integer"},
        "bond_downdelay": {
          "type": "integer"},
+       "bond_active_slave": {
+         "type": {"key": {"type": "string"},
+                  "min": 0, "max": 1},
+                  "ephemeral": true},
        "bond_fake_iface": {
          "type": "boolean"},
        "fake_bridge": {

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 2b05901..79a2b5a 100644 (file)
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1225,6 +1225,13 @@
           STP role of the port.
         </p>
       </column>
+
+      <column name="status" key="bond_active_slave">
+        <p>
+          For a bonded port, record the mac address of the current active slave.
+        </p>
+      </column>
+
     </group>
 
     <group title="Port Statistics">