Skip to content

Revert LPM test due to design change back and fix some LPM test issues#10903

Merged
liat-grozovik merged 4 commits intosonic-net:masterfrom
JibinBao:revert_lpm_master
Dec 10, 2023
Merged

Revert LPM test due to design change back and fix some LPM test issues#10903
liat-grozovik merged 4 commits intosonic-net:masterfrom
JibinBao:revert_lpm_master

Conversation

@JibinBao
Copy link
Copy Markdown
Contributor

@JibinBao JibinBao commented Nov 30, 2023

Description of PR

Summary:

  1. Revert LPM test, because the implementation has been reverted to the old way.[Mellanox] Revert LPM implementation to the old way sonic-buildimage#17096
  2. Fix issue of test_check_sfputil_low_power_mode. Previously after setting the lpmode, test checks the value is on or off, which is not correct. we should check if the value of lpmode is the set one.
  3. When the cable type is OSPF and the power class is not "Power Class 1", it also supports LPM. So, add a check accordingly.

Fixes # (issue)

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205
  • 202305

Approach

What is the motivation for this PR?

Revert lpm test

How did you do it?

Revert lpm test

How did you verify/test it?

Run lmp test on image inlcuding sonic-net/sonic-buildimage#17096

Any platform specific information?

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

Documentation

When setting lpm, we need to check it is changed to the corresponding mode, not just check lpm is on o off.

Change-Id: I97ac5818d57c4ae15be3dbee7dae4878a95f8b2c
Fix lpm test, we just need to check the interface supporting ldp instead of all interfaces

Change-Id: I5eeed28de626202b6eb43c8f7e622b3dc5c22751
@JibinBao JibinBao requested a review from prgeor as a code owner November 30, 2023 02:51
@JibinBao JibinBao changed the title Revert lpm master Revert LPM test Nov 30, 2023
@JibinBao JibinBao changed the title Revert LPM test Revert LPM test due to design change back Nov 30, 2023
@liat-grozovik
Copy link
Copy Markdown
Collaborator

@JibinBao it looks like you have done some additional enhancement in the PR and not just the revert part.
can you have separated PRs for each motivation or provide accurate description to explain the changes and the reason?

@JibinBao
Copy link
Copy Markdown
Contributor Author

JibinBao commented Dec 8, 2023

@JibinBao it looks like you have done some additional enhancement in the PR and not just the revert part. can you have separated PRs for each motivation or provide accurate description to explain the changes and the reason?
@liat-grozovik
Thank you for your comment. I do some additional enhancements indeed. And I have updated the relevant description.
Can you help review again?

@JibinBao JibinBao changed the title Revert LPM test due to design change back Revert LPM test due to design change back and fix some LPM test issues Dec 8, 2023
@liat-grozovik liat-grozovik merged commit 7d0ea9a into sonic-net:master Dec 10, 2023
new_lpmode = "off" if original_lpmode[intf].lower() == "on" else "on"
lpmode_set_result = duthost.command("{} {} {}".format(cmd_sfp_set_lpmode, new_lpmode, intf))
assert lpmode_set_result["rc"] == 0, "'{} {} {}' failed".format(cmd_sfp_set_lpmode, new_lpmode, intf)
time.sleep(10)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why sleep and not polling?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the old implementation, I just revert it to the old one.

cmd = "show interfaces transceiver eeprom {} | grep 400ZR".format(asichost.cli_ns_option)
if duthost.shell(cmd, module_ignore_errors=True)['rc'] == 0:
logging.info("sleeping for 60 seconds for ZR optics to come up")
time.sleep(60)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also here, why sleep and not poll?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same to the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants