I proposed to Ethan via IM that we should do this kind of non-urgent
feature removal in a couple of steps:
- Mention on ovs-dev that we're deprecating it.
- Add to NEWS that we're deprecating it and will remove it at
some date, perhaps 6 months away.
- After that time, if no one has complained, remove it.
Another step that perhaps we should add is to log a warning or error
the first time that such a deprecated feature is used.
On Thu, Aug 16, 2012 at 05:03:36PM -0700, Justin Pettit wrote:
> I haven't looked at the patch, but do you think it's worth mentioning in NEWS?
>
> --Justin
>
>
> On Aug 16, 2012, at 3:40 PM, Ethan Jackson <[email protected]> wrote:
>
> > Stable bond modes are an obsolete attempt to replicate the
> > functionality contained in the bundle action. They are ugly and of
> > questionable usefulness, hence their removal.
> >
> > Signed-off-by: Ethan Jackson <[email protected]>
> > ---
> > lib/bond.c | 82
> > +++-------------------------------------------
> > lib/bond.h | 4 +--
> > ofproto/ofproto-dpif.c | 12 ++-----
> > ofproto/ofproto.h | 1 -
> > vswitchd/bridge.c | 22 ++-----------
> > vswitchd/vswitch.ovsschema | 6 ++--
> > vswitchd/vswitch.xml | 25 --------------
> > 7 files changed, 15 insertions(+), 137 deletions(-)
> >
> > diff --git a/lib/bond.c b/lib/bond.c
> > index 7178416..b7be705 100644
> > --- a/lib/bond.c
> > +++ b/lib/bond.c
> > @@ -74,9 +74,6 @@ struct bond_slave {
> > struct list bal_node; /* In bond_rebalance()'s 'bals' list. */
> > struct list entries; /* 'struct bond_entry's assigned here. */
> > uint64_t tx_bytes; /* Sum across 'tx_bytes' of entries. */
> > -
> > - /* BM_STABLE specific bonding info. */
> > - uint32_t stb_id; /* ID used for 'stb_slaves' ordering. */
> > };
> >
> > /* A bond, that is, a set of network devices grouped to improve performance
> > or
> > @@ -103,9 +100,6 @@ struct bond {
> > long long int next_rebalance; /* Next rebalancing time. */
> > bool send_learning_packets;
> >
> > - /* BM_STABLE specific bonding info. */
> > - tag_type stb_tag; /* Tag associated with this bond. */
> > -
> > /* Legacy compatibility. */
> > long long int next_fake_iface_update; /* LLONG_MAX if disabled. */
> >
> > @@ -146,8 +140,6 @@ bond_mode_from_string(enum bond_mode *balance, const
> > char *s)
> > *balance = BM_TCP;
> > } else if (!strcmp(s, bond_mode_to_string(BM_SLB))) {
> > *balance = BM_SLB;
> > - } else if (!strcmp(s, bond_mode_to_string(BM_STABLE))) {
> > - *balance = BM_STABLE;
> > } else if (!strcmp(s, bond_mode_to_string(BM_AB))) {
> > *balance = BM_AB;
> > } else {
> > @@ -164,8 +156,6 @@ bond_mode_to_string(enum bond_mode balance) {
> > return "balance-tcp";
> > case BM_SLB:
> > return "balance-slb";
> > - case BM_STABLE:
> > - return "stable";
> > case BM_AB:
> > return "active-backup";
> > }
> > @@ -186,7 +176,6 @@ bond_create(const struct bond_settings *s)
> > bond = xzalloc(sizeof *bond);
> > hmap_init(&bond->slaves);
> > bond->no_slaves_tag = tag_create_random();
> > - bond->stb_tag = tag_create_random();
> > bond->next_fake_iface_update = LLONG_MAX;
> >
> > bond_reconfigure(bond, s);
> > @@ -296,17 +285,12 @@ bond_slave_set_netdev__(struct bond_slave *slave,
> > struct netdev *netdev)
> > * bond. If 'slave_' already exists within 'bond' then this function
> > * reconfigures the existing slave.
> > *
> > - * 'stb_id' is used in BM_STABLE bonds to guarantee consistent slave
> > choices
> > - * across restarts and distributed vswitch instances. It should be unique
> > per
> > - * slave, and preferably consistent across restarts and reconfigurations.
> > - *
> > * 'netdev' must be the network device that 'slave_' represents. It is
> > owned
> > * by the client, so the client must not close it before either
> > unregistering
> > * 'slave_' or destroying 'bond'.
> > */
> > void
> > -bond_slave_register(struct bond *bond, void *slave_, uint32_t stb_id,
> > - struct netdev *netdev)
> > +bond_slave_register(struct bond *bond, void *slave_, struct netdev *netdev)
> > {
> > struct bond_slave *slave = bond_slave_lookup(bond, slave_);
> >
> > @@ -324,11 +308,6 @@ bond_slave_register(struct bond *bond, void *slave_,
> > uint32_t stb_id,
> > bond_enable_slave(slave, netdev_get_carrier(netdev), NULL);
> > }
> >
> > - if (slave->stb_id != stb_id) {
> > - slave->stb_id = stb_id;
> > - bond->bond_revalidate = true;
> > - }
> > -
> > bond_slave_set_netdev__(slave, netdev);
> >
> > free(slave->name);
> > @@ -432,18 +411,11 @@ bond_run(struct bond *bond, struct tag_set *tags,
> > enum lacp_status lacp_status)
> >
> > if (bond->bond_revalidate) {
> > bond->bond_revalidate = false;
> > -
> > bond_entry_reset(bond);
> > - if (bond->balance != BM_STABLE) {
> > - struct bond_slave *slave;
> > -
> > - HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> > - tag_set_add(tags, slave->tag);
> > - }
> > - } else {
> > - tag_set_add(tags, bond->stb_tag);
> > - }
> > tag_set_add(tags, bond->no_slaves_tag);
> > + HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> > + tag_set_add(tags, slave->tag);
> > + }
> > }
> >
> > /* Invalidate any tags required by */
> > @@ -488,7 +460,6 @@ static bool
> > may_send_learning_packets(const struct bond *bond)
> > {
> > return bond->lacp_status == LACP_DISABLED
> > - && bond->balance != BM_STABLE
> > && bond->active_slave;
> > }
> >
> > @@ -613,9 +584,6 @@ bond_check_admissibility(struct bond *bond, const void
> > *slave_,
> > * exception is if we locked the learning table to avoid
> > reflections on
> > * bond slaves. */
> > return BV_DROP_IF_MOVED;
> > -
> > - case BM_STABLE:
> > - return BV_ACCEPT;
> > }
> >
> > NOT_REACHED();
> > @@ -639,7 +607,7 @@ bond_choose_output_slave(struct bond *bond, const
> > struct flow *flow,
> > {
> > struct bond_slave *slave = choose_output_slave(bond, flow, vlan);
> > if (slave) {
> > - *tags |= bond->balance == BM_STABLE ? bond->stb_tag : slave->tag;
> > + *tags |= slave->tag;
> > return slave->aux;
> > } else {
> > *tags |= bond->no_slaves_tag;
> > @@ -1284,7 +1252,6 @@ bond_slave_lookup(struct bond *bond, const void
> > *slave_)
> > static void
> > bond_enable_slave(struct bond_slave *slave, bool enable, struct tag_set
> > *tags)
> > {
> > - struct bond *bond = slave->bond;
> > slave->delay_expires = LLONG_MAX;
> > if (enable != slave->enabled) {
> > slave->enabled = enable;
> > @@ -1297,10 +1264,6 @@ bond_enable_slave(struct bond_slave *slave, bool
> > enable, struct tag_set *tags)
> > VLOG_WARN("interface %s: enabled", slave->name);
> > slave->tag = tag_create_random();
> > }
> > -
> > - if (bond->balance == BM_STABLE) {
> > - bond->bond_revalidate = true;
> > - }
> > }
> > }
> >
> > @@ -1374,35 +1337,6 @@ lookup_bond_entry(const struct bond *bond, const
> > struct flow *flow,
> > return &bond->hash[bond_hash(bond, flow, vlan) & BOND_MASK];
> > }
> >
> > -/* This function uses Highest Random Weight hashing to choose an output
> > slave.
> > - * This approach only reassigns a minimal number of flows when slaves are
> > - * enabled or disabled. Unfortunately, it has O(n) performance against the
> > - * number of slaves. There exist algorithms which are O(1), but have
> > slightly
> > - * more complex implementations and require the use of memory. This may
> > need
> > - * to be reimplemented if it becomes a performance bottleneck. */
> > -static struct bond_slave *
> > -choose_stb_slave(const struct bond *bond, uint32_t flow_hash)
> > -{
> > - struct bond_slave *best, *slave;
> > - uint32_t best_hash;
> > -
> > - best = NULL;
> > - best_hash = 0;
> > - HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> > - if (slave->enabled) {
> > - uint32_t hash;
> > -
> > - hash = hash_2words(flow_hash, slave->stb_id);
> > - if (!best || hash > best_hash) {
> > - best = slave;
> > - best_hash = hash;
> > - }
> > - }
> > - }
> > -
> > - return best;
> > -}
> > -
> > static struct bond_slave *
> > choose_output_slave(const struct bond *bond, const struct flow *flow,
> > uint16_t vlan)
> > @@ -1419,9 +1353,6 @@ choose_output_slave(const struct bond *bond, const
> > struct flow *flow,
> > case BM_AB:
> > return bond->active_slave;
> >
> > - case BM_STABLE:
> > - return choose_stb_slave(bond, bond_hash_tcp(flow, vlan,
> > bond->basis));
> > -
> > case BM_TCP:
> > if (bond->lacp_status != LACP_NEGOTIATED) {
> > /* Must have LACP negotiations for TCP balanced bonds. */
> > @@ -1429,9 +1360,6 @@ choose_output_slave(const struct bond *bond, const
> > struct flow *flow,
> > }
> > /* Fall Through. */
> > case BM_SLB:
> > - if (!bond_is_balanced(bond)) {
> > - return choose_stb_slave(bond, bond_hash(bond, flow, vlan));
> > - }
> > e = lookup_bond_entry(bond, flow, vlan);
> > if (!e->slave || !e->slave->enabled) {
> > e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves),
> > diff --git a/lib/bond.h b/lib/bond.h
> > index 7329db7..6190081 100644
> > --- a/lib/bond.h
> > +++ b/lib/bond.h
> > @@ -32,7 +32,6 @@ enum lacp_status;
> > enum bond_mode {
> > BM_TCP, /* Transport Layer Load Balance. */
> > BM_SLB, /* Source Load Balance. */
> > - BM_STABLE, /* Stable. */
> > BM_AB /* Active Backup. */
> > };
> >
> > @@ -65,8 +64,7 @@ struct bond *bond_create(const struct bond_settings *);
> > void bond_destroy(struct bond *);
> >
> > bool bond_reconfigure(struct bond *, const struct bond_settings *);
> > -void bond_slave_register(struct bond *, void *slave_,
> > - uint32_t stable_id, struct netdev *);
> > +void bond_slave_register(struct bond *, void *slave_, struct netdev *);
> > void bond_slave_set_netdev(struct bond *, void *slave_, struct netdev *);
> > void bond_slave_unregister(struct bond *, const void *slave);
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index a7e85de..45c2abc 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -490,7 +490,6 @@ struct ofport_dpif {
> > struct list bundle_node; /* In struct ofbundle's "ports" list. */
> > struct cfm *cfm; /* Connectivity Fault Management, if any. */
> > tag_type tag; /* Tag associated with this port. */
> > - uint32_t bond_stable_id; /* stable_id to use as bond slave, or 0. */
> > bool may_enable; /* May be enabled in bonds. */
> > long long int carrier_seq; /* Carrier status changes. */
> >
> > @@ -1785,8 +1784,7 @@ bundle_del_port(struct ofport_dpif *port)
> >
> > static bool
> > bundle_add_port(struct ofbundle *bundle, uint32_t ofp_port,
> > - struct lacp_slave_settings *lacp,
> > - uint32_t bond_stable_id)
> > + struct lacp_slave_settings *lacp)
> > {
> > struct ofport_dpif *port;
> >
> > @@ -1813,8 +1811,6 @@ bundle_add_port(struct ofbundle *bundle, uint32_t
> > ofp_port,
> > lacp_slave_register(bundle->lacp, port, lacp);
> > }
> >
> > - port->bond_stable_id = bond_stable_id;
> > -
> > return true;
> > }
> >
> > @@ -1922,8 +1918,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
> > ok = true;
> > for (i = 0; i < s->n_slaves; i++) {
> > if (!bundle_add_port(bundle, s->slaves[i],
> > - s->lacp ? &s->lacp_slaves[i] : NULL,
> > - s->bond_stable_ids ? s->bond_stable_ids[i] :
> > 0)) {
> > + s->lacp ? &s->lacp_slaves[i] : NULL)) {
> > ok = false;
> > }
> > }
> > @@ -2023,8 +2018,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
> > }
> >
> > LIST_FOR_EACH (port, bundle_node, &bundle->ports) {
> > - bond_slave_register(bundle->bond, port, port->bond_stable_id,
> > - port->up.netdev);
> > + bond_slave_register(bundle->bond, port, port->up.netdev);
> > }
> > } else {
> > bond_destroy(bundle->bond);
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 32a00f2..825f386 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -273,7 +273,6 @@ struct ofproto_bundle_settings {
> > bool use_priority_tags; /* Use 802.1p tag for frames in VLAN 0? */
> >
> > struct bond_settings *bond; /* Must be nonnull iff if n_slaves > 1. */
> > - uint32_t *bond_stable_ids; /* Array of n_slaves elements. */
> >
> > struct lacp_settings *lacp; /* Nonnull to enable LACP. */
> > struct lacp_slave_settings *lacp_slaves; /* Array of n_slaves elements.
> > */
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index bd8e772..7ff02b8 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -210,8 +210,7 @@ static struct port *port_lookup(const struct bridge *,
> > const char *name);
> > static void port_configure(struct port *);
> > static struct lacp_settings *port_configure_lacp(struct port *,
> > struct lacp_settings *);
> > -static void port_configure_bond(struct port *, struct bond_settings *,
> > - uint32_t *bond_stable_ids);
> > +static void port_configure_bond(struct port *, struct bond_settings *);
> > static bool port_is_synthetic(const struct port *);
> >
> > static void reconfigure_system_stats(const struct ovsrec_open_vswitch *);
> > @@ -710,11 +709,9 @@ port_configure(struct port *port)
> > /* Get bond settings. */
> > if (s.n_slaves > 1) {
> > s.bond = &bond_settings;
> > - s.bond_stable_ids = xmalloc(s.n_slaves * sizeof
> > *s.bond_stable_ids);
> > - port_configure_bond(port, &bond_settings, s.bond_stable_ids);
> > + port_configure_bond(port, &bond_settings);
> > } else {
> > s.bond = NULL;
> > - s.bond_stable_ids = NULL;
> >
> > LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> > netdev_set_miimon_interval(iface->netdev, 0);
> > @@ -728,7 +725,6 @@ port_configure(struct port *port)
> > free(s.slaves);
> > free(s.trunks);
> > free(s.lacp_slaves);
> > - free(s.bond_stable_ids);
> > }
> >
> > /* Pick local port hardware address and datapath ID for 'br'. */
> > @@ -2966,13 +2962,11 @@ iface_configure_lacp(struct iface *iface, struct
> > lacp_slave_settings *s)
> > }
> >
> > static void
> > -port_configure_bond(struct port *port, struct bond_settings *s,
> > - uint32_t *bond_stable_ids)
> > +port_configure_bond(struct port *port, struct bond_settings *s)
> > {
> > const char *detect_s;
> > struct iface *iface;
> > int miimon_interval;
> > - size_t i;
> >
> > s->name = port->name;
> > s->balance = BM_AB;
> > @@ -3024,17 +3018,7 @@ port_configure_bond(struct port *port, struct
> > bond_settings *s,
> >
> > s->fake_iface = port->cfg->bond_fake_iface;
> >
> > - i = 0;
> > LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> > - long long stable_id;
> > -
> > - stable_id = smap_get_int(&iface->cfg->other_config,
> > "bond-stable-id",
> > - 0);
> > - if (stable_id <= 0 || stable_id >= UINT32_MAX) {
> > - stable_id = iface->ofp_port;
> > - }
> > - bond_stable_ids[i++] = stable_id;
> > -
> > netdev_set_miimon_interval(iface->netdev, miimon_interval);
> > }
> > }
> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index bbfb01f..8f6fcdf 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> > @@ -1,6 +1,6 @@
> > {"name": "Open_vSwitch",
> > - "version": "6.10.0",
> > - "cksum": "3699312094 16958",
> > + "version": "6.11.0",
> > + "cksum": "854639069 16948",
> > "tables": {
> > "Open_vSwitch": {
> > "columns": {
> > @@ -130,7 +130,7 @@
> > "min": 0, "max": 1}},
> > "bond_mode": {
> > "type": {"key": {"type": "string",
> > - "enum": ["set", ["balance-tcp", "balance-slb", "active-backup",
> > "stable"]]},
> > + "enum": ["set", ["balance-tcp", "balance-slb",
> > "active-backup"]]},
> > "min": 0, "max": 1}},
> > "lacp": {
> > "type": {"key": {"type": "string",
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 7b30ce8..7021c3a 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -831,21 +831,6 @@
> > information such as destination MAC address, IP address, and TCP
> > port.
> > </dd>
> > -
> > - <dt><code>stable</code></dt>
> > - <dd>
> > - <p>Attempts to always assign a given flow to the same slave
> > - consistently. In an effort to maintain stability, no load
> > - balancing is done. Uses a similar hashing strategy to
> > - <code>balance-tcp</code>, always taking into account L3 and L4
> > - fields even if LACP negotiations are unsuccessful. </p>
> > - <p>Slave selection decisions are made based on <ref
> > table="Interface"
> > - column="other_config" key="bond-stable-id"/> if set. Otherwise,
> > - OpenFlow port number is used. Decisions are consistent across
> > all
> > - <code>ovs-vswitchd</code> instances with equivalent
> > - <ref table="Interface" column="other_config"
> > key="bond-stable-id"/>
> > - values.</p>
> > - </dd>
> > </dl>
> >
> > <p>These columns apply only to bonded ports. Their values are
> > @@ -1860,16 +1845,6 @@
> > </group>
> >
> > <group title="Bonding Configuration">
> > - <column name="other_config" key="bond-stable-id"
> > - type='{"type": "integer", "minInteger": 1}'>
> > - Used in <code>stable</code> bond mode to make slave
> > - selection decisions. Allocating <ref column="other_config"
> > - key="bond-stable-id"/> values consistently across interfaces
> > - participating in a bond will guarantee consistent slave selection
> > - decisions across <code>ovs-vswitchd</code> instances when using
> > - <code>stable</code> bonding mode.
> > - </column>
> > -
> > <column name="other_config" key="lacp-port-id"
> > type='{"type": "integer", "minInteger": 1, "maxInteger":
> > 65535}'>
> > The LACP port ID of this <ref table="Interface"/>. Port IDs are
> > --
> > 1.7.11.4
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
