| Message ID | 20200323200223.13599-1-jsquyres@cisco.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] AB bonding: Add "primary" interface concept | expand |
I haven't seen any comments on this patch yet; just wanted to make sure it didn't get overlooked. Thanks! > On Mar 23, 2020, at 4:02 PM, Jeff Squyres <jsquyres@cisco.com> wrote: > > A "primary" slave interface will always become active if it is > enabled. The "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@cisco.com> > --- > 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) { > + 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) { > + 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,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>"); > + } > + > 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> > + > <column name="bond_updelay"> > <p> > The number of milliseconds for which the link must stay up on an > -- > 2.22.0 >
Greetings all. I posted this patch about two week ago and haven't gotten any response yet. This is a patch for a feature we're using internally and would like to get upstream, if possible. Could someone have a look? Many thanks! > On Mar 23, 2020, at 4:02 PM, Jeff Squyres <jsquyres@cisco.com> wrote: > > A "primary" slave interface will always become active if it is > enabled. The "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@cisco.com> > --- > 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) { > + 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) { > + 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,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>"); > + } > + > 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> > + > <column name="bond_updelay"> > <p> > The number of milliseconds for which the link must stay up on an > -- > 2.22.0 >
Greetings all. What is the process to get someone to review a patch like the one below? We're using this patch internally (it's a feature that Linux bridges have), and would be happy to do whatever is required to upstream it. Thanks! > On Mar 23, 2020, at 4:02 PM, Jeff Squyres <jsquyres@cisco.com> wrote: > > A "primary" slave interface will always become active if it is > enabled. The "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@cisco.com> > --- > 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) { > + 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) { > + 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,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>"); > + } > + > 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> > + > <column name="bond_updelay"> > <p> > The number of milliseconds for which the link must stay up on an > -- > 2.22.0 >
On 4/13/2020 11:51 AM, Jeff Squyres (jsquyres) via dev wrote: > Greetings all. > > What is the process to get someone to review a patch like the one below? We're using this patch internally (it's a feature that Linux bridges have), and would be happy to do whatever is required to upstream it. > > Thanks! Hi Jeff, I'll have a look at this today. Thanks, - Greg > > > >> On Mar 23, 2020, at 4:02 PM, Jeff Squyres <jsquyres@cisco.com> wrote: >> >> A "primary" slave interface will always become active if it is >> enabled. The "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@cisco.com> >> --- >> 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) { >> + 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) { >> + 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,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>"); >> + } >> + >> 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> >> + >> <column name="bond_updelay"> >> <p> >> The number of milliseconds for which the link must stay up on an >> -- >> 2.22.0 >> > >
On 4/14/2020 11:08 AM, Gregory Rose wrote: > > On 4/13/2020 11:51 AM, Jeff Squyres (jsquyres) via dev wrote: >> Greetings all. >> >> What is the process to get someone to review a patch like the one >> below? We're using this patch internally (it's a feature that Linux >> bridges have), and would be happy to do whatever is required to >> upstream it. >> >> Thanks! > > Hi Jeff, > > I'll have a look at this today. > > Thanks, > > - Greg > >> >> >> >>> On Mar 23, 2020, at 4:02 PM, Jeff Squyres <jsquyres@cisco.com> wrote: >>> >>> A "primary" slave interface will always become active if it is >>> enabled. The "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@cisco.com> This patch looks fine to me - I ran all bonding tests and found no regression. Passes a Travis build here: https://travis-ci.org/github/gvrose8192/ovs-experimental/builds/674988535 Tested-by: Greg Rose <gvrose8192@gmail.com> Acked-by: Greg Rose <gvrose8192@gmail.com> >>> --- >>> 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) { >>> +               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) { >>> +           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,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>"); >>> +   } >>> + >>>     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> >>> + >>>         <column name="bond_updelay"> >>>           <p> >>>             The number of milliseconds for which the link must stay >>> up on an >>> -- >>> 2.22.0 >>> >> >>
Hi Jeff, Jeff Squyres via dev <ovs-dev@openvswitch.org> writes: > A "primary" slave interface will always become active if it is > enabled. The "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@cisco.com> > --- I only did a cursory review (sorry). I plan to test this tomorrow. > 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() > + 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. > 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? > + } > + > 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. > <column name="bond_updelay"> > <p> > The number of milliseconds for which the link must stay up on an
On Apr 20, 2020, at 9:22 AM, Aaron Conole <aconole@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)" <jsquyres@cisco.com> writes: > On Apr 20, 2020, at 9:22 AM, Aaron Conole <aconole@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
On Apr 22, 2020, at 6:36 PM, Aaron Conole <aconole@redhat.com> wrote: > > "Jeff Squyres (jsquyres)" <jsquyres@cisco.com> writes: > >> On Apr 20, 2020, at 9:22 AM, Aaron Conole <aconole@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)" +1. Done. >>>> + } >>>> + >>>> 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 >
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) { + 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) { + 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,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>"); + } + 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> + <column name="bond_updelay"> <p> The number of milliseconds for which the link must stay up on an
A "primary" slave interface will always become active if it is enabled. The "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@cisco.com> --- 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(-)