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.

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

Fix the conditional check for FEC errors #16700

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

vvolam
Copy link
Contributor

@vvolam vvolam commented Jan 28, 2025

Description of PR

FEC correctable codeword errors should not exceed FEC symbol errors, as a codeword is composed of multiple symbols. If this occurs, it likely indicates a reporting bug, misconfiguration, or data misinterpretation, requiring validation of counters, FEC settings, and system behavior. Fix the conditional check in the test case accordingly.

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405
  • 202411

Approach

What is the motivation for this PR?

Issue in the conditional check

How did you do it?

Fix the conditional check for FEC errors

How did you verify/test it?

Verified on SN4700 platform.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@vvolam vvolam requested a review from prgeor as a code owner January 28, 2025 21:23
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor merged commit 5883698 into sonic-net:master Jan 29, 2025
15 checks passed
@vvolam vvolam deleted the fix-fec-err-check branch January 29, 2025 22:54
@bingwang-ms
Copy link
Collaborator

If we do this change, does that mean passed test now will fail, and failed test now will pass?

@bingwang-ms
Copy link
Collaborator

Discussed offline, better to check if FEC counter is 0. The comparison can be skipped if FEC counter is 0.

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jan 29, 2025
* Fix the conditional check for FEC errors

* Fix the comment
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #16715

@vvolam
Copy link
Contributor Author

vvolam commented Jan 30, 2025

If we do this change, does that mean passed test now will fail, and failed test now will pass?

@bingwang-ms No, test cases which are failing are the ones with non-zero's. So this will not be true.

@vvolam
Copy link
Contributor Author

vvolam commented Jan 30, 2025

Discussed offline, better to check if FEC counter is 0. The comparison can be skipped if FEC counter is 0.

@bingwang-ms I have opened #16722 PR to address this additional check. Thank you!

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jan 30, 2025
* Fix the conditional check for FEC errors

* Fix the comment
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202411: #16724

mssonicbld pushed a commit that referenced this pull request Jan 30, 2025
* Fix the conditional check for FEC errors

* Fix the comment
mssonicbld pushed a commit that referenced this pull request Jan 31, 2025
* Fix the conditional check for FEC errors

* Fix the comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants