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

[Nvidia] Fix test_check_sfp_eeprom_with_option_dom issue due to some keys are not supported for OSFP 8X Pluggable Transceiver #8043

Closed
wants to merge 1 commit into from
Closed

[Nvidia] Fix test_check_sfp_eeprom_with_option_dom issue due to some keys are not supported for OSFP 8X Pluggable Transceiver #8043

wants to merge 1 commit into from

Conversation

JibinBao
Copy link
Contributor

@JibinBao JibinBao commented Apr 14, 2023 •
edited
Loading

Because OSFP 8X Pluggable Transceiver does not support keys: "ChannelMonitorValues", "ChannelThresholdValues", "ModuleMonitorValues", "ModuleThresholdValues", So skip checking them.

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?

Fix test_check_sfp_eeprom_with_option_dom issue

How did you do it?

Sikp checking keys: "ChannelMonitorValues", "ChannelThresholdValues", "ModuleMonitorValues", when Transceiver is OSFP 8X Pluggable

How did you verify/test it?

run test_check_sfp_eeprom_with_option_dom

Any platform specific information?

Any

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

Any

Documentation

@JibinBao
update sfp eeprom case
3e689a1 
Because the cable of OSFP does not support some keys

Change-Id: I900c6c018196e5ec33dcd6ca542b1f13015a1101
@JibinBao
Copy link
Contributor Author

/azpw run Azure.sonic-mgmt

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-mgmt

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin wangxin requested a review from prgeor April 19, 2023 03:35
@wangxin
Copy link
Collaborator

wangxin commented Apr 19, 2023

@prgeor Can you help review?

@liat-grozovik
Copy link
Collaborator

@prgeor kindly reminder to review

@prgeor
Copy link
Contributor

prgeor commented May 12, 2023

@JibinBao could you paste the output of "sfputil show eeprom -d -p EthernetXX" ?

@JibinBao
Copy link
Contributor Author

@JibinBao could you paste the output of "sfputil show eeprom -d -p EthernetXX" ?
@prgeor please see below
sudo sfputil show eeprom -d -p Ethernet8
Ethernet8: SFP EEPROM detected
Active App Selection Host Lane 1: N/A
Active App Selection Host Lane 2: N/A
Active App Selection Host Lane 3: N/A
Active App Selection Host Lane 4: N/A
Active App Selection Host Lane 5: N/A
Active App Selection Host Lane 6: N/A
Active App Selection Host Lane 7: N/A
Active App Selection Host Lane 8: N/A
Active Firmware Version: 0.0
Application Advertisement: IB NDR - Host Assign (0x11) - Copper cable - Media Assign (Unknown)
IB SDR (Arch.Spec.Vol.2) - Host Assign (0x11) - Copper cable - Media Assign (Unknown)
CMIS Revision: 5.0
Connector: No separable connector
Encoding: N/A
Extended Identifier: Power Class 1 (0.25W Max)
Extended RateSelect Compliance: N/A
Hardware Revision: 0.0
Host Electrical Interface: N/A
Host Lane Assignment Options: 17
Host Lane Count: 4
Identifier: OSFP 8X Pluggable Transceiver
Inactive Firmware Version: N/A
Length Cable Assembly(m): 1.0
Media Interface Code: Copper cable
Media Interface Technology: Copper cable unequalized
Media Lane Assignment Options: N/A
Media Lane Count: 0
Nominal Bit Rate(100Mbs): 0
Specification compliance: passive_copper_media_interface
Vendor Date Code(YYYY-MM-DD Lot): 2022-05-28
Vendor Name: NVIDIA
Vendor OUI: 48-b0-2d
Vendor PN: MCP4Y10-N001
Vendor Rev: A3
Vendor SN: MT2227VS00270
MonitorData:
ThresholdData:

@prgeor
Copy link
Contributor

prgeor commented May 12, 2023

Specification compliance: passive_copper_media_interface

@JibinBao From the output, it looks like this module is a passive cable

Specification compliance: passive_copper_media_interface

Can you check why the output is getting printed for channel monitor values for a DAC cable?

prgeor
prgeor reviewed May 12, 2023
View reviewed changes
tests/platform_tests/mellanox/util.py
@@ -215,7 +215,8 @@ def check_sfp_eeprom_info(duthost, sfp_eeprom_info, is_support_dom, show_eeprom_
["Application Advertisement", "ChannelThresholdValues", "ModuleThresholdValues"])
expected_keys = expected_keys - excluded_keys

if "Identifier" in sfp_eeprom_info and sfp_eeprom_info["Identifier"] == "SFP/SFP+/SFP28":
sfp_type_for_excluded_monitor_threshold_key = ["SFP/SFP+/SFP28", "OSFP 8X Pluggable Transceiver"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@JibinBao can you point me to the spec where OSFP does not support channel threshold values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the output doesn't include the four keys, I skip checking them.
I have checked it with @keboliu. We think for passive cable it should not include the four keys, otherwise, it should include.
So, we can change the above code to skip check these keys if the cable is a passive cable. Is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JibinBao yes we should skip checking for channel monitor values for DAC cables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. Will update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prgeor
We have one issue currently: sonic-net/sonic-buildimage#14602
Maybe after the issue is fixed or the design is figured out, we can change it accordingly.

@prgeor
Copy link
Contributor

prgeor commented May 12, 2023

@andywongarista can u review this?

@JibinBao
Copy link
Contributor Author

Specification compliance: passive_copper_media_interface

@JibinBao From the output, it looks like this module is a passive cable

Specification compliance: passive_copper_media_interface

Can you check why the output is getting printed for channel monitor values for a DAC cable?

@prgeor ,
The output doesn't include channel monitor values: "ChannelMonitorValues", "ChannelThresholdValues", "ModuleMonitorValues", "ModuleThresholdValues". Do you mean that it should include them?

@prgeor
Copy link
Contributor

prgeor commented May 19, 2023

Specification compliance: passive_copper_media_interface

@JibinBao From the output, it looks like this module is a passive cable

Specification compliance: passive_copper_media_interface

Can you check why the output is getting printed for channel monitor values for a DAC cable?

@prgeor , The output doesn't include channel monitor values: "ChannelMonitorValues", "ChannelThresholdValues", "ModuleMonitorValues", "ModuleThresholdValues". Do you mean that it should include them?

@JibinBao the identifier is "OSFP 8X Pluggable Transceiver" which is not the correct check for DAC cable identification.

@JibinBao
Copy link
Contributor Author

Specification compliance: passive_copper_media_interface

@JibinBao From the output, it looks like this module is a passive cable

Specification compliance: passive_copper_media_interface

Can you check why the output is getting printed for channel monitor values for a DAC cable?

@prgeor , The output doesn't include channel monitor values: "ChannelMonitorValues", "ChannelThresholdValues", "ModuleMonitorValues", "ModuleThresholdValues". Do you mean that it should include them?

@JibinBao the identifier is "OSFP 8X Pluggable Transceiver" which is not the correct check for DAC cable identification.

Thanks. Will Update it

@JibinBao
Copy link
Contributor Author

Close due to design change: sonic-net/sonic-utilities#2864
There is noo need to do any changes, the old test will support the new design

@JibinBao JibinBao closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prgeor prgeor prgeor left review comments

@nhe-NV nhe-NV nhe-NV approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JibinBao @mssonicbld @wangxin @liat-grozovik @prgeor @nhe-NV