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

Jump to bottom

Updated total headroom pool size to accommodate 100G ports on T2 uplinks #16690

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Updated total headroom pool size to accommodate 100G ports on T2 uplinks #16690

merged 2 commits into from
Sep 26, 2023

Conversation

vmittal-msft
Copy link
Contributor

@vmittal-msft vmittal-msft commented Sep 25, 2023 •
edited
Loading

Why I did it

sonic-mgmt xoff test was failing for [100g,120km]. Needed to update total headroom pool size when 100G line card is used as T2 uplink.

Work item tracking
  • Microsoft ADO (25266920):

How I did it

Updated total headroom size in buffer config

How to verify it

Verified with sonic-mgmt xoff test passing for [100g,120km]

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

@gechiang
Copy link
Collaborator

@vmittal-msft can you fix the test failures?

FAIL: test_qos_and_buffer_nokia_ixr7250e_36x100g_render_template (tests.test_j2files.TestJ2Files)

Traceback (most recent call last):
File "/sonic/src/sonic-config-engine/tests/test_j2files.py", line 349, in test_qos_and_buffer_nokia_ixr7250e_36x100g_render_template
'buffer-nokia-ixr7250e-36x100g.json', 1)
File "/sonic/src/sonic-config-engine/tests/test_j2files.py", line 324, in do_test_qos_and_buffer_lc_render_template
assert utils.cmp(sample_output_file, self.output_file), self.run_diff(sample_output_file, self.output_file)
AssertionError: --- /sonic/src/sonic-config-engine/tests/sample_output/py2/buffer-nokia-ixr7250e-36x100g.json 2023-09-25 22:13:56.590981203 +0000
+++ /sonic/src/sonic-config-engine/tests/output 2023-09-25 22:59:35.165952177 +0000
...

Let's not wait until all tests stop then address the test failures...

gechiang
gechiang approved these changes Sep 26, 2023
View reviewed changes
Copy link
Collaborator

@gechiang gechiang left a comment

Choose a reason for hiding this comment

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

LGTM

rlhui
rlhui reviewed Sep 26, 2023
View reviewed changes
device/nokia/x86_64-nokia_ixr7250e_36x400g-r0/Nokia-IXR7250E-36x100G/0/buffers_defaults_t2.j2
@@ -22,7 +22,7 @@
"size": "6441610000",
"type": "both",
"mode": "dynamic",
"xoff": "7785676"
"xoff": "396096307"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's reason that xoff headroom needs to be substantially increased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This size was calculated assuming 100g is used for downlink so cable length was 2km whereas it can also be used for uplink (cable length - 120km). so we need to do calculation based on 120km not 2km. Although it will be some wastage for 2km scenario but it should cover both cases.

@rlhui rlhui requested a review from abdosi September 26, 2023 05:10
@ansrajpu-git ansrajpu-git mentioned this pull request Sep 26, 2023
Merged
6 tasks
@gechiang
Copy link
Collaborator

@ansrajpu-git, can you share if there is any dependency in the order of PR needs to be merged between this one and the one you mentioned (sonic-net/sonic-mgmt#10146)?

@ansrajpu-git
Copy link

@ansrajpu-git, can you share if there is any dependency in the order of PR needs to be merged between this one and the one you mentioned (sonic-net/sonic-mgmt#10146)?

This has to go first before my PR#10146

@lguohan lguohan merged commit 9068bd9 into sonic-net:master Sep 26, 2023
@mssonicbld
Copy link
Collaborator

@vmittal-msft PR conflicts with 202211 branch

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Sep 26, 2023
@vmittal-msft @mssonicbld
[nokia]: Updated total headroom pool size to accommodate 100G ports o…
157d972 
…n T2 uplinks (sonic-net#16690)

Microsoft ADO (25266920)

sonic-mgmt xoff test was failing for [100g,120km]. Needed to update total headroom pool size when 100G line card is used as T2 uplink.

This size was calculated assuming 100g is used for downlink so cable length was 2km whereas it can also be used for uplink (cable length - 120km). so we need to do calculation based on 120km not 2km. Although it will be some wastage for 2km scenario but it should cover both cases.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202205: #16712

@mssonicbld mssonicbld mentioned this pull request Sep 26, 2023
Merged
11 tasks
mssonicbld pushed a commit that referenced this pull request Sep 27, 2023
@vmittal-msft @mssonicbld
[nokia]: Updated total headroom pool size to accommodate 100G ports o…
9ffa4bd 
…n T2 uplinks (#16690)

Microsoft ADO (25266920)

sonic-mgmt xoff test was failing for [100g,120km]. Needed to update total headroom pool size when 100G line card is used as T2 uplink.

This size was calculated assuming 100g is used for downlink so cable length was 2km whereas it can also be used for uplink (cable length - 120km). so we need to do calculation based on 120km not 2km. Although it will be some wastage for 2km scenario but it should cover both cases.
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Oct 7, 2023
@vmittal-msft @mssonicbld
[nokia]: Updated total headroom pool size to accommodate 100G ports o…
f3eb7fb 
…n T2 uplinks (sonic-net#16690)

Microsoft ADO (25266920)

sonic-mgmt xoff test was failing for [100g,120km]. Needed to update total headroom pool size when 100G line card is used as T2 uplink.

This size was calculated assuming 100g is used for downlink so cable length was 2km whereas it can also be used for uplink (cable length - 120km). so we need to do calculation based on 120km not 2km. Although it will be some wastage for 2km scenario but it should cover both cases.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202305: #16798

@mssonicbld mssonicbld mentioned this pull request Oct 7, 2023
Merged
11 tasks
mssonicbld added a commit that referenced this pull request Oct 7, 2023
@mssonicbld
[nokia]: Updated total headroom pool size to accommodate 100G ports o…
70332c1 
…n T2 uplinks (#16690) (#16798)
@mssonicbld mssonicbld mentioned this pull request Oct 19, 2023
Merged
6 tasks
@mssonicbld mssonicbld mentioned this pull request Nov 2, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gechiang gechiang gechiang approved these changes

@abdosi abdosi abdosi approved these changes

@rlhui rlhui rlhui approved these changes

@lguohan lguohan Awaiting requested review from lguohan

@yxieca yxieca Awaiting requested review from yxieca

Assignees
No one assigned
Labels
Approved for 202211 Branch Approved for 202305 Branch Cherry Pick Conflict_202211 Included in 202205 Branch Included in 202305 Branch Request for 202205 Branch Request for 202211 Branch Request for 202305 Branch
Projects
SONiC Chassis
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vmittal-msft @gechiang @ansrajpu-git @mssonicbld @rlhui @abdosi @lguohan @yxieca @StormLiangMS