Skip to content
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.

Sign up for GitHub

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

Increase min-links limit for portchannel from 128 to 1024. #7265

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Increase min-links limit for portchannel from 128 to 1024. #7265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84db180
Increase min-links limit for portchannel from 128 to 1024.
raphaelt-nvidia Apr 8, 2021
7e88d04
Portchannel min-links increased to 1024 in teamd and yang, added yang…
raphaelt-nvidia May 13, 2021
49700c1
Changed tabs to spaces
raphaelt-nvidia Jun 6, 2021
2cd61b8
Changed min-links lower limit from 0 to 1
raphaelt-nvidia Jun 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/libteam/patch/0012-Increase-min_ports-upper-limit-to-1024.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
diff --git a/man/teamd.conf.5 b/man/teamd.conf.5
index 350ffc9..dc913cd 100644
--- a/man/teamd.conf.5
+++ b/man/teamd.conf.5
@@ -248,7 +248,7 @@ Default:
.RE
.TP
.BR "runner.min_ports " (int)
-Specifies the minimum number of ports that must be active before asserting carrier in the master interface, value can be 1 \(en 255.
+Specifies the minimum number of ports that must be active before asserting carrier in the master interface, value can be 1 \(en 1024.
.RS 7
.PP
Default:
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 9354ebb..a901398 100644
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -151,6 +151,7 @@ struct lacp {
#define LACP_CFG_DFLT_FALLBACK false
int min_ports;
#define LACP_CFG_DFLT_MIN_PORTS 1
+#define LACP_CFG_DFLT_MIN_PORTS_MAX 1024
enum lacp_agg_select_policy agg_select_policy;
#define LACP_CFG_DFLT_AGG_SELECT_POLICY LACP_AGG_SELECT_LACP_PRIO
} cfg;
@@ -493,7 +494,7 @@ static int lacp_load_config(struct teamd_context *ctx, struct lacp *lacp)
err = teamd_config_int_get(ctx, &tmp, "$.runner.min_ports");
if (err) {
lacp->cfg.min_ports = LACP_CFG_DFLT_MIN_PORTS;
- } else if (tmp < 1 || tmp > UCHAR_MAX) {
+ } else if (tmp < 1 || tmp > LACP_CFG_DFLT_MIN_PORTS_MAX) {
teamd_log_err("\"min_ports\" value is out of its limits.");
return -EINVAL;
} else {
1 change: 1 addition & 0 deletions src/libteam/patch/series
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@
0009-Fix-ifinfo_link_with_port-race-condition-with-newlink.patch
0010-When-read-of-timerfd-returned-0-don-t-consider-this-.patch
0011-Remove-extensive-debug-output.patch
0012-Increase-min_ports-upper-limit-to-1024.patch
7 changes: 7 additions & 0 deletions src/sonic-yang-models/tests/yang_model_tests/tests/portchannel.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
"PORT_CHANNEL_TEST": {
"desc": "Configure a member port in PORT_CHANNEL table."
},
"PORT_CHANNEL_MAX_VALID_MIN_LINKS": {
"desc": "Configure PortChannel with maximum valid value of min-links."
},
"PORT_CHANNEL_OUT_OF_RANGE_MIN_LINKS": {
"desc": "Configure PortChannel with greater than maximum valid value of min-links.",
"eStr": ["Value", "does not satisfy the constraint"]
},
"PORT_CHANNEL_WRONG_PATTERN": {
"desc": "INCORRECT PORTCHANNEL_NAME IN PORT_CHANNEL TABLE.",
"eStrKey" : "Pattern",
Expand Down
64 changes: 64 additions & 0 deletions src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,70 @@
}
}
},
"PORT_CHANNEL_MAX_VALID_MIN_LINKS": {
"sonic-port:sonic-port": {
"sonic-port:PORT": {
"PORT_LIST": [
{
"admin_status": "up",
"alias": "eth0",
"description": "Ethernet0",
"lanes": "65",
"mtu": 9000,
"name": "Ethernet0",
"speed": 25000
}
]
}
},
"sonic-portchannel:sonic-portchannel": {
"sonic-portchannel:PORTCHANNEL": {
"PORTCHANNEL_LIST": [
{
"admin_status": "up",
"members": [
"Ethernet0"
],
"min_links": "1024",
"mtu": "9100",
"name": "PortChannel0001"
}
]
}
}
},
"PORT_CHANNEL_OUT_OF_RANGE_MIN_LINKS": {
"sonic-port:sonic-port": {
"sonic-port:PORT": {
"PORT_LIST": [
{
"admin_status": "up",
"alias": "eth0",
"description": "Ethernet0",
"lanes": "65",
"mtu": 9000,
"name": "Ethernet0",
"speed": 25000
}
]
}
},
"sonic-portchannel:sonic-portchannel": {
"sonic-portchannel:PORTCHANNEL": {
"PORTCHANNEL_LIST": [
{
"admin_status": "up",
"members": [
"Ethernet0"
],
"min_links": "1025",
"mtu": "9100",
"name": "PortChannel0001"
}
]
}
}
},
"PORT_CHANNEL_WRONG_PATTERN": {
"sonic-portchannel:sonic-portchannel": {
"sonic-portchannel:PORTCHANNEL": {
Expand Down
8 changes: 6 additions & 2 deletions src/sonic-yang-models/yang-models/sonic-portchannel.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ module sonic-portchannel {

description "PORTCHANNEL yang Module for SONiC OS";

revision 2021-06-13 {
description "Change min-links valid range from 1-128 to 1-1024";
}

revision 2021-03-31 {
description "Add PortChannel Interface List with VRF attribute";
}
Expand Down Expand Up @@ -74,8 +78,8 @@ module sonic-portchannel {
}

leaf min_links {
type uint8 {
range 1..128;
type uint16 {
range 1..1024;
}
Copy link
Contributor

@joyas-joseph joyas-joseph Jun 4, 2021 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt the range be 1..1024?

If we do support value 0, what does it indicate? #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this necessary to be compatible with related sonic-net/sonic-utilities#1630. The original code there sets the default value of min-links to 0, and now that I have introduced range checking there, keeping a lower bound of 1 causes range check to fail. So this change aligns yang with CLI.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test the 0 in current sonic image? Does teamd support it? ie. you set it as 0, and admin down all the member ports, and the portchannel itself is still up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results are the same with current master and with the changes in this PR. Recall that the default value of min-links in sonic-utilities is 0. In both cases, shutting down all members of the LAG causes the portchannel to go oper down. I noticed some non-deterministic behaviour, that may be a separate bug: with one of two member ports down, the LAG is sometimes up and sometimes down.

Since the default is 0, one would expect that in the current master, a portchannel created with default min-links should be up when all members are down. On the other hand, the work in this PR is supposed to clean up mismatches, and based on Qi's question, I now think that it doesn't do what it should. What I did was align yang with CLI, but it seems teamd is now the odd one out, as the change in teamd only adjusted the upper limit and kept the lower at 1. If you think it is reasonable to specify that a portchannel with all members down should be up (i.e. min-links = 0), then we have some more work to do to make teamd comply. (Any other places?) If not, then I should do the alignment the other way: put the lower limit in yang back to 1, and change the lower limit and default in sonic-utilities to 1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great insight! I prefer set lower limit of min-link to 1 to make life a little bit easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's have the lower limit of min-links set to 1.

}

Expand Down