Testcase modification to fetch attributes from platform.json and check if functionality is supported on DUT

[harish-kalyanaraman]

Description of PR

Summary: Fixes # (issue)

Type of change

• [x] Bug fix
• [ ] Testbed and Framework(new/improvement)
• [x] Test case(new/improvement)

Back port request

• [ ] 201911

Approach

What is the motivation for this PR?

Some vendors do not support the following functionality:

• watchdog disarm/rearm
• fetching certain attributes of a module

This causes test_module.py, and test_watchdog.py to fail

Also, test_tx_disable_channel leaves port with a single lane disabled (using mask of 1) - thus causing port to not rx/tx packets correctly.

How did you do it?

1. test_module.py:

• platform.json change: - added a get_module_attributes block in platform.json that describes the attributes that cannot be fetched. Example:

  	    "get_module_attributes": {
                 "model": false 
            }

• testcase change: - added a method get_module_attributes to fetch the required attribute from the platform.json - in testcases of non-supported attributes, added a check that skips the testcase if the fetched attribute is described as not supported("false") in the platform.json

2. test_watchdog.py:

• platform.json change: - added watchdog block in platform.json that describes disarm support ("false"). Example:

    "watchdog": {
            "disarm": false
        }

• testcase change: - added get_watchdog method to fetch attribute from platform.json - added a check in autouse fixture that skips the testcase by checking disarm support from platform.json

3. test_sfp.py:

• testcase change: - Get original mask of tx_disable_channel for each SFP, and after setting all combinations from 15 to 1, set the mask back to the original mask of tx_disable_channel.

How did you verify/test it?

ran against DUT with modified platform.json and validated that the tests were skipped

Any platform specific information?

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

Documentation

[lgtm-com[bot]]

This pull request introduces 1 alert and fixes 8 when merging 2f44cc70d53cb9238880caee69d9e541e97fdc2b into 18f60119b8b08f1778ee382e45d672e5c55a43c8 - view on LGTM.com

new alerts:

• 1 for Syntax error

fixed alerts:

• 6 for Except block handles 'BaseException'
• 1 for Unused import
• 1 for Unused argument in a formatting call

[judyjoseph]

@sujinmkang @prgeor can you take a look too

[prgeor]

@harish-kalyanaraman please clarify:

1. transmit disable channel on sfp's Why is this dependent on a particular platform.? It should be the property of the SFP, not platform. For eg. Byte 195 Bit#4 of SFF 8636 can be used to know if "TX_DISABLE" is implemented or not.

2.fetching certain attributes of a module Which attribute? Module means SFP?

[sanmalho-git]

@judyjoseph @prgeor - Our understanding is that tx_disable_channel is associated with supporting break-out mode on a port. Our linecards don't support breakout mode (discrete channel disable) - regardless of the SFP. Is it ok if we move this capability at a higher level in platform.json (single attribute per platform) instead of having the attribute per SFP. This attribute could also be used to infer whether a platform supports breakout mode.

[prgeor]

@harish-kalyanaraman the current form of test_tx_disable_channel() simply turns off the laser for each media lane. These media lane are not controlled per breakout port but per lane. The mask is obtained for range 0 to 0xf and used to disable the media lane. So, platform need not support breakout for test_tx_disable_channel() to pass

[prgeor]

@harish-kalyanaraman please fix the test failures.

[sanmalho-git]

@prgeor

1. In our platform, we have not implemented the capability in PMON to allow for tx_disable_per_channel yet, regardless of the module type (whether QSFP-28 or QSFP-DD) inserted. How would you propose handling such scenario by a vendor. a. We could add a PMON API that returns true/false whether tx_disable_per_channel is supported globally or not. Or, b. We could add this attribute to platform.json.

2. Moreover, looking at test_tx_disable_channel() in details at https://github.com/Azure/sonic-mgmt/blob/master/tests/platform_tests/api/test_sfp.py#L530 - the test is hard-wired for 4 lane QSFP28 (4x25G lanes = 100G). In our platform, we are dealing with 8 lane QSFP-DD(8x50G=400G). The test would have to be extended to support this 8 lane interface type as well.

If test is extended such that test operates on both 4 lane and 8 lane interface types with common code, it may make sense to use a common PMON API indicating whether tx_disable_per_channel is supported on a module (whether QSFP-28 or QSFP-DD).

• **For QSFP-DD, it should be noted that the CMIS specification does indeed provide for module advertising about not only whether tx_disable is supported globally (Page 01h byte 155 bit 1), but also whether tx_disable_per_channel is supported (Page 01h byte 151 bit 0)

For this PR, would recommend going with option 1b (w/ added attribute in platform.json) for globally not supporting tx_disable_per_channel regardless of the module type.

Option 2 could be done as a separate PR.

[sanmalho-git]

@prgeor We have now added support for tx_channel_disable on our DUTs. So, have removed the dependency of support of tx_channel_disable on platform.json.

