This looks pretty much on track. Here's my code review. Please fix
up the changes mentioned below and resend it.
The first line of the commit message needs period. Could we change it
to "bond: Use active-backup mode on LACP failure." to be more similar
to the rest of our commit messages?
There's no need to mention git.openvswitch.org in the commit message,
just saying the commit ID and the title is sufficient.
The first paragraph has a redundant space before the period.
The patch adds some trailing whitespace. Please remove it.
The added comments in various structs needs capitalization a periods.
All of the if statements added in the patch should have opening and
closing curly braces as required by the coding style.
"require successful LACP negotiated" -> "require successful LACP negotiations"
The code which has been added to choose_output_slave() is really
unreadable. I really don't like the use of "-1" in particular. Could
we replace the ternary operator with a series of if statements which
are more "obviously correct"?
Thanks,
Ethan
On Wed, Oct 30, 2013 at 1:06 PM, Ravi Kondamuru
<ravi.kondam...@citrix.com> wrote:
> Commit bdebeece5 on git.openvswitch.org (lacp: Require successful LACP
> negotiations when configured.) makes successful LACP negotiation
> mandatory for the bond to come UP. This patch provides a configuration
> option to bring up the bond by falling back to active-backup mode on
> LACP negotiation failure .
>
> Several of the physical switches that support LACP block all traffic for ports
> that are configured to use LACP, until LACP is negotiated with the host. When
> configuring a LACP bond on a OVS host (eg: XenServer), this means that there
> will be an interruption of the network connectivity between the time the ports
> on the physical switch and the bond on the OVS host are configured. The
> interruption may be relatively long, if different people are responsible for
> managing the switches and the OVS host.
>
> Such network connectivity failure can be avoided if LACP can be configured on
> the OVS host before configuring the physical switch, and having the OVS host
> fall back to a bond mode (active-backup) till the physical switch LACP
> configuration is complete. An option "lacp-fallback-ab" is introduced with
> this
> patch to provide such behavior on openvswitch.
>
> Signed-off-by: Ravi Kondamuru <ravi.kondam...@citrix.com>
> Signed-off-by: Dominic Curran <dominic.cur...@citrix.com>
> ---
> lib/bond.c | 47 +++++++++++++++++++++++++++++++----------------
> lib/bond.h | 1 +
> lib/lacp.c | 16 ++++++++++++++--
> lib/lacp.h | 1 +
> vswitchd/INTERNALS | 14 ++++++++++++++
> vswitchd/bridge.c | 18 ++++++++++++++++++
> vswitchd/vswitch.xml | 19 +++++++++++++++++--
> 7 files changed, 96 insertions(+), 20 deletions(-)
>
> diff --git a/lib/bond.c b/lib/bond.c
> index 5be1bae..107b3e5 100644
> --- a/lib/bond.c
> +++ b/lib/bond.c
> @@ -99,6 +99,7 @@ struct bond {
>
> /* Legacy compatibility. */
> long long int next_fake_iface_update; /* LLONG_MAX if disabled. */
> + bool lacp_fallback_ab; /* fallback to active-backup on lacp failure */
>
> atomic_int ref_cnt;
> };
> @@ -258,6 +259,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_ab != s->lacp_fallback_ab_cfg) {
> + bond->lacp_fallback_ab = s->lacp_fallback_ab_cfg;
> + revalidate = true;
> + }
> +
> if (bond->rebalance_interval != s->rebalance_interval) {
> bond->rebalance_interval = s->rebalance_interval;
> revalidate = true;
> @@ -491,8 +497,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_ab && bond->lacp_status == LACP_CONFIGURED))
> && bond->active_slave;
> }
>
> @@ -585,13 +592,14 @@ 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_ab is enabled. */
> switch (bond->lacp_status) {
> case LACP_NEGOTIATED:
> verdict = slave->enabled ? BV_ACCEPT : BV_DROP;
> goto out;
> case LACP_CONFIGURED:
> - goto out;
> + if (!bond->lacp_fallback_ab)
> + goto out;
> case LACP_DISABLED:
> break;
> }
> @@ -604,6 +612,14 @@ bond_check_admissibility(struct bond *bond, const void
> *slave_,
> }
>
> switch (bond->balance) {
> + case BM_TCP:
> + /* TCP balanced bonds require successful LACP negotiated. Based on
> the
> + * above check, LACP is off or lacp_fallback_ab is true on this bond.
> + * If lacp_fallback_ab is true fall through to BM_AB case else, we
> + * drop all incoming traffic. */
> + if (!bond->lacp_fallback_ab)
> + goto out;
> +
> case BM_AB:
> /* Drop all packets which arrive on backup slaves. This is similar
> to
> * how Linux bonding handles active-backup bonds. */
> @@ -618,12 +634,6 @@ bond_check_admissibility(struct bond *bond, const void
> *slave_,
> verdict = BV_ACCEPT;
> goto out;
>
> - 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. */
> - goto out;
> -
> case BM_SLB:
> /* Drop all packets for which we have learned a different input port,
> * because we probably sent the packet on one slave and got it back
> on
> @@ -1416,14 +1426,19 @@ choose_output_slave(const struct bond *bond, const
> struct flow *flow,
> struct flow_wildcards *wc, uint16_t vlan)
> {
> struct bond_entry *e;
> + int balance;
>
> - if (bond->lacp_status == LACP_CONFIGURED) {
> - /* LACP has been configured on this bond but negotiations were
> - * unsuccussful. Drop all traffic. */
> - return NULL;
> - }
> + balance = (bond->lacp_status == LACP_CONFIGURED)?
> + ((bond->lacp_fallback_ab &&
> + (bond->balance==BM_AB || bond->balance==BM_TCP))?BM_AB:-1):
> + bond->balance;
> +
> + switch (balance) {
> + case -1:
> + /* LACP has been configured on this bond but negotiations were
> + * unsuccussful and fallback not configured. Drop all traffic. */
> + return NULL;
>
> - switch (bond->balance) {
> case BM_AB:
> return bond->active_slave;
>
> diff --git a/lib/bond.h b/lib/bond.h
> index f80fead..e8725ee 100644
> --- a/lib/bond.h
> +++ b/lib/bond.h
> @@ -53,6 +53,7 @@ struct bond_settings {
>
> /* Legacy compatibility. */
> bool fake_iface; /* Update fake stats for netdev 'name'? */
> + bool lacp_fallback_ab_cfg; /* fallback to active-backup on lacp failure
> */
> };
>
> /* Program startup. */
> diff --git a/lib/lacp.c b/lib/lacp.c
> index 5421e2a..d5c2a2a 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -102,6 +102,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_ab; /* True if fallback to active-backup on lacp failure */
>
> atomic_int ref_cnt;
> };
> @@ -283,6 +284,12 @@ lacp_configure(struct lacp *lacp, const struct
> lacp_settings *s)
>
> lacp->active = s->active;
> lacp->fast = s->fast;
> +
> + if (lacp->fallback_ab != s->fallback_ab_cfg) {
> + lacp->fallback_ab = s->fallback_ab_cfg;
> + lacp->update = true;
> + }
> +
> ovs_mutex_unlock(&mutex);
> }
>
> @@ -450,7 +457,9 @@ slave_may_enable__(struct slave *slave)
> OVS_REQUIRES(mutex)
> {
> /* 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_ab
> + && slave->status == LACP_DEFAULTED));
> }
>
> /* This function should be called before enabling 'slave_' to send or receive
> @@ -587,6 +596,8 @@ lacp_update_attached(struct lacp *lacp)
> OVS_REQUIRES(mutex)
> }
>
> if (slave->status == LACP_DEFAULTED) {
> + if (lacp->fallback_ab)
> + slave->attached = true;
> continue;
> }
>
> @@ -603,7 +614,8 @@ lacp_update_attached(struct lacp *lacp)
> OVS_REQUIRES(mutex)
>
> if (lead) {
> HMAP_FOR_EACH (slave, node, &lacp->slaves) {
> - if (lead->partner.key != slave->partner.key
> + if ((lacp->fallback_ab && 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 89b0e0a..330949b 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_ab_cfg; /* fallback to BM_SLB on lacp failure */
> };
>
> void lacp_init(void);
> diff --git a/vswitchd/INTERNALS b/vswitchd/INTERNALS
> index 2b48c9b..e25cd65 100644
> --- a/vswitchd/INTERNALS
> +++ b/vswitchd/INTERNALS
> @@ -143,6 +143,20 @@ LACP bonding requires the remote switch to implement
> LACP, but it is
> otherwise very simple in that, after LACP negotiation is complete,
> there is no need for special handling of received packets.
>
> +Several of the physical switches that support LACP block all traffic
> +for ports that are configured to use LACP, until LACP is negotiated with
> +the host. When configuring a LACP bond on a OVS host (eg: XenServer),
> +this means that there will be an interruption of the network connectivity
> +between the time the ports on the physical switch and the bond on the OVS
> +host are configured. The interruption may be relatively long, if different
> +people are responsible for managing the switches and the OVS host.
> +
> +Such network connectivity failure can be avoided if LACP can be configured
> +on the OVS host before configuring the physical switch, and having
> +the OVS host fall back to a bond mode (active-backup) till the physical
> +switch LACP configuration is complete. An option "lacp-fallback-ab" exists to
> +provide such behavior on openvswitch.
> +
> Active Backup Bonding
> ---------------------
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index ec3633c..7d5f87d 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -235,6 +235,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_ab_cfg(struct port *);
>
> static void reconfigure_system_stats(const struct ovsrec_open_vswitch *);
> static void run_system_stats(void);
> @@ -3283,6 +3284,18 @@ enable_lacp(struct port *port, bool *activep)
> }
> }
>
> +static bool
> +set_lacp_fallback_ab_cfg(struct port *port)
> +{
> + const char *lacp_fallback_s;
> +
> + lacp_fallback_s = smap_get(&port->cfg->other_config, "lacp-fallback-ab");
> + 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)
> {
> @@ -3321,6 +3334,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_ab_cfg = set_lacp_fallback_ab_cfg(port);
> +
> return s;
> }
>
> @@ -3409,6 +3425,8 @@ port_configure_bond(struct port *port, struct
> bond_settings *s)
>
> s->fake_iface = port->cfg->bond_fake_iface;
>
> + s->lacp_fallback_ab_cfg = set_lacp_fallback_ab_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 4ad0d63..f2a1b1e 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -941,7 +941,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-ab is true, then
> <code>active-backup</code>
> + mode is used:
> </p>
>
> <dl>
> @@ -1031,7 +1033,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-ab is set to true.
> + Defaults to <code>off</code> if unset.
> </column>
>
> <column name="other_config" key="lacp-system-id">
> @@ -1059,6 +1062,18 @@
> rate of once every 30 seconds.
> </p>
> </column>
> +
> + <column name="other_config" key="lacp-fallback-ab"
> + 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
> + active-backup. 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, the bond will use LACP.
> + </p>
> + </column>
> </group>
>
> <group title="Rebalancing Configuration">
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
