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? >> 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 jsquy...@cisco.com _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev