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

raphaelt-nvidia · 2021-04-08T16:10:59Z

Signed-off-by: Raphael Tryster [email protected]

Why I did it

Restrict the min-links parameter in "config portchannel" to the range 1-1024.
Fixes #6781 in conjunction with sonic-net/sonic-utilities#1630.
Align YANG model with limits in libteam and sonic-utilties.

How I did it

PR 1630 in sonic-utilities prevents CLI user from entering a value outside the allowed range. This PR does the following:

Increases the maximum value of min-links from 128 to 1024.
Provides validation in libteam, incorporating as a patch the code in https://git.kernel.org/pub/scm/linux/kernel/git/jpirko/libteam.git/commit/?id=69a7494bb77dc10bb27076add07b380dbd778592.
Updates the Yang model upper limit from 128 to 1024 (was inconsistent with libteam value).
Updates the Yang model lower limit from 1 to 0, since 0 is set as default in sonic-utilities which would fail its new range check otherwise.
Added Yang tests for valid and invalid value.

How to verify it

config portchannel add PortChannel0004 --min-links 1024

Command should be accepted.

show interfaces portchannel

Output should show PortChannel0004, no errors on CLI.

config portchannel add PortChannel0005 --min-links 1025

Command should be rejected

show interfaces portchannel

Output should not show PortChannel0005 , no errors on CLI.

Which release branch to backport (provide reason below if selected)

Description for the changelog

Updates YANG model to allow up to 1024 min_links for portchannel. Fixes #6781 in conjunction with #1630.

A picture of a cute animal (not mandatory but encouraged)

lguohan · 2021-04-08T16:52:03Z

do you need to update the libteam as well?

raphaelt-nvidia · 2021-04-08T16:57:14Z

The link I quoted above is the change in libteam.

anshuv-mfst · 2021-04-08T17:12:54Z

src/sonic-yang-models/yang-models/sonic-portchannel.yang

