[ovs-dev] [PATCH v2] AB bonding: Add "primary" interface concept
Aaron Conole
aconole at redhat.com
Mon May 4 14:27:44 UTC 2020
Aaron Conole <aconole at redhat.com> writes:
> Aaron Conole <aconole at redhat.com> writes:
>
>> Jeff Squyres via dev <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>
>>> ---
>>
>> 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>
>>
>>> 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 | 1 +
>>> tests/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.
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 b/tests/lacp.at
>>> index 7b460d7be..696ffc6d4 100644
>>> --- a/tests/lacp.at
>>> +++ b/tests/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 b/tests/ofproto-dpif.at
>>> index d444cf084..06b940f1a 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/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
More information about the dev
mailing list