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