[ovs-dev] [PATCH v2] AB bonding: Add "primary" interface concept
Jeff Squyres (jsquyres)
jsquyres at cisco.com
Mon May 4 15:17:28 UTC 2020
On May 4, 2020, at 10:27 AM, Aaron Conole <aconole at redhat.com<mailto:aconole at redhat.com>> wrote:
Aaron Conole <aconole at redhat.com<mailto:aconole at redhat.com>> writes:
Aaron Conole <aconole at redhat.com<mailto:aconole at redhat.com>> writes:
Jeff Squyres via dev <ovs-dev at openvswitch.org<mailto:ovs-dev at openvswitch.org>> writes:
In AB bonding, if the current active slave becomes disabled, a
replacement slave is arbitrarily picked from the remaining set of
enabled slaves. This commit adds the concept of a "primary" slave: an
interface that will always be (or become) the current active slave if
it is enabled.
The rationale for this functionality is to allow the designation of a
preferred interface for a given bond. For example:
1. Bond is created with interfaces p1 (primary) and p2, both enabled.
2. p1 becomes the current active slave (because it was designated as
the primary).
3. Later, p1 fails/becomes disabled.
4. p2 is chosen to become the current active slave.
5. Later, p1 becomes re-enabled.
6. p1 is chosen to become the current active slave (because it was
designated as the primary).
Note that p1 becomes the active slave once it becomes re-enabled, even
if nothing has happened to p2.
This "primary" concept exists in Linux kernel network interface
bonding, but did not previously exist in OVS bonding.
Only one primary slave inteface is supported per bond, and is only
supported for active/backup bonding.
The primary slave interface is designated via
"other_config:bond-primary" when creating a bond.
Signed-off-by: Jeff Squyres <jsquyres at cisco.com<mailto:jsquyres at cisco.com>>
---
Looks good to me. Please fix the test failure I flagged below. Then
feel free to re-submit.
When the test failure is addressed,
Acked-by: Aaron Conole <aconole at redhat.com<mailto:aconole at redhat.com>>
v2:
- Added "ovs-vsctl bond/show" label when primary interface is no
longer enslaved
- Fixed strcmp() usage nits
- Moved "other_config:bond-primary" docs to the "Bonding
Configuration" group
- Expanded commit message
- Added more test cases (including one for when the primary interface
is no longer enslaved)
ofproto/bond.c | 66 ++++++++++++++++++++-
ofproto/bond.h | 1 +
tests/lacp.at<http://lacp.at> | 1 +
tests/ofproto-dpif.at<http://ofproto-dpif.at> | 133 +++++++++++++++++++++++++++++++++++++++++-
vswitchd/bridge.c | 5 ++
vswitchd/vswitch.xml | 8 +++
6 files changed, 212 insertions(+), 2 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index 405202fb6..4acead26c 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -93,6 +93,7 @@ struct bond_slave {
/* Link status. */
bool enabled; /* May be chosen for flows? */
bool may_enable; /* Client considers this slave bondable. */
+ bool is_primary; /* This slave is preferred over others. */
long long delay_expires; /* Time after which 'enabled' may change. */
/* Rebalancing info. Used only by bond_rebalance(). */
@@ -126,6 +127,7 @@ struct bond {
enum lacp_status lacp_status; /* Status of LACP negotiations. */
bool bond_revalidate; /* True if flows need revalidation. */
uint32_t basis; /* Basis for flow hash function. */
+ char *primary; /* Name of the primary slave interface. */
/* SLB specific bonding info. */
struct bond_entry *hash; /* An array of BOND_BUCKETS elements. */
@@ -241,6 +243,7 @@ bond_create(const struct bond_settings *s, struct ofproto_dpif *ofproto)
bond->active_slave_mac = eth_addr_zero;
bond->active_slave_changed = false;
+ bond->primary = NULL;
bond_reconfigure(bond, s);
return bond;
@@ -290,6 +293,7 @@ bond_unref(struct bond *bond)
update_recirc_rules__(bond);
hmap_destroy(&bond->pr_rule_ops);
+ free(bond->primary);
free(bond->name);
free(bond);
}
@@ -459,6 +463,31 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
bond->bond_revalidate = false;
}
+ /*
+ * If a primary interface is set on the new settings:
+ * 1. If the bond has no primary previously set, save it and
+ * revalidate.
+ * 2. If the bond has a different primary previously set, save the
+ * new one and revalidate.
+ * 3. If the bond has the same primary previously set, do nothing.
+ */
+ if (s->primary) {
+ bool changed = false;
+ if (bond->primary) {
+ if (strcmp(bond->primary, s->primary)) {
+ free(bond->primary);
+ changed = true;
+ }
+ } else {
+ changed = true;
+ }
+
+ if (changed) {
+ bond->primary = xstrdup(s->primary);
+ revalidate = true;
+ }
+ }
+
if (bond->balance != BM_AB) {
if (!bond->recirc_id) {
bond->recirc_id = recirc_alloc_id(bond->ofproto);
@@ -549,6 +578,12 @@ bond_slave_register(struct bond *bond, void *slave_,
slave->name = xstrdup(netdev_get_name(netdev));
bond->bond_revalidate = true;
+ if (bond->primary && !strcmp(bond->primary, slave->name)) {
+ slave->is_primary = true;
+ } else {
+ slave->is_primary = false;
+ }
+
slave->enabled = false;
bond_enable_slave(slave, netdev_get_carrier(netdev));
}
@@ -644,6 +679,7 @@ bond_run(struct bond *bond, enum lacp_status lacp_status)
{
struct bond_slave *slave;
bool revalidate;
+ struct bond_slave *primary;
ovs_rwlock_wrlock(&rwlock);
if (bond->lacp_status != lacp_status) {
@@ -659,11 +695,19 @@ bond_run(struct bond *bond, enum lacp_status lacp_status)
}
/* Enable slaves based on link status and LACP feedback. */
+ primary = NULL;
HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
bond_link_status_update(slave);
slave->change_seq = seq_read(connectivity_seq_get());
+
+ /* Discover if there is an active slave marked "primary". */
+ if (bond->balance == BM_AB && slave->is_primary && slave->enabled) {
+ primary = slave;
+ }
}
- if (!bond->active_slave || !bond->active_slave->enabled) {
+
+ if (!bond->active_slave || !bond->active_slave->enabled ||
+ (primary && bond->active_slave != primary)) {
bond_choose_active_slave(bond);
}
@@ -1393,6 +1437,19 @@ bond_print_details(struct ds *ds, const struct bond *bond)
ds_put_format(ds, "lacp_fallback_ab: %s\n",
bond->lacp_fallback_ab ? "true" : "false");
+ bool found_primary = false;
+ HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
+ if (slave->is_primary) {
+ found_primary = true;
+ }
+ }
+
+ if (bond->balance == BM_AB) {
+ ds_put_format(ds, "primary: %s%s\n",
+ bond->primary ? bond->primary : "<none>",
+ found_primary ? "" : " (no such slave)");
Should be (found_primary && bond->primary) ? "" : " (no such slave)"
Okay, I should not do coffee-less reviews ever.
(!found_primary && bond->primary) ? " (no such slave)" : ""
I think should cover it. The case we're trying to print is only when:
1. We have a configured primary
AND
2. We didn't find it
Otherwise, it should be blank.
Awesome; thanks. I just sent v3 of the patch with this change.
The test cases LGTM.
+ }
+
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);
@@ -1862,6 +1919,13 @@ bond_choose_slave(const struct bond *bond)
{
struct bond_slave *slave, *best;
+ /* If there's a primary and it's active, return that. */
+ HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
+ if (slave->is_primary && slave->enabled) {
+ return slave;
+ }
+ }
+
/* Find the last active slave. */
slave = bond_find_slave_by_mac(bond, bond->active_slave_mac);
if (slave && slave->enabled) {
diff --git a/ofproto/bond.h b/ofproto/bond.h
index e7c3d9bc3..3a923dcfa 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -45,6 +45,7 @@ struct bond_settings {
/* Balancing configuration. */
enum bond_mode balance;
+ const char *primary; /* For AB balance, primary interface name. */
int rebalance_interval; /* Milliseconds between rebalances.
Zero to disable rebalancing. */
diff --git a/tests/lacp.at<http://lacp.at> b/tests/lacp.at<http://lacp.at>
index 7b460d7be..696ffc6d4 100644
--- a/tests/lacp.at<http://lacp.at>
+++ b/tests/lacp.at<http://lacp.at>
@@ -125,6 +125,7 @@ updelay: 0 ms
downdelay: 0 ms
lacp_status: negotiated
lacp_fallback_ab: false
+primary: <none>
This test will fail (need to add the no such slave notice).
Actually, it doesn't make sense to add no such slave here - so the if
check above needs to be fixed.
active slave mac: 00:00:00:00:00:00(none)
slave p1: disabled
diff --git a/tests/ofproto-dpif.at<http://ofproto-dpif.at> b/tests/ofproto-dpif.at<http://ofproto-dpif.at>
index d444cf084..06b940f1a 100644
--- a/tests/ofproto-dpif.at<http://ofproto-dpif.at>
+++ b/tests/ofproto-dpif.at<http://ofproto-dpif.at>
@@ -29,7 +29,137 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
-AT_SETUP([ofproto-dpif - active-backup bonding])
+AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
+
+# Create br0 with interfaces p1, p2 and p7, creating bond0 with p1 and
+# p2 (p1 as primary) and br1 with interfaces p3, p4 and p8.
+# toggle p1,p2 of bond0 up and down to test bonding in active-backup mode.
+# With p1 down and p2 up/active, bring p1 back up. Since p1 is the primary,
+# it should become active.
+OVS_VSWITCHD_START(
+ [add-bond br0 bond0 p1 p2 bond_mode=active-backup \
+ other_config:bond-primary=p1 -- \
+ 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 other-config:datapath-id=1234 \
+ fail-mode=secure -- \
+ add-port br1 p3 -- set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock ofport_request=3 -- \
+ add-port br1 p4 -- 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([test -n "`ovs-appctl bond/show | grep 'primary: p1'`"])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+AT_CHECK([ovs-ofctl add-flow br1 action=normal])
+ovs-appctl netdev-dummy/set-admin-state up
+ovs-appctl time/warp 100
+ovs-appctl netdev-dummy/set-admin-state p2 down
+ovs-appctl time/stop
+ovs-appctl time/warp 100
+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 p7 'in_port(7),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3,dst=10.0.0.4,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+ovs-appctl time/warp 100
+ovs-appctl netdev-dummy/set-admin-state p2 up
+ovs-appctl netdev-dummy/set-admin-state p1 down
+wovs-appctl time/warp 100
+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:0d),eth_type(0x0800),ipv4(src=10.0.0.5,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 p7 'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0e),eth_type(0x0800),ipv4(src=10.0.0.6,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+ovs-appctl time/warp 200 100
+sleep 1
+AT_CHECK([grep 'in_port([[348]])' ovs-vswitchd.log | filter_flow_install | strip_xout], [0], [dnl
+recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(frag=no), actions: <del>
+recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(frag=no), actions: <del>
+recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0d),eth_type(0x0800),ipv4(frag=no), actions: <del>
+recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0e),eth_type(0x0800),ipv4(frag=no), actions: <del>
+recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8035), actions: <del>
+recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8035), actions: <del>
+])
+
+ovs-appctl netdev-dummy/set-admin-state p1 up
+AT_CHECK([ovs-appctl bond/show | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC], [0], [dnl
+---- bond0 ----
+bond_mode: active-backup
+bond may use recirculation: no, <del>
+bond-hash-basis: 0
+updelay: 0 ms
+downdelay: 0 ms
+lacp_status: off
+lacp_fallback_ab: false
+primary: p1
+<active slave mac del>
+
+slave p1: enabled
+ active slave
+ may_enable: true
+
+slave p2: enabled
+ may_enable: true
+
+])
+
+AT_CHECK([test -n "`ovs-appctl bond/show | grep 'primary: p1'`"])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - active-backup bonding (primary validation)])
+# Make a switch with 3 ports in a bond, so that when we delete one of
+# the ports from the bond, there are still 2 ports left and the bond
+# remains functional.
+OVS_VSWITCHD_START(
+ [add-bond br0 bond0 p1 p2 p3 bond_mode=active-backup \
+ other_config:bond-primary=p1 -- \
+ 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 -- \
+ set interface p3 type=dummy options:pstream=punix:$OVS_RUNDIR/p3.sock ofport_request=3 -- \
+ add-port br0 p7 -- set interface p7 ofport_request=7 type=dummy --])
+
+dnl Make sure the initial primary interface is set
+AT_CHECK([test -n "`ovs-appctl bond/show | grep 'primary: p1'`"])
+
+dnl Down the primary interface and verify that we switched. Then
+dnl bring the primary back and verify that we switched back to the
+dnl primary.
+ovs-appctl netdev-dummy/set-admin-state p1 down
+AT_CHECK([test -n "`ovs-appctl bond/show | fgrep 'slave p1: disabled'`"])
+ovs-appctl netdev-dummy/set-admin-state p1 up
+AT_CHECK([ovs-appctl bond/show | STRIP_RECIRC_ID | STRIP_ACTIVE_SLAVE_MAC], [0], [dnl
+---- bond0 ----
+bond_mode: active-backup
+bond may use recirculation: no, <del>
+bond-hash-basis: 0
+updelay: 0 ms
+downdelay: 0 ms
+lacp_status: off
+lacp_fallback_ab: false
+primary: p1
+<active slave mac del>
+
+slave p1: enabled
+ active slave
+ may_enable: true
+
+slave p2: enabled
+ may_enable: true
+
+slave p3: enabled
+ may_enable: true
+
+])
+
+dnl Now delete the primary and verify that the output shows that the
+dnl primary is no longer enslaved
+ovs-vsctl --id=@p1 get Interface p1 -- remove Port bond0 interfaces @p1
+AT_CHECK([test -n "`ovs-appctl bond/show | fgrep 'primary: p1 (no such slave)'`"])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - active-backup bonding (without primary)])
# Create br0 with interfaces p1, p2 and p7, creating bond0 with p1 and p2
# and br1 with interfaces p3, p4 and p8.
# toggle p1,p2 of bond0 up and down to test bonding in active-backup mode.
@@ -46,6 +176,7 @@ OVS_VSWITCHD_START(
add-port br1 p4 -- 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([test -n "`ovs-appctl bond/show | grep 'primary: <none>'`"])
AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
AT_CHECK([ovs-ofctl add-flow br0 action=normal])
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index fe73c38d4..5f30b7737 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -4563,6 +4563,11 @@ port_configure_bond(struct port *port, struct bond_settings *s)
port->name);
}
+ s->primary = NULL;
+ if (s->balance == BM_AB) {
+ s->primary = smap_get(&port->cfg->other_config, "bond-primary");
+ }
+
miimon_interval = smap_get_int(&port->cfg->other_config,
"bond-miimon-interval", 0);
if (miimon_interval <= 0) {
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 6d334370d..a2a26e849 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1994,6 +1994,14 @@
<code>active-backup</code>.
</column>
+ <column name="other_config" key="bond-primary"
+ type='{"type": "string"}'>
+ If a slave interface with this name exists in the bond and
+ is up, it will be made active. Relevant only when <ref
+ column="other_config" key="bond-mode"/> is
+ <code>active-backup</code>.
+ </column>
+
<group title="Link Failure Detection">
<p>
An important part of link bonding is detecting that links are down so
--
Jeff Squyres
jsquyres at cisco.com<mailto:jsquyres at cisco.com>
More information about the dev
mailing list