@@ -23,7 +23,7 @@ module sonic-portchannel {
 	description "PORTCHANNEL yang Module for SONiC OS";

 	revision 2019-07-01 {
-		description "First Revision";
+		description "Increased range for min_links";


Yang model subgroup meeting 4/8:
Please confirm if revision needs to be changed. (cc: @praveen-li)

For each update to the yang, it is better to maintain a corresponding revision with the description of the change.

anshuv-mfst · 2021-04-08T17:14:44Z

src/sonic-yang-models/yang-models/sonic-portchannel.yang

-					type uint8 {
-						range 1..128;
+					type uint16 {
+						range 1..1024;
 					}


Yang model subgroup meeting 4/8:
Do any tests need to be updated with new range limit (128->1024)? (cc: @praveen-li )

Looks like a test case needs to be added for the range check.

joyas-joseph · 2021-04-09T03:19:23Z

The link I quoted above is the change in libteam.

That change is not available in sonic yet, right? The version being checked out is much older (from Feb 4, 2020).

        git clone https://github.com/jpirko/libteam.git
        pushd ./libteam
        git checkout -b teamd -f c7237377dead39ae4a711297203bacf7edb9fa41

liat-grozovik · 2021-04-27T17:00:27Z

@lguohan and @judyjoseph can you please clarify what is needed?
do we want to keep this change and work to fix the conflict or should we abandon this as libteam is not the latest one? i dont think libteam should be updated just because of this yang model.

joyas-joseph · 2021-04-28T17:27:56Z

@lguohan and @judyjoseph can you please clarify what is needed?
do we want to keep this change and work to fix the conflict or should we abandon this as libteam is not the latest one? i dont think libteam should be updated just because of this yang model.

Let's have it consistent.
We can have the range 1..1024, if we bring in at least the commit (jpirko/libteam@69a7494) as a patch.
Without that, lets have the range 1..255.

lguohan · 2021-05-07T22:49:22Z

can you help to resolve conflict?

joyas-joseph · 2021-05-10T21:30:31Z

@lguohan and @judyjoseph can you please clarify what is needed?
do we want to keep this change and work to fix the conflict or should we abandon this as libteam is not the latest one? i dont think libteam should be updated just because of this yang model.

Let's have it consistent.
We can have the range 1..1024, if we bring in at least the commit (jpirko/libteam@69a7494) as a patch.
Without that, lets have the range 1..255.

@raphaelt-nvidia. Please update to have range from 1..255.

raphaelt-nvidia · 2021-05-11T05:23:41Z

The proposed change to the Yang model was to be consistent with ***@***.***<jpirko/libteam@69a7494>), which I wrongly thought had already been pushed. Working on that. Raphael Tryster Staff Engineer, Software From: joyas-joseph ***@***.***> Sent: Tuesday, 11 May, 2021 12:31 AM To: Azure/sonic-buildimage ***@***.***> Cc: Raphael Tryster ***@***.***>; Mention ***@***.***> Subject: Re: [Azure/sonic-buildimage] Increase min-links limit for portchannel from 128 to 1024. (#7265) @lguohan<https://github.com/lguohan> and @judyjoseph<https://github.com/judyjoseph> can you please clarify what is needed? do we want to keep this change and work to fix the conflict or should we abandon this as libteam is not the latest one? i dont think libteam should be updated just because of this yang model. Let's have it consistent. We can have the range 1..1024, if we bring in at least the commit ***@***.***<jpirko/libteam@69a7494>) as a patch. Without that, lets have the range 1..255. @raphaelt-nvidia<https://github.com/raphaelt-nvidia>. Please update to have range from 1..255. - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#7265 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ASDJDC5GCTW5MFFDFKFA3O3TNBGA3ANCNFSM42TGQXOA>.

joyas-joseph · 2021-06-04T17:21:09Z

src/sonic-yang-models/yang-models/sonic-portchannel.yang

-                    type uint8 {
-                        range 1..128;
+                    type uint16 {
+                        range 0..1024;
                    }


Shouldnt the range be 1..1024?

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

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.

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?

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.

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

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

src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json

qiluo-msft

As comments

Signed-off-by: Raphael Tryster <[email protected]>

… tests Signed-off-by: Raphael Tryster <[email protected]>

Signed-off-by: Raphael Tryster <[email protected]>

liat-grozovik · 2021-06-29T20:07:02Z

@qiluo-msft and @judyjoseph could you please help to review recent fixes and approve for merge?

qiluo-msft · 2021-07-01T06:43:54Z

I am confused about the numbers in PR description, like 0, 255. So I changed it according to my understanding. Please double check.

raphaelt-nvidia · 2021-07-01T07:17:04Z

I am confused about the numbers in PR description, like 0, 255. So I changed it according to my understanding. Please double check.

Now I am confused. What and where did you change? I think your confusion stems from the mismatches that existed before this work was done, and it appears above in the discussion. Originally, teamd was 1-255, cli was any integer with default 0, and yang was 1-128. Now, they should all be aligned on 1-1024.

qiluo-msft · 2021-07-01T07:20:35Z

You can check my changes by clicking the dropdown menu button

raphaelt-nvidia · 2021-07-01T07:40:11Z

You can check my changes by clicking the dropdown menu button

I had a bit of trouble finding it because I see it as Apr 8. I deduce from this that you are in China (I am in Israel) and github shows the date of a comment according to the viewer's time zone. Anyway, I saw your changes, and I think they are neither better nor worse than the original, because of the mismatches between the modules. So, no problem.

…#7265) #### Why I did it Restrict the min-links parameter in "config portchannel" to the range 1-1024. Fixes sonic-net#6781 in conjunction with sonic-net#1630. Align YANG model with limits in libteam and sonic-utilties. #### How I did it PR 1630 in sonic-utilities prevents CLI user from entering a value outside the allowed range. This PR does the following: - Increases the maximum value of min-links from 128 to 1024. - Provides validation in libteam, incorporating as a patch the code in https://git.kernel.org/pub/scm/linux/kernel/git/jpirko/libteam.git/commit/?id=69a7494bb77dc10bb27076add07b380dbd778592. - Updates the Yang model upper limit from 128 to 1024 (was inconsistent with libteam value). - Updates the Yang model lower limit from 1 to 0, since 0 is set as default in sonic-utilities which would fail its new range check otherwise. - Added Yang tests for valid and invalid value. #### How to verify it config portchannel add PortChannel0004 --min-links 1024 Command should be accepted. show interfaces portchannel Output should show PortChannel0004, no errors on CLI. config portchannel add PortChannel0005 --min-links 1025 Command should be rejected show interfaces portchannel Output should not show PortChannel0005 , no errors on CLI. #### Which release branch to backport (provide reason below if selected) #### Description for the changelog Updates YANG model to allow up to 1024 min_links for portchannel. Fixes sonic-net#6781 in conjunction with sonic-net#1630.

raphaelt-nvidia · 2021-10-11T08:11:38Z

@liat-grozovik This PR fixes #6781 in conjunction with sonic-net/sonic-utilities#1630. The other PR was also merged to 202106 but this one was not. Please add a label to request that this PR be merged to 202106. I see no merge conflicts.

#### Why I did it Restrict the min-links parameter in "config portchannel" to the range 1-1024. Fixes #6781 in conjunction with #1630. Align YANG model with limits in libteam and sonic-utilties. #### How I did it PR 1630 in sonic-utilities prevents CLI user from entering a value outside the allowed range. This PR does the following: - Increases the maximum value of min-links from 128 to 1024. - Provides validation in libteam, incorporating as a patch the code in https://git.kernel.org/pub/scm/linux/kernel/git/jpirko/libteam.git/commit/?id=69a7494bb77dc10bb27076add07b380dbd778592. - Updates the Yang model upper limit from 128 to 1024 (was inconsistent with libteam value). - Updates the Yang model lower limit from 1 to 0, since 0 is set as default in sonic-utilities which would fail its new range check otherwise. - Added Yang tests for valid and invalid value. #### How to verify it config portchannel add PortChannel0004 --min-links 1024 Command should be accepted. show interfaces portchannel Output should show PortChannel0004, no errors on CLI. config portchannel add PortChannel0005 --min-links 1025 Command should be rejected show interfaces portchannel Output should not show PortChannel0005 , no errors on CLI. #### Which release branch to backport (provide reason below if selected) #### Description for the changelog Updates YANG model to allow up to 1024 min_links for portchannel. Fixes #6781 in conjunction with #1630.

raphaelt-nvidia requested a review from lguohan as a code owner April 8, 2021 16:10

anshuv-mfst reviewed Apr 8, 2021

View reviewed changes

lguohan added the YANG YANG model related changes label Apr 18, 2021

prsunny mentioned this pull request Apr 22, 2021

PortChannel creation with out of range(0-255) value of "min-links", leads to Exception during update_data() #6781

Closed

raphaelt-nvidia requested review from jleveque, qiluo-msft and xumia as code owners May 23, 2021 13:19

raphaelt-nvidia force-pushed the min_links branch from 73f78ed to 325c54e Compare May 23, 2021 17:03

raphaelt-nvidia force-pushed the min_links branch from 325c54e to 7d2b567 Compare June 3, 2021 12:10

joyas-joseph reviewed Jun 4, 2021

View reviewed changes

qiluo-msft reviewed Jun 5, 2021

View reviewed changes

src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json Outdated Show resolved Hide resolved

qiluo-msft requested changes Jun 5, 2021

View reviewed changes

raphaelt-nvidia force-pushed the min_links branch from db7f697 to fde97d1 Compare June 13, 2021 14:44

joyas-joseph previously approved these changes Jun 14, 2021

View reviewed changes

qiluo-msft previously approved these changes Jun 14, 2021

View reviewed changes

raphaelt-nvidia dismissed stale reviews from qiluo-msft and joyas-joseph via a479ee7 June 24, 2021 13:32

raphaelt-nvidia added 4 commits June 28, 2021 06:47

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

84db180

Signed-off-by: Raphael Tryster <[email protected]>

Portchannel min-links increased to 1024 in teamd and yang, added yang…

7e88d04

… tests Signed-off-by: Raphael Tryster <[email protected]>

Changed tabs to spaces

49700c1

Signed-off-by: Raphael Tryster <[email protected]>

Changed min-links lower limit from 0 to 1

2cd61b8

Signed-off-by: Raphael Tryster <[email protected]>

raphaelt-nvidia force-pushed the min_links branch from a479ee7 to 2cd61b8 Compare June 28, 2021 06:51

joyas-joseph approved these changes Jun 30, 2021

View reviewed changes

qiluo-msft approved these changes Jul 1, 2021

View reviewed changes

qiluo-msft merged commit 79300a9 into sonic-net:master Jul 8, 2021

liat-grozovik added the Request for 202106 Branch label Oct 11, 2021

judyjoseph added the Included in 202106 Branch label Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raphaelt-nvidia commented Apr 8, 2021 •

edited

Loading

lguohan commented Apr 8, 2021

raphaelt-nvidia commented Apr 8, 2021

anshuv-mfst Apr 8, 2021

joyas-joseph Apr 8, 2021

anshuv-mfst Apr 8, 2021

venkatmahalingam Apr 29, 2021

joyas-joseph commented Apr 9, 2021

liat-grozovik commented Apr 27, 2021

joyas-joseph commented Apr 28, 2021

lguohan commented May 7, 2021

joyas-joseph commented May 10, 2021

raphaelt-nvidia commented May 11, 2021 via email

joyas-joseph Jun 4, 2021 •

edited by qiluo-msft

Loading

qiluo-msft Jun 5, 2021

raphaelt-nvidia Jun 6, 2021

qiluo-msft Jun 10, 2021

raphaelt-nvidia Jun 10, 2021

qiluo-msft Jun 10, 2021

joyas-joseph Jun 10, 2021

qiluo-msft left a comment

liat-grozovik commented Jun 29, 2021

qiluo-msft commented Jul 1, 2021

raphaelt-nvidia commented Jul 1, 2021

qiluo-msft commented Jul 1, 2021

raphaelt-nvidia commented Jul 1, 2021

raphaelt-nvidia commented Oct 11, 2021 •

edited

Loading

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

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

Conversation

raphaelt-nvidia commented Apr 8, 2021 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

lguohan commented Apr 8, 2021

raphaelt-nvidia commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyas-joseph commented Apr 9, 2021

liat-grozovik commented Apr 27, 2021

joyas-joseph commented Apr 28, 2021

lguohan commented May 7, 2021

joyas-joseph commented May 10, 2021

raphaelt-nvidia commented May 11, 2021 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

liat-grozovik commented Jun 29, 2021

qiluo-msft commented Jul 1, 2021

raphaelt-nvidia commented Jul 1, 2021

qiluo-msft commented Jul 1, 2021

raphaelt-nvidia commented Jul 1, 2021

raphaelt-nvidia commented Oct 11, 2021 • edited Loading

raphaelt-nvidia commented Apr 8, 2021 •

edited

Loading

joyas-joseph Jun 4, 2021 •

edited by qiluo-msft

Loading

raphaelt-nvidia commented Oct 11, 2021 •

edited

Loading