I haven't looked at this patch closely yet. Before I do would you please rebase it against master?
Also please change it to fall back to active-backup instead of SLB. SLB is weird and dangerous, I don't want to encourage it's use further than we have to, especially in a network which is likely misconfigured. Ethan On Mon, Jul 8, 2013 at 1:32 PM, Ravi Kondamuru <[email protected]> wrote: > Commit bdebeece5 (lacp: Require successful LACP negotiations when configured.) > makes successful LACP negotiation mandatory. This patch makes it configurable. > > The primary need for this patch is to provide backwards compatibility. In the > upcoming XenServer release we are updating OpenVSwitch from 1.4.2 to 1.9.x. We > found this change in LACP behavior while testing 1.9.x. We would like to > continue to provide the 1.4.2 behavior for XenServer customers upgrading to > the > version containing 1.9.x. By making it configurable, it allows for changing it > as needed. The default in the patch is to provide the current 1.9.x behavior. > The default from XenServer commands is to provide 1.4.2 behavior so there is > no > change on upgrade. > > We internally had discussed options to make the fallback method as > Active-Backup > or user-configurable. In the interest of keeping the change small and > retaining > 1.4.2 behavior we reverted to SLB with a flag check. Fallback to SLB in this > patch is mostly existing (1.4.2) code. > > 1. When partner switch is not configured for LACP, 1.4.2 behavior allows to > maintain the bonding. We are looking to continue to support this usecase. > 2. When partner switch is misconfigured and LACP not negotiated, 1.9.x > actively > disables the bond on openvswitch. With 1.4.2, openvswitch tries to maintain > bonding, but the partner switch could turn off and disable bonding. In both > cases bond is rendered unusable. We could add lacp_status message as > configured > (currently: balance-slb) to explicitly indicate bond in fallback mode on > openvswitch. > > Signed-off-by: Ravi Kondamuru <[email protected]> > Signed-off-by: Dominic Curran <[email protected]> > --- > lib/bond.c | 41 +++++++++++++++++++++++++++-------------- > lib/bond.h | 1 + > lib/lacp.c | 15 +++++++++++++-- > lib/lacp.h | 1 + > vswitchd/bridge.c | 17 +++++++++++++++++ > vswitchd/vswitch.xml | 19 +++++++++++++++++-- > 6 files changed, 76 insertions(+), 18 deletions(-) > > diff --git a/lib/bond.c b/lib/bond.c > index 68ac068..6365cd0 100644 > --- a/lib/bond.c > +++ b/lib/bond.c > @@ -103,6 +103,7 @@ struct bond { > > /* Legacy compatibility. */ > long long int next_fake_iface_update; /* LLONG_MAX if disabled. */ > + bool lacp_fallback_slb; /* fallback to balance-slb on lacp failure */ > > /* Tag set saved for next bond_run(). This tag set is a kluge for cases > * where we can't otherwise provide revalidation feedback to the client. > @@ -238,6 +239,11 @@ bond_reconfigure(struct bond *bond, const struct > bond_settings *s) > bond->updelay = s->up_delay; > bond->downdelay = s->down_delay; > > + if (bond->lacp_fallback_slb != s->lacp_fallback_slb_cfg) { > + bond->lacp_fallback_slb = s->lacp_fallback_slb_cfg; > + revalidate = true; > + } > + > if (bond->rebalance_interval != s->rebalance_interval) { > bond->rebalance_interval = s->rebalance_interval; > revalidate = true; > @@ -463,8 +469,9 @@ bond_wait(struct bond *bond) > static bool > may_send_learning_packets(const struct bond *bond) > { > - return bond->lacp_status == LACP_DISABLED > - && (bond->balance == BM_SLB || bond->balance == BM_AB) > + return ((bond->lacp_status == LACP_DISABLED > + && (bond->balance == BM_SLB || bond->balance == BM_AB)) > + || (bond->lacp_fallback_slb && bond->lacp_status == LACP_CONFIGURED)) > && bond->active_slave; > } > > @@ -546,10 +553,12 @@ bond_check_admissibility(struct bond *bond, const void > *slave_, > * packets to them. > * > * If LACP is configured, but LACP negotiations have been unsuccessful, > we > - * drop all incoming traffic. */ > + * drop all incoming traffic except if lacp_fallback_slb is true. */ > switch (bond->lacp_status) { > case LACP_NEGOTIATED: return slave->enabled ? BV_ACCEPT : BV_DROP; > - case LACP_CONFIGURED: return BV_DROP; > + case LACP_CONFIGURED: > + if (!bond->lacp_fallback_slb) > + return BV_DROP; > case LACP_DISABLED: break; > } > > @@ -578,9 +587,11 @@ bond_check_admissibility(struct bond *bond, const void > *slave_, > > case BM_TCP: > /* TCP balanced bonds require successful LACP negotiated. Based on > the > - * above check, LACP is off on this bond. Therfore, we drop all > - * incoming traffic. */ > - return BV_DROP; > + * above check, LACP is off or lacp_fallback_slb is true on this > bond. > + * If lacp_fallback_slb is true fall through to BM_SLB case else, we > + * drop all incoming traffic. */ > + if (!bond->lacp_fallback_slb) > + return BV_DROP; > > case BM_SLB: > /* Drop all packets for which we have learned a different input port, > @@ -1359,9 +1370,9 @@ choose_output_slave(const struct bond *bond, const > struct flow *flow, > { > struct bond_entry *e; > > - if (bond->lacp_status == LACP_CONFIGURED) { > + if (bond->lacp_status == LACP_CONFIGURED && !bond->lacp_fallback_slb) { > /* LACP has been configured on this bond but negotiations were > - * unsuccussful. Drop all traffic. */ > + * unsuccussful and lacp_fallback_slb not true. Drop all traffic. */ > return NULL; > } > > @@ -1370,13 +1381,15 @@ choose_output_slave(const struct bond *bond, const > struct flow *flow, > return bond->active_slave; > > case BM_TCP: > - if (bond->lacp_status != LACP_NEGOTIATED) { > - /* Must have LACP negotiations for TCP balanced bonds. */ > + if (bond->lacp_status == LACP_NEGOTIATED) { > + if (wc) > + flow_mask_hash_fields(wc, NX_HASH_FIELDS_SYMMETRIC_L4); > + } else if (!((bond->lacp_status == LACP_CONFIGURED) > + && bond->lacp_fallback_slb)) { > + /* Must have LACP negotiations for TCP balanced bonds or LACP > + * Configured with lacp_fallback_slb set to true. */ > return NULL; > } > - if (wc) { > - flow_mask_hash_fields(wc, NX_HASH_FIELDS_SYMMETRIC_L4); > - } > /* Fall Through. */ > case BM_SLB: > if (wc) { > diff --git a/lib/bond.h b/lib/bond.h > index 306cf42..f466383 100644 > --- a/lib/bond.h > +++ b/lib/bond.h > @@ -54,6 +54,7 @@ struct bond_settings { > > /* Legacy compatibility. */ > bool fake_iface; /* Update fake stats for netdev 'name'? */ > + bool lacp_fallback_slb_cfg; /* fallback to balance-slb on lacp > failure */ > }; > > /* Program startup. */ > diff --git a/lib/lacp.c b/lib/lacp.c > index 8bc115d..e2b6e91 100644 > --- a/lib/lacp.c > +++ b/lib/lacp.c > @@ -100,6 +100,7 @@ struct lacp { > bool fast; /* True if using fast probe interval. */ > bool negotiated; /* True if LACP negotiations were successful. */ > bool update; /* True if lacp_update() needs to be called. */ > + bool fallback_slb; /* fallback to balance-slb on lacp failure */ > }; > > struct slave { > @@ -238,6 +239,11 @@ lacp_configure(struct lacp *lacp, const struct > lacp_settings *s) > > lacp->active = s->active; > lacp->fast = s->fast; > + > + if (lacp->fallback_slb != s->fallback_slb_cfg) { > + lacp->fallback_slb = s->fallback_slb_cfg; > + lacp->update = true; > + } > } > > /* Returns true if 'lacp' is configured in active mode, false if 'lacp' is > @@ -366,7 +372,9 @@ slave_may_enable__(struct slave *slave) > { > /* The slave may be enabled if it's attached to an aggregator and its > * partner is synchronized.*/ > - return slave->attached && (slave->partner.state & LACP_STATE_SYNC); > + return slave->attached && (slave->partner.state & LACP_STATE_SYNC > + || (slave->lacp && slave->lacp->fallback_slb > + && slave->status == LACP_DEFAULTED)); > } > > /* This function should be called before enabling 'slave_' to send or receive > @@ -483,6 +491,8 @@ lacp_update_attached(struct lacp *lacp) > } > > if (slave->status == LACP_DEFAULTED) { > + if (lacp->fallback_slb) > + slave->attached = true; > continue; > } > > @@ -499,7 +509,8 @@ lacp_update_attached(struct lacp *lacp) > > if (lead) { > HMAP_FOR_EACH (slave, node, &lacp->slaves) { > - if (lead->partner.key != slave->partner.key > + if ((lacp->fallback_slb && slave->status == LACP_DEFAULTED) > + || lead->partner.key != slave->partner.key > || !eth_addr_equals(lead->partner.sys_id, > slave->partner.sys_id)) { > slave->attached = false; > diff --git a/lib/lacp.h b/lib/lacp.h > index 399b39e..21f2290 100644 > --- a/lib/lacp.h > +++ b/lib/lacp.h > @@ -35,6 +35,7 @@ struct lacp_settings { > uint16_t priority; /* System priority. */ > bool active; /* Active or passive mode? */ > bool fast; /* Fast or slow probe interval. */ > + bool fallback_slb_cfg; /* fallback to BM_SLB on lacp failure > */ > }; > > void lacp_init(void); > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 4ac2b26..4c9e698 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -234,6 +234,7 @@ static struct lacp_settings *port_configure_lacp(struct > port *, > struct lacp_settings *); > static void port_configure_bond(struct port *, struct bond_settings *); > static bool port_is_synthetic(const struct port *); > +static bool set_lacp_fallback_slb_cfg(struct port *); > > static void reconfigure_system_stats(const struct ovsrec_open_vswitch *); > static void run_system_stats(void); > @@ -3264,6 +3265,17 @@ enable_lacp(struct port *port, bool *activep) > } > } > > +static bool set_lacp_fallback_slb_cfg(struct port *port) > +{ > + const char *lacp_fallback_s; > + > + lacp_fallback_s = smap_get(&port->cfg->other_config, > "lacp-fallback-slb"); > + if (lacp_fallback_s && !strcmp(lacp_fallback_s, "true")) > + return true; > + > + return false; > +} > + > static struct lacp_settings * > port_configure_lacp(struct port *port, struct lacp_settings *s) > { > @@ -3302,6 +3314,9 @@ port_configure_lacp(struct port *port, struct > lacp_settings *s) > > lacp_time = smap_get(&port->cfg->other_config, "lacp-time"); > s->fast = lacp_time && !strcasecmp(lacp_time, "fast"); > + > + s->fallback_slb_cfg = set_lacp_fallback_slb_cfg(port); > + > return s; > } > > @@ -3390,6 +3405,8 @@ port_configure_bond(struct port *port, struct > bond_settings *s) > > s->fake_iface = port->cfg->bond_fake_iface; > > + s->lacp_fallback_slb_cfg = set_lacp_fallback_slb_cfg(port); > + > LIST_FOR_EACH (iface, port_elem, &port->ifaces) { > netdev_set_miimon_interval(iface->netdev, miimon_interval); > } > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 12780d6..2e894c5 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -925,7 +925,9 @@ > > <p> > The following modes require the upstream switch to support 802.3ad > with > - successful LACP negotiation: > + successful LACP negotiation. If LACP negotiation fails and > + other-config:lacp-fallback-slb is true, then <code>balance-slb</code> > + style flow hashing is used: > </p> > > <dl> > @@ -1015,7 +1017,8 @@ > in LACP negotiations initiated by a remote switch, but not allowed > to > initiate such negotiations themselves. If LACP is enabled on a > port > whose partner switch does not support LACP, the bond will be > - disabled. Defaults to <code>off</code> if unset. > + disabled, unless other-config:lacp-fallback-slb is set to true. > + Defaults to <code>off</code> if unset. > </column> > > <column name="other_config" key="lacp-system-id"> > @@ -1043,6 +1046,18 @@ > rate of once every 30 seconds. > </p> > </column> > + > + <column name="other_config" key="lacp-fallback-slb" > + type='{"type": "boolean"}'> > + <p> > + Determines the behavior of openvswitch bond in LACP mode. If > + the partner switch does not support LACP, setting this option > + to <code>true</code> allows openvswitch to fallback to > + balance-slb. If the option is set to <code>false</code>, the > + bond will be disabled. In both the cases, once the partner switch > + is configured to LACP mode then the bond will use LACP. > + </p> > + </column> > </group> > > <group title="Rebalancing Configuration"> > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
