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

Add fixes to test_intf_fec.py #16302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add fixes to test_intf_fec.py #16302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions tests/common/platform/transceiver_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,3 +387,16 @@ def is_passive_cable(sfp_eeprom_info):
"CR" in spec_compliance.get("Extended Specification Compliance", " "):
return True
return False


def get_passive_cable_port_list(dut):
passive_cable_port_list = []
cmd_show_eeprom = "sudo sfputil show eeprom -d"
eeprom_infos = dut.command(cmd_show_eeprom)['stdout']
eeprom_infos = parse_sfp_eeprom_infos(eeprom_infos)
for port_name, eeprom_info in eeprom_infos.items():
if is_passive_cable(eeprom_info):
logging.info(f"{port_name} is passive cable")
passive_cable_port_list.append(port_name)
logging.info(f"Ports with passive cable are: {passive_cable_port_list}")
return passive_cable_port_list
58 changes: 36 additions & 22 deletions tests/platform_tests/test_intf_fec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import logging
import pytest

from tests.common.helpers.assertions import pytest_assert
from tests.common.utilities import skip_release, wait_until
from tests.common.utilities import skip_release
from tests.common.platform.transceiver_utils import get_passive_cable_port_list
from retry.api import retry_call

pytestmark = [
pytest.mark.disable_loganalyzer, # disable automatic loganalyzer
Expand All @@ -20,6 +21,16 @@
"100G", "200G", "400G", "800G", "1600G"
]

FEC_CONFIG_DEFAULT_RETRIES = 5
FEC_CONFIG_NON_COPPER_RETRIES = 10


@pytest.fixture(scope="module", autouse=True)
def disable_bgp(duthost):
Copy link
Contributor

Choose a reason for hiding this comment

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

@nhe-NV where is the fixture used? why is this fixture in this file related to fec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @prgeor the fixture is autouse.
Why we need it:
When we choose the ports for the test, if on the ports there lot of routes on it, it causes results in oper up notification processed very late as orchagent has to reprogram all the 12 k routes with new nexthop information after a link is removed. To avoid such case, we disable the bgp before the test, and enable it back. to make the test much more stable.

duthost.shell("sudo config feature state bgp disabled")
yield
duthost.shell("sudo config feature state bgp enabled")


@pytest.fixture(autouse=True)
def is_supported_platform(duthost):
Expand Down Expand Up @@ -80,7 +91,7 @@ def test_config_fec_oper_mode(duthosts, enum_rand_one_per_hwsku_frontend_hostnam

logging.info("Get output of '{}'".format("show interface status"))
intf_status = duthost.show_and_parse("show interface status")

passive_copper_ports = get_passive_cable_port_list(duthost)
for intf in intf_status:
sfp_presence = duthost.show_and_parse("sudo sfpshow presence -p {}"
.format(intf['interface']))
Expand All @@ -99,12 +110,10 @@ def test_config_fec_oper_mode(duthosts, enum_rand_one_per_hwsku_frontend_hostnam
config_status = duthost.command("sudo config interface fec {} {}"
.format(intf['interface'], fec_mode))
if config_status:
pytest_assert(wait_until(30, 2, 0, duthost.is_interface_status_up, intf["interface"]),
"Interface {} did not come up after configuring FEC mode".format(intf["interface"]))
# Verify the FEC operational mode is restored
post_fec = get_fec_oper_mode(duthost, intf['interface'])
if not (post_fec == fec_mode):
pytest.fail("FEC status is not restored for interface {}".format(intf['interface']))
# interfaces that are not copper could take longer to configure fec
Copy link
Contributor

Choose a reason for hiding this comment

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

@nhe-NV Why FEC behavior is different between copper and active cables? Any IEEE spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @prgeor This is not related to the fec itself, it it due to the opticial module will take longer to come up than the copper, when run ""sudo config interface fec rs", in sonic, it will first do port shutdown, then change fec mode, and startup port, the startup port will take longer for optical

tries = FEC_CONFIG_NON_COPPER_RETRIES if (intf['interface']
not in passive_copper_ports) else FEC_CONFIG_DEFAULT_RETRIES
retry_call(validate_fec_oper_status, fargs=[duthost, intf['interface'], fec_mode], tries=tries, delay=3)


def get_interface_speed(duthost, interface_name):
Expand All @@ -122,8 +131,6 @@ def get_interface_speed(duthost, interface_name):
logging.info(f"Interface {interface_name} has speed {speed}")
return speed

pytest.fail(f"Interface {interface_name} not found")


def test_verify_fec_stats_counters(duthosts, enum_rand_one_per_hwsku_frontend_hostname,
enum_frontend_asic_index, conn_graph_facts):
Expand Down Expand Up @@ -153,14 +160,15 @@ def skip_ber_counters_test(intf_status: dict) -> bool:
if speed not in SUPPORTED_SPEEDS:
continue

fec_corr = intf.get('fec_corr', '').lower()
fec_uncorr = intf.get('fec_uncorr', '').lower()
fec_symbol_err = intf.get('fec_symbol_err', '').lower()
# Removes commas from "show interfaces counters fec-stats" (i.e. 12,354 --> 12354) to allow int conversion
fec_corr = intf.get('fec_corr', '').replace(',', '').lower()
fec_uncorr = intf.get('fec_uncorr', '').replace(',', '').lower()
fec_symbol_err = intf.get('fec_symbol_err', '').replace(',', '').lower()
# Check if fec_corr, fec_uncorr, and fec_symbol_err are valid integers
try:
fec_corr_int = int(fec_corr.replace(',', ''))
fec_uncorr_int = int(fec_uncorr.replace(',', ''))
fec_symbol_err_int = int(fec_symbol_err.replace(',', ''))
fec_corr_int = int(fec_corr)
fec_uncorr_int = int(fec_uncorr)
fec_symbol_err_int = int(fec_symbol_err)
except ValueError:
pytest.fail("FEC stat counters are not valid integers for interface {}, \
fec_corr: {} fec_uncorr: {} fec_symbol_err: {}"
Expand All @@ -171,11 +179,10 @@ def skip_ber_counters_test(intf_status: dict) -> bool:
pytest.fail("FEC uncorrectable errors are non-zero for interface {}: {}"
.format(intf_name, fec_uncorr_int))

# Check for valid FEC correctable codeword errors > FEC symbol errors
if fec_symbol_err_int > fec_corr_int:
pytest.fail("FEC symbol errors:{} are higher than FEC correctable errors:{} for interface {}"
.format(intf_name, fec_symbol_err_int, fec_corr_int))

# Check for valid FEC symbol errors > FEC correctable codeword errors
if fec_corr_int > fec_symbol_err_int:
pytest.fail("FEC correctable errors:{} are higher than FEC symbol errors:{} for interface {}"
.format(intf_name, fec_corr_int, fec_symbol_err_int))
if skip_ber_counters_test(intf):
continue
fec_pre_ber = intf.get('fec_pre_ber', '').lower()
Expand All @@ -189,3 +196,10 @@ def skip_ber_counters_test(intf_status: dict) -> bool:
pytest.fail("Pre-FEC and Post-FEC BER are not valid floats for interface {}, \
fec_pre_ber: {} fec_post_ber: {}"
.format(intf_name, fec_pre_ber, fec_post_ber))


def validate_fec_oper_status(duthost, interface, fec_mode):
# Verify the FEC operational mode is restored
post_fec = get_fec_oper_mode(duthost, interface)
if not (post_fec == fec_mode):
pytest.fail("FEC status is not restored for interface {}".format(interface))
Loading