Also, while running the tx_channel_disable, we noticed inthe test case loops through the combinations of a four-channel transceiver, but it loops upto mask of '1' and not '0'. We have added code to get the original tx_disable_channel mask before trying the combinations, and then restore that at the end of the loop for each SFP. Have updated the PR comments as well for this.

Could you please re-review.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 127e18437fa78e242185b6752cbe956545abb9bc into 94073fa1ca0f7889cbc02ec9a3554cac6e3e3746 - view on LGTM.com

new alerts:

• 1 for Unused import

[sanmalho-git]

@prgeor could you please re-review this PR - it is critical for platform tests to pass successfully on Nokia chassis. @judyjoseph - FYI.

[sujinmkang]

@sanmalho-git can you please sign the easyCLA for this PR? Once you are done the CLA sign in this repo, you don't need to do it from next PR.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: harish-kalyanaraman / name: Harish Kalyanaraman (f223deb7fa734d33ed4b1b4e22dde7482c36c5de, ecef92dc1bbbcc5a7a9f44661b03bafdb63c45e5, 6f8c15e5a8096b1389f4de86174092b185082490, 606c843499e6ff1bcb5b1ccfd42ab2467ce4776c, 7407fd5052e644b94eea6205a6a97a2dd4a03d96, a5a821cc817688413e3206f709e6b095bb3418c7)
• :white_check_mark: login: mannytaheri (aa919fd05912342ecf4f02eccd148974e4592c13)

[judyjoseph]

@harish-kalyanaraman could you resolve the conflicts

[azure-pipelines[bot]]

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.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_module.py
Fixing tests/platform_tests/api/conftest.py
Fixing tests/platform_tests/api/test_sfp.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_watchdog.py

check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Failed
- hook id: check-ast
- exit code: 1

tests/platform_tests/api/test_module.py: failed parsing with CPython 3.8.10:

