[Mellanox] Add new cases for eeprom by JibinBao · Pull Request #6649 · sonic-net/sonic-mgmt

JibinBao · 2022-10-28T07:02:57Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

201911
202012
202205

Approach

What is the motivation for this PR?

Check the outputs are correct for "show interface transceiver eeprom -d", "sudo sfputil show eeprom -d".

Check if all expected keys exist in the outputs
When cable support dom, check the corresponding keys related to monitor exist, and the the corresponding value has the correct format

How did you do it?

Add new tests

How did you verify/test it?

Run script of test_check_sfp_eeprom.py

Any platform specific information?

Yes. Mellanox

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

Any

Documentation

azure-pipelines · 2022-10-28T07:03:54Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:1:1: F401 'logging' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:3:1: F401 're' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:5:1: F401 'random' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:7:1: F401 'tests.common.fixtures.conn_graph_facts.conn_graph_facts' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:19:64: F811 redefinition of unused 'conn_graph_facts' from line 7 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:50:121: E501 line too long (140 > 120 characters) tests/platform_tests/mellanox/util.py:4:49: W605 invalid escape sequence 'd' tests/platform_tests/mellanox/util.py:8:39: W605 invalid escape sequence '.' tests/platform_tests/mellanox/util.py:20:121: E501 line too long (133 > 120 characters) tests/platform_tests/mellanox/util.py:82:121: E501 line too long (138 > 120 characters) tests/platform_tests/mellanox/util.py:187:121: E501 line too long (136 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

azure-pipelines · 2022-10-28T09:18:07Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:4:1: F401 'tests.common.fixtures.conn_graph_facts.conn_graph_facts' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:16:64: F811 redefinition of unused 'conn_graph_facts' from line 4 tests/platform_tests/mellanox/util.py:204:121: E501 line too long (122 > 120 characters) tests/platform_tests/mellanox/util.py:230:42: W605 invalid escape sequence '.' tests/platform_tests/mellanox/util.py:231:41: W605 invalid escape sequence '.' tests/platform_tests/mellanox/util.py:232:41: W605 invalid escape sequence '.' tests/platform_tests/mellanox/util.py:233:40: W605 invalid escape sequence '.'

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

azure-pipelines · 2022-10-28T09:34:22Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:4:1: F401 'tests.common.fixtures.conn_graph_facts.conn_graph_facts' imported but unused tests/platform_tests/mellanox/test_check_sfp_eeprom.py:16:64: F811 redefinition of unused 'conn_graph_facts' from line 4

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

JibinBao · 2022-10-28T09:57:20Z

The pre-commit check detected issues in the files touched by this pull request. The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results: trim trailing whitespace.................................................Passedfix end of files.........................................................Passedcheck yaml...........................................(no files to check)Skippedcheck for added large files..............................................Passedcheck python ast.........................................................Passedflake8...................................................................Failed- hook id: flake8- exit code: 1tests/platform_tests/mellanox/test_check_sfp_eeprom.py:4:1: F401 'tests.common.fixtures.conn_graph_facts.conn_graph_facts' imported but unusedtests/platform_tests/mellanox/test_check_sfp_eeprom.py:16:64: F811 redefinition of unused 'conn_graph_facts' from line 4

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.

Ensure that the pre-commit package is installed:
sudo pip install pre-commit
Go to repository root folder

Install the pre-commit hooks:
pre-commit install
Use pre-commit to check staged file:
pre-commit
Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

For this pre-commit issue, I am not sure how to resolve it. Maybe the pep8 check rule need to do some modification? Because conn_graph_facts is a fixture Actually it has been used by test case.

JibinBao · 2022-10-31T06:08:20Z

@nhe-NV

liat-grozovik · 2022-11-03T09:14:48Z

@Junchao-Mellanox could you please help to review?

Junchao-Mellanox · 2022-11-04T01:50:06Z

tests/platform_tests/mellanox/util.py

+    second_layer_dict = {}
+    previous_key = ""
+    top_key = ""
+    for line in sfp_eeprom_info.split("\n"):


Can we parse those SFP which has no EEPROM?

Yes. for this case, case will raise failure

Junchao-Mellanox · 2022-11-04T05:52:39Z

tests/platform_tests/mellanox/util.py

+                key, dom_monitor_data[key], pattern)
+
+
+def is_support_dom(duthost, port_index):


