On Apr 22, 2020, at 6:36 PM, Aaron Conole <acon...@redhat.com> wrote: > > "Jeff Squyres (jsquyres)" <jsquy...@cisco.com> writes: > >> On Apr 20, 2020, at 9:22 AM, Aaron Conole <acon...@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 > -- Jeff Squyres jsquy...@cisco.com _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev