-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libteam] Add fallback support for single-member-port LAG #1118
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9dce7cc
[libteam] Add fallback support for single-member-port LAG
hzheng5 28f3439
[libteam] resolve the merge conflict
hzheng5 6d4f830
[team] Add lacp fallback config to teamd.j2 template
hzheng5 4d5358d
[teamd] Resolve config conflict between fallback and minlink
hzheng5 381d263
[teamd] Only enable fallback if it is single-member-port LAG
hzheng5 a16d686
[teamd] Removing the admin status check in lacp_port_link_update
hzheng5 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
151 changes: 151 additions & 0 deletions
151
src/libteam/0004-libteam-Add-lacp-fallback-support-for-single-member-.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| From 08f4c83a6960fd922d1baac2293a95250f36fa96 Mon Sep 17 00:00:00 2001 | ||
| From: Haiyang Zheng <haiyang.z@alibaba-inc.com> | ||
| Date: Mon, 6 Nov 2017 00:24:00 -0800 | ||
| Subject: [patch libteam] libteam: Add lacp fallback support for | ||
| single-member-port LAG | ||
|
|
||
| Signed-off-by: Haiyang Zheng <haiyang.z@alibaba-inc.com> | ||
| --- | ||
| teamd/teamd_runner_lacp.c | 67 ++++++++++++++++++++++++++++++++++++++++++----- | ||
| 1 file changed, 61 insertions(+), 6 deletions(-) | ||
|
|
||
| diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c | ||
| index 9c77fae..872af8a 100644 | ||
| --- a/teamd/teamd_runner_lacp.c | ||
| +++ b/teamd/teamd_runner_lacp.c | ||
| @@ -138,6 +138,8 @@ struct lacp { | ||
| #define LACP_CFG_DFLT_SYS_PRIO 0xffff | ||
| bool fast_rate; | ||
| #define LACP_CFG_DFLT_FAST_RATE false | ||
| + bool fallback; | ||
| +#define LACP_CFG_DFLT_FALLBACK false | ||
| int min_ports; | ||
| #define LACP_CFG_DFLT_MIN_PORTS 1 | ||
| enum lacp_agg_select_policy agg_select_policy; | ||
| @@ -272,6 +274,11 @@ static int lacp_load_config(struct teamd_context *ctx, struct lacp *lacp) | ||
| lacp->cfg.fast_rate = LACP_CFG_DFLT_FAST_RATE; | ||
| teamd_log_dbg("Using fast_rate \"%d\".", lacp->cfg.fast_rate); | ||
|
|
||
| + err = teamd_config_bool_get(ctx, &lacp->cfg.fallback, "$.runner.fallback"); | ||
| + if (err) | ||
| + lacp->cfg.fallback = LACP_CFG_DFLT_FALLBACK; | ||
| + teamd_log_dbg("Using fallback \"%d\".", lacp->cfg.fallback); | ||
| + | ||
| err = teamd_config_int_get(ctx, &tmp, "$.runner.min_ports"); | ||
| if (err) { | ||
| lacp->cfg.min_ports = LACP_CFG_DFLT_MIN_PORTS; | ||
| @@ -308,9 +315,24 @@ static bool lacp_port_loopback_free(struct lacp_port *lacp_port) | ||
| return true; | ||
| } | ||
|
|
||
| +/* | ||
| + * is_lacp_fallback_eligible - is lacp_port eligible to go into lacp fallback mode | ||
| + * | ||
| + * Return true if it is, false otherwise | ||
| + */ | ||
| +static bool is_lacp_fallback_eligible(struct lacp_port *lacp_port) | ||
| +{ | ||
| + teamd_log_dbg("%s fallback eligible state \"%d \" cfg \"%d\".", | ||
| + lacp_port->tdport->ifname, lacp_port->state, | ||
| + lacp_port->lacp->cfg.fallback); | ||
| + return lacp_port->state == PORT_STATE_DEFAULTED && | ||
| + lacp_port->lacp->cfg.fallback; | ||
| +} | ||
| + | ||
| static bool lacp_port_selectable_state(struct lacp_port *lacp_port) | ||
| { | ||
| - if (lacp_port->state == PORT_STATE_CURRENT) | ||
| + if (lacp_port->state == PORT_STATE_CURRENT || | ||
| + is_lacp_fallback_eligible(lacp_port)) | ||
| return true; | ||
| return false; | ||
| } | ||
| @@ -318,7 +340,8 @@ static bool lacp_port_selectable_state(struct lacp_port *lacp_port) | ||
| static bool lacp_port_unselectable_state(struct lacp_port *lacp_port) | ||
| { | ||
| if (lacp_port->state == PORT_STATE_CURRENT || | ||
| - lacp_port->state == PORT_STATE_EXPIRED) | ||
| + lacp_port->state == PORT_STATE_EXPIRED || | ||
| + is_lacp_fallback_eligible(lacp_port)) | ||
| return false; | ||
| return true; | ||
| } | ||
| @@ -1002,6 +1025,12 @@ static int lacp_port_link_update(struct lacp_port *lacp_port) | ||
| uint32_t speed = team_get_port_speed(team_port); | ||
| uint8_t duplex = team_get_port_duplex(team_port); | ||
| int err; | ||
| + bool admin_state; | ||
| + | ||
| + admin_state = team_get_ifinfo_admin_state(lacp_port->ctx->ifinfo); | ||
| + teamd_log_dbg("%s admin_state \"%d\" link_up \"%d\".", | ||
| + lacp_port->tdport->ifname, | ||
| + admin_state, linkup); | ||
|
|
||
| if (linkup != lacp_port->__link_last.up || | ||
| duplex != lacp_port->__link_last.duplex) { | ||
| @@ -1011,8 +1040,9 @@ static int lacp_port_link_update(struct lacp_port *lacp_port) | ||
| * will provide speed == 0 and duplex == 0. If that is the | ||
| * case now, do not set disabled state and allow such devices | ||
| * to work properly. | ||
| + * Only enable port if the it is admin_up and link_up. | ||
| */ | ||
| - if (linkup && (!duplex == !speed)) | ||
| + if (linkup && (!duplex == !speed) && admin_state) | ||
| err = lacp_port_set_state(lacp_port, PORT_STATE_EXPIRED); | ||
| else | ||
| err = lacp_port_set_state(lacp_port, PORT_STATE_DISABLED); | ||
| @@ -1344,9 +1374,19 @@ static int lacp_event_watch_admin_state_changed(struct teamd_context *ctx, | ||
|
|
||
| teamd_for_each_tdport(tdport, ctx) { | ||
| struct lacp_port *lacp_port = lacp_port_get(lacp, tdport); | ||
| - | ||
| - err = lacp_port_set_state(lacp_port, | ||
| - admin_state?PORT_STATE_EXPIRED:PORT_STATE_DISABLED); | ||
| + /* Only enable port if it is admin_up and link_up.*/ | ||
| + if (lacp_port->__link_last.up) { | ||
| + teamd_log_dbg("%s admin_state \"%d\" link_up.", | ||
| + lacp_port->tdport->ifname, | ||
| + admin_state); | ||
| + err = lacp_port_set_state(lacp_port, | ||
| + admin_state?PORT_STATE_EXPIRED:PORT_STATE_DISABLED); | ||
| + } else { | ||
| + teamd_log_dbg("%s admin_state \"%d\" link_down.", | ||
| + lacp_port->tdport->ifname, | ||
| + admin_state); | ||
| + err = lacp_port_set_state(lacp_port, PORT_STATE_DISABLED); | ||
| + } | ||
| if (err) | ||
| return err; | ||
| } | ||
| @@ -1452,6 +1492,16 @@ static int lacp_state_fast_rate_get(struct teamd_context *ctx, | ||
| return 0; | ||
| } | ||
|
|
||
| +static int lacp_state_fallback_get(struct teamd_context *ctx, | ||
| + struct team_state_gsc *gsc, | ||
| + void *priv) | ||
| +{ | ||
| + struct lacp *lacp = priv; | ||
| + | ||
| + gsc->data.bool_val = lacp->cfg.fallback; | ||
| + return 0; | ||
| +} | ||
| + | ||
| static int lacp_state_select_policy_get(struct teamd_context *ctx, | ||
| struct team_state_gsc *gsc, | ||
| void *priv) | ||
| @@ -1479,6 +1529,11 @@ static const struct teamd_state_val lacp_state_vals[] = { | ||
| .getter = lacp_state_fast_rate_get, | ||
| }, | ||
| { | ||
| + .subpath = "fallback", | ||
| + .type = TEAMD_STATE_ITEM_TYPE_BOOL, | ||
| + .getter = lacp_state_fallback_get, | ||
| + }, | ||
| + { | ||
| .subpath = "select_policy", | ||
| .type = TEAMD_STATE_ITEM_TYPE_STRING, | ||
| .getter = lacp_state_select_policy_get, | ||
| -- | ||
| 2.7.4 | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it required to check admin state? If link is up, then we definitely know that admin state is also up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Marian @marian-pritsak for the code review.
Yes, I removed the lacp fallback timer, which is the transition time from defaulted state to fallback state. Since we reached an agreement to not modify existing LACP receive state machine, I removed the fallback state and the fallback timer is no longer needed. Also I have updated the design document
https://github.com/Azure/SONiC/blob/gh-pages/doc/LACP%20Fallback%20Feature%20for%20SONiC_v0.5.md
We need to check the admin state because if we create a Lag PortChannel01 with member ports Ethernet1. The PortChannel will be by default admin Shut, but the member port will be in admin UP state. If we don't check the admin status of the LAG, the member will be moved into defaulted state since it's link up, and then be moved to fallback state if lacp fallback is enabled.
Please let me know if you still have questions.
Thanks,
Haiyang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marian-pritsak
Hi Marian,
"If link is up, then we definitely know that admin state is also up."
This is not always true in libteam, here the admin state is actually the admin state of the LAG . So it's possible that the PortChannel is admin down, but the member port is admin up and link up. In this case, the member port will be moved into defaulted state (or fallback state if enabled), which is not desired as the PortChannel is admin shutdown.
Please let me know if you still have concerns regarding this admin check. If so, we can setup a skype call to discuss further.
FYI. Some log I captured after shutting down the PortChannel11, the admin_state is "0" (down), but the member port Ethernet11 is still link up
Thanks,
Haiyang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzheng5 , this is actually separate issue. whether or not member port admin status depends on the lag admin status is a separate issue. i tested on arista switch, when the lag is down, the member port is errdisabled. is this issue related to lacp fallback mechanism? can you explain?