Maybe it is better to use ethtool -m sfp{index}. Part of the data of "show interface transceiver eeprom -d" comes from sfp.get_transceiver_bulk_status. So, using sfp.get_transceiver_bulk_status to test "show interface transceiver eeprom -d" may not find real issues.

maybe can use mlxlink/ethtool to check whether relevant fields are valid.

Junchao-Mellanox · 2022-11-04T05:54:31Z

tests/platform_tests/mellanox/util.py

+    return sfp_eeprom_info_dict
+
+
+def check_sfp_eeprom_info(duthost, sfp_eeprom_info, is_support_dom, show_eeprom_cmd):


Could you please consider using compare the output of show eeprom command with ethtool/mlxlink output?

for a specific field, the output from ethtool/mlxlink could have some differences with the SONiC CLI output, no easy way to directly compare of them. only the Vendor OUI, Vendor Name, Vendor SN.. are the same and can be compared directly.

Will use mlxlink tool to check if it supports dom.

Junchao-Mellanox · 2022-11-04T05:56:12Z

tests/platform_tests/mellanox/util.py

+    """
+    bulk_status_str = get_transceiver_bulk_status(duthost, port_index)
+    bulk_status_str = bulk_status_str.replace('-inf', '\'-inf\'')
+    bulk_status_dict = eval(bulk_status_str)


suggest use literal_eval instead of eval

Junchao-Mellanox · 2022-11-04T06:02:17Z

I am not sure about the scope of this PR. A few general comments:

"show interface transceiver eeprom" does not cover everything, will you consider verify the value in STATE DB?
Will you consider platform API level verification? Like sfp.get_transceiver_info, sfp.get_transceiver_dom_threshold...
Will you add case to verify EEPROM writing? For example, Y cable.

JibinBao · 2022-11-04T10:33:53Z

I am not sure about the scope of this PR. A few general comments:

"show interface transceiver eeprom" does not cover everything, will you consider verify the value in STATE DB?

Will you consider platform API level verification? Like sfp.get_transceiver_info, sfp.get_transceiver_dom_threshold...

Will you add case to verify EEPROM writing? For example, Y cable.

As what we discussed at the beginning, the PR doesn't include these scopes, we just check the two cmds' outputs. If needed, we can add one task to add them.

wangxin · 2022-11-07T08:15:48Z

This PR added 2 new files, can you ensure that the new files can pass the pre-commit check? Thanks! @JibinBao

JibinBao · 2022-11-07T08:20:00Z

This PR added 2 new files, can you ensure that the new files can pass the pre-commit check? Thanks! @JibinBao

Sure. There is only one issue in which fixture is used, but pep8 checks it is not imported. I will update it.

azure-pipelines · 2022-11-07T08:55:00Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/platform_tests/mellanox/util.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:16:64: F811 redefinition of unused 'conn_graph_facts' from line 4

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

azure-pipelines · 2022-11-07T10:07:24Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/platform_tests/mellanox/util.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/platform_tests/mellanox/test_check_sfp_eeprom.py:16:64: F811 redefinition of unused 'conn_graph_facts' from line 4

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

azure-pipelines · 2022-11-07T10:25:12Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/platform_tests/mellanox/util.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Passed

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

…er_bulk_status for branches apart from 202012

azure-pipelines · 2022-11-07T10:33:06Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/platform_tests/mellanox/test_check_sfp_eeprom.py Fixing tests/platform_tests/mellanox/util.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Passed

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

JibinBao · 2022-11-09T01:55:59Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2022-11-09T01:56:01Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2022-11-09T01:56:10Z

Azure Pipelines successfully started running 1 pipeline(s).

- What is the motivation for this PR? Check the outputs are correct for "show interface transceiver eeprom -d", "sudo sfputil show eeprom -d". Check if all expected keys exist in the outputs When cable support dom, check the corresponding keys related to monitor exist, and the the corresponding value has the correct format - How did you do it? Add new tests - How did you verify/test it? Run script of test_check_sfp_eeprom.py - Any platform specific information? Yes. Mellanox - Supported testbed topology if it's a new test case? Any

Add new cases for eeprom

95c62dc

JibinBao changed the title ~~Mellanox]Add new cases for eepromAdd new cases for eeprom~~ [Mellanox]Add new cases for eepromAdd new cases for eeprom Oct 28, 2022

fix pre-commit issues

04caa11

JibinBao force-pushed the add_new_eeprom_case branch from 1002f19 to 04caa11 Compare October 28, 2022 09:33

liat-grozovik changed the title ~~[Mellanox]Add new cases for eepromAdd new cases for eeprom~~ [Mellanox] Add new cases for eeprom Oct 30, 2022

liat-grozovik added Request for 202012 branch Request for 202205 branch labels Oct 30, 2022

Junchao-Mellanox reviewed Nov 4, 2022

View reviewed changes

JibinBao force-pushed the add_new_eeprom_case branch from ee2c8e6 to 32e9aa9 Compare November 7, 2022 10:06

JibinBao force-pushed the add_new_eeprom_case branch from 32e9aa9 to 78cb479 Compare November 7, 2022 10:24

Useing mlxlink to check if cable support dom instead of get_transceiv…

95661bf

…er_bulk_status for branches apart from 202012

JibinBao force-pushed the add_new_eeprom_case branch from 78cb479 to 95661bf Compare November 7, 2022 10:32

Junchao-Mellanox previously approved these changes Nov 7, 2022

View reviewed changes

Fix pep8 issue by pre-commit

0413adf

JibinBao dismissed Junchao-Mellanox’s stale review via 0413adf November 8, 2022 05:52

Junchao-Mellanox approved these changes Nov 8, 2022

View reviewed changes

liat-grozovik approved these changes Nov 8, 2022

View reviewed changes

liat-grozovik merged commit 49a1417 into sonic-net:master Nov 9, 2022

wangxin added the Included in 202012 branch label Nov 11, 2022

wangxin added the Included in 202205 branch label Nov 11, 2022

		key, dom_monitor_data[key], pattern)


		def is_support_dom(duthost, port_index):

		return sfp_eeprom_info_dict


		def check_sfp_eeprom_info(duthost, sfp_eeprom_info, is_support_dom, show_eeprom_cmd):

Conversation

JibinBao commented Oct 28, 2022 • edited by liat-grozovik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

Uh oh!

azure-pipelines bot commented Oct 28, 2022

Uh oh!

azure-pipelines bot commented Oct 28, 2022

Uh oh!

azure-pipelines bot commented Oct 28, 2022

Uh oh!

JibinBao commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JibinBao commented Oct 31, 2022

Uh oh!

liat-grozovik commented Nov 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Junchao-Mellanox commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JibinBao commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangxin commented Nov 7, 2022

Uh oh!

JibinBao commented Nov 7, 2022

Uh oh!

azure-pipelines bot commented Nov 7, 2022

Uh oh!

azure-pipelines bot commented Nov 7, 2022

Uh oh!

azure-pipelines bot commented Nov 7, 2022

Uh oh!

azure-pipelines bot commented Nov 7, 2022

Uh oh!

JibinBao commented Nov 9, 2022

Uh oh!

mssonicbld commented Nov 9, 2022

Uh oh!

azure-pipelines bot commented Nov 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JibinBao commented Oct 28, 2022 •

edited by liat-grozovik

Loading

JibinBao commented Oct 28, 2022 •

edited

Loading

Junchao-Mellanox commented Nov 4, 2022 •

edited

Loading

JibinBao commented Nov 4, 2022 •

edited

Loading