In the original Open vSwitch LACP implementation, when no slaves
found a LACP partner, the LACP module would attach all of them.
This allowed the LACP bond to fall back to a standard bond when
partnered with a non-LACP switch. In practice, this has caused
confusion with marginal benefit, so this feature is removed with
this patch.
Signed-off-by: Ethan Jackson <et...@nicira.com>
---
NEWS | 4 +++-
lib/bond.c | 24 +++---------------------
lib/lacp.c | 15 +++++----------
3 files changed, 11 insertions(+), 32 deletions(-)
diff --git a/NEWS b/NEWS
index 1128af0..1addec7 100644
--- a/NEWS
+++ b/NEWS
@@ -11,7 +11,9 @@ post-v1.4.0
{=}, {!=}, {<}, {>}, {<=}, and {>=}.
- ovsdb-tool now uses the typical database and schema installation
directories as defaults.
-
+ - bonding
+ - LACP bonds no longer fall back to balance-slb when negotiations
+ fail.
v1.4.0 - xx xxx xxxx
------------------------
diff --git a/lib/bond.c b/lib/bond.c
index a2105ca..68ec0c5 100644
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -122,7 +122,6 @@ static void bond_enable_slave(struct bond_slave *, bool
enable,
struct tag_set *);
static void bond_link_status_update(struct bond_slave *, struct tag_set *);
static void bond_choose_active_slave(struct bond *, struct tag_set *);
-static bool bond_is_tcp_hash(const struct bond *);
static unsigned int bond_hash_src(const uint8_t mac[ETH_ADDR_LEN],
uint16_t vlan, uint32_t basis);
static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan,
@@ -405,7 +404,6 @@ void
bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated)
{
struct bond_slave *slave;
- bool is_tcp_hash = bond_is_tcp_hash(bond);
if (bond->lacp_negotiated != lacp_negotiated) {
bond->lacp_negotiated = lacp_negotiated;
@@ -427,10 +425,6 @@ bond_run(struct bond *bond, struct tag_set *tags, bool
lacp_negotiated)
bond->next_fake_iface_update = time_msec() + 1000;
}
- if (is_tcp_hash != bond_is_tcp_hash(bond)) {
- bond->bond_revalidate = true;
- }
-
if (bond->bond_revalidate) {
bond->bond_revalidate = false;
@@ -950,11 +944,6 @@ bond_print_details(struct ds *ds, const struct bond *bond)
ds_put_format(ds, "bond_mode: %s\n",
bond_mode_to_string(bond->balance));
- if (bond->balance != BM_AB) {
- ds_put_format(ds, "bond-hash-algorithm: %s\n",
- bond_is_tcp_hash(bond) ? "balance-tcp" : "balance-slb");
- }
-
ds_put_format(ds, "bond-hash-basis: %"PRIu32"\n", bond->basis);
ds_put_format(ds, "updelay: %d ms\n", bond->updelay);
@@ -1324,13 +1313,6 @@ bond_link_status_update(struct bond_slave *slave, struct
tag_set *tags)
}
}
-static bool
-bond_is_tcp_hash(const struct bond *bond)
-{
- return (bond->balance == BM_TCP && bond->lacp_negotiated)
- || bond->balance == BM_STABLE;
-}
-
static unsigned int
bond_hash_src(const uint8_t mac[ETH_ADDR_LEN], uint16_t vlan, uint32_t basis)
{
@@ -1352,9 +1334,9 @@ bond_hash_tcp(const struct flow *flow, uint16_t vlan,
uint32_t basis)
static unsigned int
bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan)
{
- assert(bond->balance != BM_AB);
+ assert(bond->balance == BM_TCP || bond->balance == BM_SLB);
- return (bond_is_tcp_hash(bond)
+ return (bond->balance == BM_TCP
? bond_hash_tcp(flow, vlan, bond->basis)
: bond_hash_src(flow->dl_src, vlan, bond->basis));
}
@@ -1381,7 +1363,7 @@ choose_stb_slave(const struct bond *bond, const struct
flow *flow,
best = NULL;
best_hash = 0;
- flow_hash = bond_hash(bond, flow, vlan);
+ flow_hash = bond_hash_tcp(flow, vlan, bond->basis);
HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
if (slave->enabled) {
uint32_t hash;
diff --git a/lib/lacp.c b/lib/lacp.c
index 244b86e..598d783 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -384,12 +384,8 @@ lacp_slave_may_enable(const struct lacp *lacp, const void
*slave_)
struct slave *slave = slave_lookup(lacp, slave_);
/* The slave may be enabled if it's attached to an aggregator and its
- * partner is synchronized. The only exception is defaulted slaves.
- * They are not required to have synchronized partners because they
- * have no partners at all. They will only be attached if negotiations
- * failed on all slaves in the bond. */
- return slave->attached && (slave->partner.state & LACP_STATE_SYNC
- || slave->status == LACP_DEFAULTED);
+ * partner is synchronized.*/
+ return slave->attached && (slave->partner.state & LACP_STATE_SYNC);
} else {
return true;
}
@@ -504,14 +500,13 @@ lacp_update_attached(struct lacp *lacp)
HMAP_FOR_EACH (slave, node, &lacp->slaves) {
struct lacp_info pri;
- slave->attached = true;
+ slave->attached = false;
/* XXX: In the future allow users to configure the expected system ID.
* For now just special case loopback. */
if (eth_addr_equals(slave->partner.sys_id, slave->lacp->sys_id)) {
VLOG_WARN_RL(&rl, "slave %s: Loopback detected. Slave is "
"connected to its own bond", slave->name);
- slave->attached = false;
continue;
}
@@ -519,6 +514,7 @@ lacp_update_attached(struct lacp *lacp)
continue;
}
+ slave->attached = true;
slave_get_priority(slave, &pri);
if (!lead || memcmp(&pri, &lead_pri, sizeof pri) < 0) {
@@ -531,8 +527,7 @@ lacp_update_attached(struct lacp *lacp)
if (lead) {
HMAP_FOR_EACH (slave, node, &lacp->slaves) {
- if (slave->status == LACP_DEFAULTED
- || lead->partner.key != slave->partner.key
+ if (lead->partner.key != slave->partner.key
|| !eth_addr_equals(lead->partner.sys_id,
slave->partner.sys_id)) {
slave->attached = false;
--
1.7.7.1
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
