Create xcvrd restart testcases by Sutter-Pinewave · Pull Request #26534 · sonic-net/sonic-mgmt

Sutter-Pinewave · 2026-07-23T18:21:13Z

Description of PR

Summary:
Created xcvrd restart testcase.

Tests located at tests/transceiver/system/process_restart/test_xcvrtd_restart.py
Original testcase described in docs/testplan/transceiver/system_test_plan.md

NOTE: Please note that this PR is dependent on the helper functions in PR #26273

Fixes # (issue)

Type of change

Approach

What is the motivation for this PR?

Fulfilling the test plan described in system_test_plan.md

How did you do it?

Test initially logs link states, uptime, and DUT corefiles. Restarts syncd and waits based on related settle_sec system attribute. Performs standard port validation and recovery process on all participating ports.

How did you verify/test it?

Tested in Pinewave lab on Arista 7060 using loopback connections.
(P5<->P7), (P13<->P15)

Any platform specific information?

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

Documentation

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-07-23T18:21:20Z

/azp run

azure-pipelines · 2026-07-23T18:21:21Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-23T18:21:36Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-07-23T18:23:42Z

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

_{---Powered by SONiC BuildBot}

Copilot

Pull request overview

Adds a new transceiver System / Process Restart test module intended to validate xcvrd restart/crash recovery behavior as part of the transceiver system test plan.

Changes:

Introduces test_system_xcvrd_restart to restart xcvrd and verify port recovery.
Introduces test_system_xcvrd_crash_recovery to SIGKILL xcvrd and verify recovery.
Wires the tests to shared health-check / link-check style helpers (notably via helper modules referenced as dependencies).

Comments suppressed due to low confidence (2)

tests/transceiver/system/process_restart/test_xcvrd_restart.py:144

Keyword arguments should not contain spaces around '=' (flake8 E251).

        health_baseline = health_baseline,

tests/transceiver/system/process_restart/test_xcvrd_restart.py:133

check_links_up() returns a dict (with passed/details), but this code treats it as a boolean. Since non-empty dicts are always truthy, the warning branch will never run even when links are down.

    if not check_links_up(duthost, port_attributes_dict):
        logger.warning("Validation on Start FAILED: some ports are down")

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-07-28T20:36:32Z

/azp run

azure-pipelines · 2026-07-28T20:36:48Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

…ilty fixes Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-06T00:14:47Z

/azp run

azure-pipelines · 2026-08-06T00:14:54Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Sutter-Pinewave · 2026-08-06T00:21:08Z

test_report_for_xcvrd_restart-Aug5-512pm.html
test_report_for_xcvrd_restart-Aug5-358pm.html

Hi all, uploaded the first draft of the updated version of the code. Also attached two passlogs. The 5:12pm one is a pass, the 3:58 one is a failure on a slightly older version that was misusing a helper, but I've included it as it happened to capture a link flap, proving the negative case.

Copilot

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 5 comments.

Suppressed comments (12)

tests/common/platform/interface_utils.py:120

wait_ports_oper_status currently returns a list of formatted failure strings, but callers (e.g. standard_port_recovery_and_verification) treat the return value as a list of port names. This breaks down_ports/up_ports computation and can also raise KeyError when indexing per_port_failures.

    if wait_until(wait_sec, poll_interval_sec, 0, lambda: not _ports_not_at_status()):
        return []
    return [
        "port {} did not reach oper-{} within {}s".format(port, status, wait_sec)
        for port in _ports_not_at_status()

tests/transceiver/common/verification.py:380

standard_port_recovery_and_verification’s docstring lists LLDP and CMIS checks, but those steps are currently commented out in the implementation. Either re-enable them or update the docstring so the function contract matches what actually runs.

    """Run the Standard Port Recovery and Verification Procedure on a
    batch of ports (link status, flap/stability, LLDP, CMIS state,
    docker/process health), batched across ``ports`` so fixed per-call
    costs aren't multiplied by port count and every port's failures are
    surfaced in one call.

tests/transceiver/system/conftest.py:63

_state_db_key_exists reads STATE_DB without a per-port ASIC namespace. On multi-ASIC DUTs this can report false 'missing' entries because each ASIC has its own STATE_DB namespace. Use the existing db_helpers wrappers and accept a namespace argument.

def _state_db_key_exists(duthost, key):
    cmd = f'sonic-db-cli STATE_DB hgetall "{key}"'
    out = duthost.shell(cmd, module_ignore_errors=True)
    if out.get("rc", 1) != 0:
        return False
    return bool((out.get("stdout") or "").strip())

tests/transceiver/system/conftest.py:123

The STATE_DB consistency loop should pass the owning ASIC namespace for each port when checking TRANSCEIVER_INFO/DOM entries; otherwise multi-ASIC DUTs can be incorrectly flagged as missing data.

    for port in sorted(port_attributes_dict.keys()):
        if not _state_db_key_exists(duthost, f"TRANSCEIVER_INFO|{port}"):
            missing_info.append(port)
        if not _state_db_key_exists(duthost, f"TRANSCEIVER_DOM_SENSOR|{port}"):
            missing_dom.append(port)

tests/common/platform/interface_utils.py:105

The docstring says wait_ports_oper_status returns one failure string per port, but for downstream use it should return port names (and let callers format messages). Please update the docstring to match the function’s actual contract.

This issue also appears on line 116 of the same file.

    Returns a list with one string per port still not at oper-``status`` after
    ``wait_sec``; empty once all reach it. A port absent from the dump is reported
    as a failure (rather than raising) so a missing/renamed port aggregates like
    any other laggard.

tests/transceiver/common/verification.py:241

Typo in the check_cmis_state docstring ('breakou t') makes the comment hard to read and looks like an accidental paste/formatting artifact.

This issue also appears on line 376 of the same file.

    Why: ``TRANSCEIVER_STATUS`` is published once per physical module (under
    the first sub-port of a breakou                   t group) and carries every host lane of
    the module, so a breakout sub-port must be checked only against its own

tests/transceiver/common/verification.py:9

Module docstring claims all parent/child helpers return the per-port dict shape, but standard_port_recovery_and_verification returns an aggregate dict with top-level 'passed'/'per_port'/'details'. This mismatch will confuse future call sites and reviewers.

Implements the Standard Port and Verification function,
as well as the related child functions. All parent and
child functions will return an output following the format:

dict: ``{port: {'passed': bool, 'details': str}}``

tests/transceiver/system/process_restart/test_swss_restart.py:64

This test restarts swss, so the monitored process expected to change PID is orchagent (in the swss container), not xcvrd. As written, the per-test health check will likely fail because orchagent PID changes are treated as unexpected.

    expected_pid_changes.add("xcvrd")

tests/transceiver/system/process_restart/test_swss_restart.py:73

check_links_up returns a result dict (with a 'passed' key). Using it as a boolean will always treat failures as success because non-empty dicts are truthy, so the test will log uptime even when ports are down.

    if not check_links_up(duthost, port_attributes_dict):
        logger.warning("Validation on Start FAILED: some ports are down")
    else:

tests/transceiver/system/process_restart/test_syncd_restart.py:64

This test restarts syncd, so the monitored process expected to change PID is syncd (in the syncd container), not xcvrd. As written, the per-test health check will likely fail because syncd PID changes are treated as unexpected.

    expected_pid_changes.add("xcvrd")

tests/transceiver/system/process_restart/test_syncd_restart.py:73

check_links_up returns a result dict (with a 'passed' key). Using it as a boolean will always treat failures as success because non-empty dicts are truthy, so the test will log uptime even when ports are down.

    if not check_links_up(duthost, port_attributes_dict):
        logger.warning("Validation on Start FAILED: some ports are down")
    else:

tests/transceiver/system/process_restart/test_pmon_restart.py:70

check_links_up returns a result dict (with a 'passed' key). Using it as a boolean will always treat failures as success because non-empty dicts are truthy, so the test will log uptime even when ports are down.

    if not check_links_up(duthost, port_attributes_dict):
        logger.warning("Validation on Start FAILED: some ports are down")
    else:

+from tests.common.helpers.sonic_db import AppDbCli as sdbHelp
+from tests.common.platform.processes_utils import check_process_up


Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-06T21:50:08Z

/azp run

azure-pipelines · 2026-08-06T21:50:14Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

mssonicbld · 2026-08-06T22:06:46Z

/azp run

azure-pipelines · 2026-08-06T22:07:02Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T18:50:10Z

/azp run

azure-pipelines · 2026-08-07T18:50:27Z

Azure Pipelines: Successfully started running 1 pipeline(s).

…issues Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T18:51:24Z

/azp run

azure-pipelines · 2026-08-07T18:51:42Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 10 comments.

Suppressed comments (7)

tests/transceiver/system/process_restart/test_swss_restart.py:40

tests.common.platform.processes_utils does not define check_process_up, so this import raises ImportError during pytest collection and none of the swss restart test code can run. Replace it with an existing helper or implement restart verification using a pre/post pmon/xcvrd PID comparison.

from tests.common.platform.processes_utils import check_process_up

tests/transceiver/system/process_restart/test_syncd_restart.py:40

tests.common.platform.processes_utils does not define check_process_up, so this import raises ImportError during pytest collection and none of the syncd restart test code can run. Replace it with an existing helper or implement restart verification using a pre/post pmon/xcvrd PID comparison.

from tests.common.platform.processes_utils import check_process_up

tests/transceiver/system/process_restart/test_xcvrd_restart.py:144

expected_pid_changes permits a changed PID but does not assert one, while kill_pmon_daemon_pid_w_sig ignores kill errors. A failed SIGKILL can therefore leave the original xcvrd running and this crash-recovery test still passes. Capture the pre-crash PID and require a RUNNING xcvrd with a different PID after recovery.

    expected_pid_changes.add("xcvrd")

tests/transceiver/system/conftest.py:123

These STATE_DB reads always use the default namespace. On multi-ASIC DUTs, transceiver keys for frontend ports reside in their owning ASIC namespaces, so every such port is falsely reported missing. Resolve each port's namespace and issue the check there, as the new DB helpers already do.

    for port in sorted(port_attributes_dict.keys()):
        if not _state_db_key_exists(duthost, f"TRANSCEIVER_INFO|{port}"):
            missing_info.append(port)
        if not _state_db_key_exists(duthost, f"TRANSCEIVER_DOM_SENSOR|{port}"):
            missing_dom.append(port)

tests/transceiver/system/process_restart/test_xcvrd_restart.py:21

The module advertises an I2C-error restart test as skipped, but no test_system_xcvrd_restart_with_i2c_errors function or skip marker exists, so pytest reports no skipped test and that test-plan case is silently absent. Add the test with an explicit pytest.mark.skip(reason=...) until it is implementable, or narrow the stated PR/module scope.

    `- test_system_xcvrd_restart_with_i2c_errors
            NOTE: this test has been skipped during initial development
            due to library issues

tests/transceiver/common/state_management.py:148

The helper claims to restore and report admin/link, high-power, and CMIS datapath state, but the final verdict rechecks only links. A failed sfputil lpmode off command or a datapath that remains invalid after recycling leaves still_failing empty and the teardown reports recovery. Revalidate LPMode and CMIS state after the actions and include those failures in the summary.

    final_link = check_links_up(duthost, port_attributes_dict)
    if not final_link["passed"]:
        summary["still_failing"].extend(final_link["down"])

tests/transceiver/system/conftest.py:123

A DOM entry is not expected for every logical breakout port: dom/test_dom_availability.py:108-120 explicitly requires non-primary breakout subports to have no TRANSCEIVER_DOM_SENSOR data. This loop therefore reports valid breakout configurations as inconsistent. Check the physical module's first subport only (using lport_to_first_subport_mapping) for DOM presence.

        if not _state_db_key_exists(duthost, f"TRANSCEIVER_DOM_SENSOR|{port}"):
            missing_dom.append(port)

+# sonic-db-cli database identifiers (the first positional arg to sonic-db-cli).
+STATE_DB = "STATE_DB"


+    # NOTE: Temporarily disabled for PR streamlining
+    #       All commented code below is UNTESTED
+    #


+    All (port, step) failures are accumulated and reported in a single
+    ``pytest.fail`` so one run surfaces every issue.
+    """
+    expected_pid_changes.add("xcvrd")


+    All (port, step) failures are accumulated and reported in a single
+    ``pytest.fail`` so one run surfaces every issue.
+    """
+    expected_pid_changes.add("xcvrd")


+    All (port, step) failures are accumulated and reported in a single
+    ``pytest.fail`` so one run surfaces every issue.
+    """
+    expected_pid_changes.add("xcvrd")


+# Failures are reported as warnings instead of fixture errors so they
+# don't mask the actual test results that are already on the report.


+    config_facts = duthost.get_running_config_facts()
+    return set(config_facts.get("PORT", {}).keys())


+    for port in down_ports:
+        per_port_failures[port].append(
+            f"port {port} did not reach oper-up within {link_up_timeout_sec}s"


+    time.sleep(pmon_wait + 60)
+
+    # Wait for settle time and verify
+    result = standard_port_recovery_and_verification(


+        if datapath_fields_seen == 0 and config_fields_seen == 0:
+            per_port[port] = {
+                "passed": False,
+                "details": (
+                    f"{port} (parent {parent}) TRANSCEIVER_STATUS|{parent} "
+                    f"has no DP<N>State or config_state_hostlane<N> fields "
+                    f"for this port's active host lanes "
+                    f"{sorted(active_lanes)} - cannot confirm CMIS state "
+                    "(schema mismatch, partial publish, or lane-range "
+                    "mismatch)"
+                ),
+            }
+            continue


Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T20:47:56Z

/azp run

azure-pipelines · 2026-08-07T20:48:12Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T22:35:31Z

/azp run

azure-pipelines · 2026-08-07T22:35:46Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T23:08:18Z

/azp run

azure-pipelines · 2026-08-07T23:08:35Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T23:20:05Z

/azp run

azure-pipelines · 2026-08-07T23:20:21Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: sutter <sutter.lum@pinewave.com>

mssonicbld · 2026-08-07T23:26:22Z

/azp run

azure-pipelines · 2026-08-07T23:26:38Z

Azure Pipelines: Successfully started running 1 pipeline(s).

xcvrd draft

ccc5e8c

Signed-off-by: sutter <sutter.lum@pinewave.com>

github-actions Bot requested review from rawal01, wangxin and xwjiang-ms July 23, 2026 18:21

mihirpat1 added the Transceiver Test Dev 🔌 Action item for transceiver test development label Jul 23, 2026

mihirpat1 requested a review from Copilot July 23, 2026 18:24

Copilot started reviewing on behalf of mihirpat1 July 23, 2026 18:24 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Addressed GH copilot comments - still non functional

8ce4483

Signed-off-by: sutter <sutter.lum@pinewave.com>

Sutter-Pinewave added 5 commits August 5, 2026 11:07

Combined restart tcs and refactored to work w new verification func

85fea0d

Signed-off-by: sutter <sutter.lum@pinewave.com>

Added optional flag to return uptime in get_program_info

2a78003

Signed-off-by: sutter <sutter.lum@pinewave.com>

SPRaV streamlining and flake8 fixes

e738a54

Signed-off-by: sutter <sutter.lum@pinewave.com>

Updated to simon vers

a9fe265

Signed-off-by: sutter <sutter.lum@pinewave.com>

Updated misc files to bring in line with simon changes, plus compatib…

4289287

…ilty fixes Signed-off-by: sutter <sutter.lum@pinewave.com>

github-actions Bot requested review from cyw233, mihirpat1 and yutongzhang-microsoft August 6, 2026 00:15

mihirpat1 requested a lite review from Copilot August 6, 2026 04:01

Copilot started reviewing on behalf of mihirpat1 August 6, 2026 04:02 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Updated helper function calls

9e391a1

Signed-off-by: sutter <sutter.lum@pinewave.com>

Merge branch 'master' into fix/add-tests-transceiver-xcvrd-restart

2f28f09

Sutter-Pinewave added 2 commits August 7, 2026 11:49

Backported Simon's db helpers

52badb8

Signed-off-by: sutter <sutter.lum@pinewave.com>

db update

80694b5

Signed-off-by: sutter <sutter.lum@pinewave.com>

Updating db_helpers to simons latest PR - dupe bc last one had merge …

c709d7c

…issues Signed-off-by: sutter <sutter.lum@pinewave.com>

az-pz requested a balanced review from Copilot August 7, 2026 19:42

Copilot started reviewing on behalf of az-pz August 7, 2026 19:43 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

Trailing whitespace fixes

66942a5

Signed-off-by: sutter <sutter.lum@pinewave.com>

Updated pmon polling function calls

71dcc91

Signed-off-by: sutter <sutter.lum@pinewave.com>

Restoring deleted functions

820a029

Signed-off-by: sutter <sutter.lum@pinewave.com>

removed dupe definition

9d63b5b

Signed-off-by: sutter <sutter.lum@pinewave.com>

Added a blank line

84720c7

Signed-off-by: sutter <sutter.lum@pinewave.com>

		from tests.common.helpers.sonic_db import AppDbCli as sdbHelp
		from tests.common.platform.processes_utils import check_process_up

		# sonic-db-cli database identifiers (the first positional arg to sonic-db-cli).
		STATE_DB = "STATE_DB"

		# Failures are reported as warnings instead of fixture errors so they
		# don't mask the actual test results that are already on the report.

		config_facts = duthost.get_running_config_facts()
		return set(config_facts.get("PORT", {}).keys()) No newline at end of file

Uh oh!

Conversation

Sutter-Pinewave commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

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

Uh oh!

mssonicbld commented Jul 23, 2026

Uh oh!

azure-pipelines Bot commented Jul 23, 2026

Uh oh!

azure-pipelines Bot commented Jul 23, 2026

Uh oh!

mssonicbld commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

mssonicbld commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

Sutter-Pinewave commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

mssonicbld commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

mssonicbld commented Aug 7, 2026

Uh oh!

azure-pipelines Bot commented Aug 7, 2026

Uh oh!

mssonicbld commented Aug 7, 2026

Uh oh!

azure-pipelines Bot commented Aug 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Aug 7, 2026

Uh oh!

azure-pipelines Bot commented Aug 7, 2026

Uh oh!

mssonicbld commented Aug 7, 2026

Uh oh!

azure-pipelines Bot commented Aug 7, 2026

Uh oh!

Sutter-Pinewave commented Jul 23, 2026 •

edited

Loading