-
Notifications
You must be signed in to change notification settings - Fork 717
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
[Nvidia] Fix test_check_sfp_eeprom_with_option_dom issue due to some keys are not supported for OSFP 8X Pluggable Transceiver #8043
Conversation
Because the cable of OSFP does not support some keys Change-Id: I900c6c018196e5ec33dcd6ca542b1f13015a1101
/azpw run Azure.sonic-mgmt |
/AzurePipelines run Azure.sonic-mgmt |
Azure Pipelines successfully started running 1 pipeline(s). |
@prgeor Can you help review? |
@prgeor kindly reminder to review |
@JibinBao could you paste the output of "sfputil show eeprom -d -p EthernetXX" ? |
|
@JibinBao From the output, it looks like this module is a passive cable
Can you check why the output is getting printed for channel monitor values for a DAC cable? |
@@ -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"] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. Will update it
There was a problem hiding this comment.
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.
@andywongarista can u review this? |
@prgeor , |
@JibinBao the identifier is "OSFP 8X Pluggable Transceiver" which is not the correct check for DAC cable identification. |
Thanks. Will Update it |
Close due to design change: sonic-net/sonic-utilities#2864 |
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
Back port request
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