Greetings. Is there an estimate on when this patch can be merged?
Thanks! > On May 22, 2020, at 5:12 PM, Jeff Squyres <jsquy...@cisco.com> wrote: > > 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 <jsquy...@cisco.com> > Reviewed-by: Aaron Conole <acon...@redhat.com> > Tested-by: Greg Rose <gvrose8...@gmail.com> > Acked-by: Greg Rose <gvrose8...@gmail.com> > --- > v7: > - After rebasing patch to head of tree, adjust test output as result > of changes from 81f71381f. > > v6: > - Style: bleep bloop. Fix use of {}. > > v5: > - Handle when bond is reconfigured to remove "bond-primary" config. > - Fix potential flapping between remaining slaves. > - Add a test to re-add a removed primary interface and make sure the > bond reflects that properly. > - Add a test to remove "bond-primary" config and make sure the bond > reflects that properly. > > v4: > - Style: bleep bloop. Trim line length below 79 characters. > > v3: > - Adjusted print logic for when the primary slave is not attached > > 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 | 75 +++++++++++++++- > ofproto/bond.h | 1 + > tests/lacp.at | 1 + > tests/ofproto-dpif.at | 199 +++++++++++++++++++++++++++++++++++++++++- > vswitchd/bridge.c | 5 ++ > vswitchd/vswitch.xml | 8 ++ > 6 files changed, 287 insertions(+), 2 deletions(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 405202fb6..4ad1a20ae 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,39 @@ 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; > + } > + } else if (bond->primary) { > + /* > + * If the new settings have no primary interface, but the > + * bond already has a primary, remove the bond's primary. > + */ > + free(bond->primary); > + bond->primary = NULL; > + revalidate = true; > + } > + > if (bond->balance != BM_AB) { > if (!bond->recirc_id) { > bond->recirc_id = recirc_alloc_id(bond->ofproto); > @@ -549,6 +586,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 +687,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 +703,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 && primary->enabled && bond->active_slave)) { > bond_choose_active_slave(bond); > } > > @@ -1393,6 +1445,20 @@ 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 && bond->primary) ? > + " (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 +1928,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 41164d735..b2d8c089f 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -29,7 +29,203 @@ 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 > +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: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:09,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8035), > 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(3),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> > +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)'`"]) > + > +dnl Now re-add the primary and verify that the output shows that the > +dnl primary is available again. > +dnl > +dnl First, get the UUIDs of the interfaces that exist on bond0. > +dnl Strip the trailing ] so that we can add a new UUID to the end. > +uuids=`ovs-vsctl get Port bond0 interfaces | sed -e 's/]//'` > +dnl Create a new port "p1" and add its UUID to the set of interfaces > +dnl on bond0. > +ovs-vsctl \ > + --id=@p1 create Interface name=p1 type=dummy > options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \ > + set Port bond0 interfaces="$uuids, @p1]" > +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 Remove the "bond-primary" config directive from the bond. > +dnl First, find the other_config values on bond0. > +other_config=`ovs-vsctl get Port bond0 other_config | sed -e 's/\(, > \)*bond-primary=p1//'` > +dnl Now re-set the other_config on bond0. > +ovs-vsctl set Port bond0 other_config="$other_config" > +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: <none> > +<active slave mac del> > + > +slave p1: enabled > + active slave > + may_enable: true > + > +slave p2: enabled > + may_enable: true > + > +slave p3: 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 +242,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 > -- > 2.22.0 > -- Jeff Squyres jsquy...@cisco.com _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev