[sonic-mgmt] Update transceiver onboarding HLD to make parameters attribute based#20230
Conversation
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
az-pz
left a comment
There was a problem hiding this comment.
Minor comments. Looks great otherwise. Very thorough and comprehensive.
| "field_2": "specific_value_2", | ||
| "field_3": "specific_value_3", | ||
| "platform_hwsku_overrides": { | ||
| "PLATFORM_NAME+HWSKU_NAME": { |
There was a problem hiding this comment.
How are we concatenating PLATFORM_NAME and HWSKU_NAME? Is it joined by +?
| | Issue CLI command to shutdown a port | Validate link status using CLI configuration | Ensure that the link goes down | | ||
| | Issue CLI command to startup a port | Validate link status using CLI configuration | Ensure that the link is up and the port appears in the LLDP table. | | ||
| | In a loop, issue startup/shutdown command 100 times | Stress test for link status validation | Ensure link status toggles to up/down appropriately with each startup/shutdown command. Verify ports appear in the LLDP table when the link is up | | ||
| | In a loop, issue startup/shutdown command for a port 100 times | Stress test for link status validation | Ensure link status toggles to up/down appropriately with each startup/shutdown command. Verify ports appear in the LLDP table when the link is up | |
There was a problem hiding this comment.
Are we not using an attribute for the iteration count here?
There was a problem hiding this comment.
@az-pz I am planning to create a separate PR to use attributes for the system tests and will modify this TC with the relevant PR.
Added the current change to ensure that we don't miss documenting the TC
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
@AnoopKamath, @bobby-nexthop @Junchao-Mellanox @roy-sror Can you please help in reviewing this PR? |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
| "port_1": { | ||
| "vendor_name": "Vendor/Inc", | ||
| "vendor_pn": "QSFP-100G-AOC-10M" | ||
| "Ethernet0-96:4": { |
There was a problem hiding this comment.
Should we use Python list slicing syntax instead?
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
|
||
| **Port Range Parsing Rules:** | ||
|
|
||
| - **Range Format**: `EthernetX-Y` where X and Y are port numbers (X ≤ Y) |
There was a problem hiding this comment.
We can use python slicing for range format as well. It'd be consistent.
|
|
||
| **Port Configuration Keys:** | ||
| - **Individual Ports**: Standard SONiC port names (e.g., `"Ethernet0"`, `"Ethernet4"`) | ||
| - **Port Ranges**: Range notation (e.g., `"Ethernet4-12"`, `"Ethernet0-96:4"`) |
There was a problem hiding this comment.
"Ethernet0-96:4" should be updated to "Ethernet0:97:4".
|
|
||
| **Requirements**: | ||
|
|
||
| - Parse the mandatory 6-component format: `{TYPE}-{SPEED}-{FORM_FACTOR}-{DEPLOYMENT}-{MEDIA_LANE_MASK}-{HOST_LANE_MASK}` |
There was a problem hiding this comment.
Any thoughts on making this attribute a dictionary?
"transceiver_configuration": "AOC-100-QSFP-100G_STRAIGHT-0x0F-0x0F"
The string is quite lengthy and more often it will be generated, so we will be generating the configuration and then decoding it, it will be more intutive if the fields of this string are a dictionary.
There was a problem hiding this comment.
@arpit-nexthop I think that adding more fields to the dictionary in the corresponding DUT info file will increase the length to a large extent. Hence, I added the relevant details to be hardcoded in the string format and then have the parser to translate it to a dictionary.
Let me know if you still think that having a dictionary is better.
There was a problem hiding this comment.
@mihirpat1 I am more concerned with human understandability here than the file size.
AOC-100-QSFP-100G_STRAIGHT-0x0F-0x0F compresses lot of data that needs to be understood.
Just a preference on my part. I will let others comment on this as well.
Signed-off-by: Mihir Patel <[email protected]>
4f8740d to
521da12
Compare
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Signed-off-by: Mihir Patel <[email protected]>
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Signed-off-by: Mihir Patel <[email protected]>
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
| "mandatory": ["sff8024_identifier", "dual_bank_supported"], | ||
| "defaults": { | ||
| "vdm_supported": false, | ||
| "is_non_dac_and_cmis": false, |
There was a problem hiding this comment.
@prgeor Following is the meaning of this

This flag should be true for all CMIS optics which are non-DAC (400ZR, DR8 etc)
docs/testplan/transceiver/examples/inventory/attributes/eeprom.json
Outdated
Show resolved
Hide resolved
| "transceivers": { | ||
| "deployment_configurations": { | ||
| "8x100G_DR8": { | ||
| "vdm_supported": true, |
There was a problem hiding this comment.
@mihirpat1 do we have similar for pm_supported ?
There was a problem hiding this comment.
@prgeor Added it now in the EEPROM attributes table
There was a problem hiding this comment.
@mihirpat1 some optics may support few DOM flags and some not. how are we handling such scenario?
There was a problem hiding this comment.
@prgeor We plan to have separate attributes for each of such DOM flags.
There was a problem hiding this comment.
@mihirpat1 should we limit only the infra part in this file? All detailed specific test should be in https://github.com/sonic-net/sonic-mgmt/pull/20359/changes ?
There was a problem hiding this comment.
@prgeor Yes - we plan to remove the test case specific details from this file once this PR merges and we are fine with the test implementation HLD.
Signed-off-by: Mihir Patel <[email protected]>
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Signed-off-by: Mihir Patel <[email protected]>
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
| @@ -0,0 +1,17 @@ | |||
| { | |||
There was a problem hiding this comment.
@prgeor No - this file is not supposed to contain testbed specific info.
It is supposed to exist as a single file containing all the supported deployment templates and their corresponding mandatory, optional attributes.
| @@ -431,13 +1127,13 @@ This section describes the automated process for copying firmware binaries to th | |||
| To ensure only the necessary firmware binaries are present for each transceiver: | |||
|
|
|||
| 1. **Parse `transceiver_firmware_info.csv`** to obtain the list of available firmware binaries, their versions, and associated vendor and part numbers. | |||
There was a problem hiding this comment.
why this file remains in a CSV format?
Description of PR
This PR modernizes and significantly enhances the transceiver onboarding test plan by introducing a robust, modular attribute management system that replaces the previous CSV-based approach with a comprehensive JSON-based configuration framework.
Also, moved the
transceiver_onboarding_test_plan.mdfile totransceiver/test_plan.md.PRs planned to add attributes for various test categories
Summary:
Fixes # (issue)
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
ADO - 33984323