[ovs-dev] [PATCH] AB bonding: Add "primary" interface concept
Aaron Conole
aconole at redhat.com
Wed Apr 22 22:36:21 UTC 2020
"Jeff Squyres (jsquyres)" <jsquyres at cisco.com> writes:
> On Apr 20, 2020, at 9:22 AM, Aaron Conole <aconole at redhat.com> wrote:
>>
>> I only did a cursory review (sorry). I plan to test this tomorrow.
>
> Awesome; thanks.
>
>>> ofproto/bond.c | 58 +++++++++++++++++++++++++++++++-
>>> ofproto/bond.h | 1 +
>>> tests/lacp.at | 1 +
>>> tests/ofproto-dpif.at | 78 ++++++++++++++++++++++++++++++++++++++++++-
>>> vswitchd/bridge.c | 5 +++
>>> vswitchd/vswitch.xml | 8 +++++
>>> 6 files changed, 149 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>>> index 405202fb6..2437246ca 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) != 0) {
>>
>> ^
>> through the codebase we typically use if(strcmp(...))
>>
>>> + 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) == 0) {
>>
>> ^ as above, !strcmp()
>
> Thanks; I'll fix both strcmps.
>
>>> + 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)) {
>>
>> I'm probably misunderstanding it, but I think this will set
>> bond->active_slave to 'primary' even for Active-Active bonds. I don't
>> think it matters much, but it's an interesting side effect.
>
> Primary will only be non-NULL when (bond->balance == BM_AB), so I
> don't think the value of primary will drive a new side-effect.
>
> Am I missing a deeper implication?
I don't think so.
>>> bond_choose_active_slave(bond);
>>> }
>>>
>>> @@ -1393,6 +1437,11 @@ 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");
>>>
>>> + if (bond->balance == BM_AB) {
>>> + ds_put_format(ds, "primary: %s\n",
>>> + bond->primary ? bond->primary : "<none>");
>>
>> What happens after I remove the 'primary' slave from the bond without
>> removing the 'other-config:bond-primary' key? I guess this would
>> display something inconsistent. Is it possible to flag that case
>> in the output so the user can get an explicit idea that the slave device
>> isn't enslaved to the bond?
>
> Good point. When I delete the primary interface ("p1") from a bond,
> it still shows up in the ovs-appctl bond/show output:
>
> ---- bond0 ----
> bond_mode: active-backup
> bond may use recirculation: no, Recirc-ID : -1
> bond-hash-basis: 0
> updelay: 0 ms
> downdelay: 0 ms
> lacp_status: off
> lacp_fallback_ab: false
> primary: p1
> active slave mac: aa:55:aa:55:00:02(p2)
>
> slave p2: enabled
> active slave
> may_enable: true
>
> slave p3: enabled
> may_enable: true
>
> Instead of emitting "primary: p1", how about "primary: p1 (not currently enslaved)"?
Bike shedding, but maybe "primary: p1 (no such slave)"
>>> + }
>>> +
>>> 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 +1911,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>
>>> 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..de499bd9a 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -29,7 +29,82 @@ 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
>>> +
>>> +])
>>> +
>>> +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 +121,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 4a74ed3ef..f20b3cb6a 100644
>>> --- a/vswitchd/vswitch.xml
>>> +++ b/vswitchd/vswitch.xml
>>> @@ -2016,6 +2016,14 @@
>>> key="bond-detect-mode"/> is <code>miimon</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>
>>> +
>>
>> This should probably be in the "Bonding Configuration" group rather than
>> "Link Failure Detection."
>>
>> I don't have much of an opinion whether it should be an other_config key
>> or whether it should have its own entry.
>
> I moved it to "Bonding Configuration", and left it as an other_config.
Awesome.
>>> <column name="bond_updelay">
>>> <p>
>>> The number of milliseconds for which the link must stay up on an
More information about the dev
mailing list