[ovs-dev] [PATCH] AB bonding: Add "primary" interface concept

Jeff Squyres (jsquyres) jsquyres at cisco.com
Wed Apr 22 19:03:09 UTC 2020


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?

>>         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)"?

>> +    }
>> +
>>     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.

>>         <column name="bond_updelay">
>>           <p>
>>             The number of milliseconds for which the link must stay up on an

-- 
Jeff Squyres
jsquyres at cisco.com



More information about the dev mailing list