This doesn't constitute a full review, but I have a couple of comments.
Can you describe a realistic situation in which a user configured
their hypervisor to run LACP, the negotiation failed, and the correct
behavior is to forward traffic despite a clear problem in their
network config? At best, the upstream switch is dead or missing, at
worst it's completely misconfigured and we could be doing more harm
than good. In short, I don't understand the use case for this option.
Assuming we decide to optionally fall back. I'd prefer we fall back
to active-backup instead of SLB which is far more dangerous.
Please add your signed-off-by to this patch.
Ethan
On Fri, Jun 28, 2013 at 9:58 AM, Ravi Kondamuru
<ravi.kondam...@citrix.com> wrote:
> Commit bdebeece5 (lacp: Require successful LACP negotiations when
> configured.) makes successful LACP negotiation mandatory.
> This change makes it configurable. The user has the option to
> set other-config:lacp-fallback-slb to true if pre 1.5.0 behavior
> is needed. The switch will fallback to balance-slb when LACP
> negotiation fails or is unsupported by the partner switch. The
> default is false, requiring a successful LACP negotiation (which is
> the current behavior).
>
> Signed-off-by: Dominic Curran <dominic.cur...@citrix.com>
> ---
> 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
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