Traceback (most recent call last):
File "/home/vsts/.cache/pre-commit/repo710l5hvr/py_env-python3/lib/python3.8/site-packages/pre_commit_hooks/check_ast.py", line 21, in main
ast.parse(f.read(), filename=filename)
File "/usr/lib/python3.8/ast.py", line 47, in parse
return compile(source, filename, mode, flags,
File "tests/platform_tests/api/test_module.py", line 212
if not self.expect(base_mac is not None, "Module {}: Failed to retrieve base MAC address".format(i)):
^
TabError: inconsistent use of tabs and spaces in indentation

autopep8.................................................................Failed
- hook id: autopep8
- files were modified by this hook

:2: SyntaxWarning: "is" with a literal. Did you mean "=="?
:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
:1: SyntaxWarning: "is" with a literal. Did you mean "=="?

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/platform_tests/api/test_module.py:5:1: F401 'yaml' imported but unused
tests/platform_tests/api/test_module.py:18:19: F821 undefined name 'basestring'
tests/platform_tests/api/test_module.py:73:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:176:121: E501 line too long (121 > 120 characters)
tests/platform_tests/api/test_module.py:223:121: E501 line too long (133 > 120 characters)
tests/platform_tests/api/test_module.py:259:121: E501 line too long (121 > 120 characters)
tests/platform_tests/api/test_module.py:267:121: E501 line too long (126 > 120 characters)
tests/platform_tests/api/test_module.py:270:121: E501 line too long (143 > 120 characters)
tests/platform_tests/api/test_module.py:274:121: E501 line too long (123 > 120 characters)
tests/platform_tests/api/test_module.py:304:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:326:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:347:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:368:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:381:29: F523 '...'.format(...) has unused arguments at position(s): 1
tests/platform_tests/api/test_module.py:390:13: E722 do not use bare 'except'
tests/platform_tests/api/test_module.py:469:121: E501 line too long (124 > 120 characters)
tests/platform_tests/api/test_module.py:494:121: E501 line too long (138 > 120 characters)
tests/platform_tests/api/conftest.py:2:1: F401 'time' imported but unused
tests/platform_tests/api/test_sfp.py:11:1: F401 'tests.common.fixtures.conn_graph_facts.conn_graph_facts' imported but unused
tests/platform_tests/api/test_sfp.py:12:1: F401 'tests.common.fixtures.duthost_utils.shutdown_ebgp' imported but unused
tests/platform_tests/api/test_sfp.py:22:19: F821 undefined name 'basestring'
tests/platform_tests/api/test_sfp.py:35:80: F811 redefinition of unused 'conn_graph_facts' from line 11
tests/platform_tests/api/test_sfp.py:35:98: F811 redefinition of unused 'shutdown_ebgp' from line 12
tests/platform_tests/api/test_sfp.py:62:9: F402 import 'sfp' from line 6 shadowed by loop variable
tests/platform_tests/api/test_sfp.py:195:12: E713 test for membership should be 'not in'
tests/platform_tests/api/test_sfp.py:275:25: E265 block comment should start with '# '
tests/platform_tests/api/test_sfp.py:293:121: E501 line too long (145 > 120 characters)
tests/platform_tests/api/test_sfp.py:296:121: E501 line too long (150 > 120 characters)
tests/platform_tests/api/test_sfp.py:315:121: E501 line too long (125 > 120 characters)
tests/platform_tests/api/test_sfp.py:334:121: E501 line too long (127 > 120 characters)
tests/platform_tests/api/test_sfp.py:343:121: E501 line too long (121 > 120 characters)
tests/platform_tests/api/test_sfp.py:366:121: E501 line too long (124 > 120 characters)
tests/platform_tests/api/test_sfp.py:384:121: E501 line too long (126 > 120 characters)
tests/platform_tests/api/test_sfp.py:402:121: E501 line too long (129 > 120 characters)
tests/platform_tests/api/test_sfp.py:419:121: E501 line too long (125 > 120 characters)
tests/platform_tests/api/test_sfp.py:478:121: E501 line too long (126 > 120 characters)
tests/platform_tests/api/test_sfp.py:533:121: E501 line too long (137 > 120 characters)
tests/platform_tests/api/test_sfp.py:535:121: E501 line too long (122 > 120 characters)
tests/platform_tests/api/test_sfp.py:553:121: E501 line too long (124 > 120 characters)
tests/platform_tests/api/test_sfp.py:573:121: E501 line too long (125 > 120 characters)
tests/platform_tests/api/test_sfp.py:576:121: E501 line too long (137 > 120 characters)
tests/platform_tests/api/test_sfp.py:599:121: E501 line too long (123 > 120 characters)
tests/platform_tests/api/test_sfp.py:609:121: E501 line too long (134 > 120 characters)
tests/platform_tests/api/test_sfp.py:611:121: E501 line too long (160 > 120 characters)
tests/platform_tests/api/test_sfp.py:635:121: E501 line too long (126 > 120 characters)
tests/platform_tests/api/test_sfp.py:642:121: E501 line too long (122 > 120 characters)
tests/platform_tests/api/test_sfp.py:652:121: E501 line too long (123 > 120 characters)
tests/platform_tests/api/test_sfp.py:655:100: F821 undefined name 'unicode'
tests/platform_tests/api/test_sfp.py:655:121: E501 line too long (173 > 120 characters)
tests/platform_tests/api/test_watchdog.py:112:121: E501 line too long (170 > 120 characters)
tests/platform_tests/api/test_watchdog.py:123:121: E501 line too long (160 > 120 characters)
tests/platform_tests/api/test_watchdog.py:154:121: E501 line too long (128 > 120 characters)
tests/platform_tests/api/test_watchdog.py:158:121: E501 line too long (127 > 120 characters)
tests/platform_tests/api/test_watchdog.py:160:121: E501 line too long (155 > 120 characters)
tests/platform_tests/api/test_watchdog.py:166:121: E501 line too long (152 > 120 characters)
tests/platform_tests/api/test_watchdog.py:181:121: E501 line too long (177 > 120 characters)
tests/platform_tests/api/test_watchdog.py:183:121: E501 line too long (182 > 120 characters)
tests/platform_tests/api/test_watchdog.py:200:121: E501 line too long (140 > 120 characters)
tests/platform_tests/api/test_watchdog.py:203:121: E501 line too long (163 > 120 characters)
tests/platform_tests/api/test_watchdog.py:221:121: E501 line too long (143 > 120 characters)
tests/platform_tests/api/test_watchdog.py:224:121: E501 line too long (160 > 120 characters)

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

1. Ensure that the pre-commit package is installed:

sudo pip install pre-commit

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

pre-commit install

4. Use pre-commit to check staged file:

pre-commit

5. Alternatively, you can check committed files using:

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

[harish-kalyanaraman]

@harish-kalyanaraman could you resolve the conflicts

Have rebased and pushed

[judyjoseph]

/AzurePipelines run Azure.sonic-buildimage

[azure-pipelines[bot]]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[judyjoseph]

/AzurePipelines run Azure.sonic-mgmt

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[azure-pipelines[bot]]

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.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_module.py
Fixing tests/platform_tests/api/conftest.py
Fixing tests/platform_tests/api/test_sfp.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_watchdog.py

check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Failed
- hook id: check-ast
...
[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:

1. 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.
2. Ensure that the pre-commit package is installed:

sudo pip install pre-commit

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

pre-commit install

5. Use pre-commit to check staged file:

pre-commit

6. Alternatively, you can check committed files using:

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

[azure-pipelines[bot]]

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.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_module.py
Fixing tests/platform_tests/api/conftest.py
Fixing tests/platform_tests/api/test_sfp.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/platform_tests/api/test_watchdog.py

check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Failed
- hook id: check-ast
...
[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:

1. 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.
2. Ensure that the pre-commit package is installed:

sudo pip install pre-commit

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

pre-commit install

5. Use pre-commit to check staged file:

pre-commit

6. Alternatively, you can check committed files using:

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

[rlhui]

@prgeor , @judyjoseph - would we please re-review this one and close this one at earliest? Thanks.

[sanmalho-git]

PR #7303 replaces this PR. Will close this one.