Fix MACsec test reliability and configuration issues (sonic-net#21372)

rajshekhar-nexthop · abhishek-nexthop · commit dfbb6e72e92d · 2026-03-17T14:58:27.000Z
* ISS-2888:Fix JSON syntax in golden_config_db_t2.j2 template (sonic-net#401)  ### Description of PR  Summary: Fixes # (issue) Fixes below json syntax error. It is seen only when dut is prepared with macsec enable flag. json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 2 column 3 (char 4) ### Type of change  - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### 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  Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> * ISS-2969:Generate golden config only if macsec_profile is defined (sonic-net#420)  ### Description of PR Redundant override config is avoided as no macsec profile is set in the prepare phase. Below are details how macsec profile configurations are rendered: PREPARE phase: Uses generate_t2_golden_config_db() → template rendering → file-based config RUN phase: Uses set_macsec_profile() → direct sonic-db-cli commands → immediate CONFIG_DB update Summary: Fixes # (issue) ### Type of change  - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### 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  Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> * ISS-3251: Guard MACsec restart against systemd StartLimitHit; add restart helper (sonic-net#562)  ### Description of PR  Summary: • Add a StartLimitHit-safe restart helper and use it in MACsec docker restart test to reduce flakiness • New helper restart_service_with_startlimit_guard() in tests/common/helpers/dut_utils.py: • Proactively clears systemd failure counters (systemctl reset-failed) • Attempts restart, detects systemd rate limiting (StartLimitHit), applies bounded backoff (default 35s), then start • Verifies the target container becomes running within a timeout • Update tests/macsec/test_docker_restart.py to use the new helper instead of duthost.restart_service("macsec") Fixes # (issue) MACsec docker restart tests can intermittently fail due to systemd rate limiting after repeated restarts during teardown/restart cycles. • Guarding against StartLimitHit with a clear backoff-and-start flow improves test reliability without changing device behavior. ### Type of change  - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ x] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### Approach #### What is the motivation for this PR? • MACsec docker restart tests can intermittently fail when systemd enforces StartLimitHit due to rapid restart attempts during teardown/restart cycles. • This PR makes the restart path resilient to StartLimitHit by proactively clearing counters, applying bounded backoff, and verifying the container reaches the running state, thereby reducing test flakiness. #### How did you do it? • Added a helper restart_service_with_startlimit_guard() in tests/common/helpers/dut_utils.py that: • Detects StartLimitHit pre/post restart attempts • Runs systemctl reset-failed to clear counters • Applies a fixed backoff when rate-limited, then systemctl start • Verifies the container is running within a configurable timeout using existing wait_until/state checks • Updated tests/macsec/test_docker_restart.py to use the helper instead of a direct duthost.restart_service("macsec") call. #### How did you verify/test it? • Local validation in lab: • Executed tests/macsec/test_docker_restart.py::test_restart_macsec_docker with MACsec enabled. • Repeated the restart sequence to emulate rate limiting scenarios. • Verified the helper reliably recovers from StartLimitHit and the container becomes running within the timeout. #### Any platform specific information? #### Supported testbed topology if it's a new test case? ### Documentation  Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> * NOS-3311: Fix MACsec test race and cleanup sync (sonic-net#678) NOS-3311 tracks MACsec test flakiness caused by races between: * wpa_supplicant/MKA programming MACsec state into Redis (APPL/STATE DB), and * the test harness eagerly reading that state to build `MACSEC_INFO` (via `get_macsec_attr`). This can manifest as exceptions like `KeyError('sak')` when the MACsec egress SA row does not yet exist, even though `MACSEC_PORT_TABLE` already shows `enable_encrypt="true"`. There are also cleanup races where tests check for removal of MACsec DB entries before the background cleanup logic has finished. This PR adds two pieces of synchronization in sonic-mgmt: 1. Ensure MKA establishment before pre-loading MACsec session info for tests 2. Provide a helper to wait for MACsec DB cleanup after disabling MACsec File: `tests/common/macsec/__init__.py` * The `load_macsec_info` fixture (module-scoped, autouse) previously called `load_all_macsec_info()` immediately when MACsec was enabled and a profile was present. That in turn calls `get_macsec_attr()`, which expects APP/STATE DB MACsec SC/SA entries (including `sak`) to be fully programmed. * In environments where MACsec is pre-configured before tests start, this created a race: `MACSEC_PORT_TABLE` might already exist (with `enable_encrypt="true"`), but the egress SA row for the active AN might not yet have been written to APP_DB, leading to `KeyError('sak')` when `macsec_sa["sak"]` is accessed. * Fix: * When MACsec is enabled and a profile is present, the fixture now first *attempts* to resolve the `wait_mka_establish` fixture: ```python try: request.getfixturevalue('wait_mka_establish') except Exception: pass ``` * `wait_mka_establish` is defined in `tests/macsec/conftest.py` and internally uses `check_appl_db` plus `wait_until(...)` to ensure APP/STATE DB MACsec SC/SA tables are populated (including `sak`/`auth_key`/PN relationships) before returning. * If the fixture is not defined (e.g., in other environments or test suites), the code falls back to the previous behavior. * After this synchronization point, if `is_macsec_configured(...)` is true, `load_all_macsec_info()` is called to populate `MACSEC_INFO` for all control links. Otherwise, the original `macsec_setup` flow is triggered. This makes `get_macsec_attr()` execution order consistent with the rest of the MACsec test suite, which already relies on `wait_mka_establish`/`check_appl_db` to guarantee that egress SAs and SAKs exist before validating state. cleanup File: `tests/common/macsec/macsec_config_helper.py` * Add `wait_for_macsec_cleanup(host, interfaces, timeout=90)` and export it via `__all__`. * This helper is designed for tests that: * disable MACsec on one or more interfaces, and then * need to assert that all associated MACsec entries (port, SC, SA) have been automatically removed from Redis before proceeding. * Behavior: * For EOS neighbors, it is a no-op: they do not use Redis DBs and the function returns `True` immediately. * For SONiC hosts, it: * Polls both `APPL_DB` and `STATE_DB` using `redis_get_keys_all_asics` with patterns `MACSEC_*:{interface}*` (APPL_DB) and `MACSEC_*|{interface}*` (STATE_DB). * Aggregates any remaining keys per DB. * Returns `True` as soon as all such keys are gone for the given interfaces, logging total time taken. * If the `timeout` is exceeded, logs a warning, prints a summary of remaining entries, and returns `False`. * This centralizes the logic for “wait until MACsec entries are gone from Redis” instead of having ad hoc sleeps or partial checks in individual tests. * MACsec control-plane actions (via wpa_supplicant and swss/macsecorch) are asynchronous relative to the tests. It is valid for `MACSEC_PORT_TABLE` to show `enable_encrypt="true"` while transmit SAs and their SAKs are still being programmed. * `get_macsec_attr()` assumes that: * APP_DB `MACSEC_EGRESS_SC_TABLE` for `(port, sci)` exists and has a valid `encoding_an`, and * APP_DB `MACSEC_EGRESS_SA_TABLE` for `(port, sci, an)` exists and has a `sak` field. Without synchronization, tests that pre-load `MACSEC_INFO` can hit a window where the SA row does not yet exist and crash with `KeyError('sak')`. * By tying `load_macsec_info` to `wait_mka_establish` where available, we ensure those pre-loads happen only after the expected MACsec state has been fully written to Redis. * Similarly, when disabling MACsec, asynchronous background cleanup can lag behind the test’s expectations. Having a dedicated, reusable `wait_for_macsec_cleanup` helper lets future tests explicitly wait for cleanup completion instead of guessing with sleeps. * Verified that the new fixtures and helpers are imported and wired correctly: * `load_macsec_info` remains `autouse=True` at module scope, so existing MACsec tests automatically benefit from the additional synchronization. * `wait_for_macsec_cleanup` is exported in `__all__` for use by future MACsec tests. * Manually exercised MACsec configuration and teardown flows in a MACsec-enabled testbed (e.g., humm120) to confirm: * MACsec sessions establish successfully and APP/STATE DB contain expected MACsec entries before `load_all_macsec_info` is invoked. * Disabling MACsec followed by `wait_for_macsec_cleanup` results in all MACSEC_* keys being removed from APPL/STATE DB within the timeout window. --- Pull Request opened by [Augment Code](https://www.augmentcode.com/) with guidance from the PR author Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> * taking care of review comments - Refine restart_service_with_startlimit_guard to better handle pre-existing StartLimitHit, avoid unnecessary restarts, and apply a shorter backoff when not actually rate-limited. - Narrow the exception in MacsecPlugin to pytest.FixtureLookupError so we only fall back when the wait_mka_establish fixture is truly missing. - Make wait_for_macsec_cleanup more flexible by using a dynamic poll interval and relying on its default timeout from callers. Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> --------- Signed-off-by: rajshekhar <rajshekhar@nexthop.ai> Signed-off-by: Abhishek <abhishek@nexthop.ai>
diff --git a/ansible/config_sonic_basedon_testbed.yml b/ansible/config_sonic_basedon_testbed.yml
@@ -756,7 +756,7 @@
           macsec_profile: "{{ macsec_profile }}"
           num_asics: "{{ num_asics }}"
         become: true
-        when: "('t2' in topo) and (enable_macsec is defined)"
+        when: "('t2' in topo) and (macsec_profile is defined)"
 
       - name: Use minigraph case
         block:
diff --git a/ansible/templates/golden_config_db_t2.j2 b/ansible/templates/golden_config_db_t2.j2
@@ -23,8 +23,7 @@
 {% endif %}
 {%- endfor -%}
 {% else %}
-  {
-    "MACSEC_PROFILE": {
+  "MACSEC_PROFILE": {
       "{{macsec_profile}}": {
         "priority": "{{priority}}",
         "cipher_suite": "{{cipher_suite}}",
@@ -34,6 +33,5 @@
         "send_sci": "{{send_sci}}"
       }
     }
-  },
 {%- endif -%}
 }
diff --git a/tests/common/helpers/dut_utils.py b/tests/common/helpers/dut_utils.py
@@ -126,6 +126,64 @@ def clear_failed_flag_and_restart(duthost, container_name):
     pytest_assert(restarted, "Failed to restart container '{}' after reset-failed was cleared".format(container_name))
 
 
+def restart_service_with_startlimit_guard(duthost, service_name, backoff_seconds=30, verify_timeout=180):
+    """
+    Restart a systemd-managed service with StartLimitHit guard.
+
+    Strategy:
+    0) Pre-detect StartLimitHit and, if present, skip a failing restart
+    1) When not rate-limited, reset-failed to clear stale counters and try restart
+    2) If restart fails, rate-limit is detected, or container isn't running:
+       - 'systemctl reset-failed <service>.service'
+       - fixed backoff (default 30s when rate-limited, 1s otherwise)
+       - 'systemctl start <service>.service'
+       - wait until container is running
+
+    Returns: True when the service is (re)started and running; asserts on failure.
+    """
+
+    # 0) Pre-detect StartLimitHit so we can optionally skip a failing restart
+    pre_rate_limited = is_hitting_start_limit(duthost, service_name)
+
+    if not pre_rate_limited:
+        # 1) Proactively clear stale failure counters and try a normal restart
+        duthost.shell(
+            f"sudo systemctl reset-failed {service_name}.service",
+            module_ignore_errors=True
+        )
+        ret = duthost.shell(
+            f"sudo systemctl restart {service_name}.service",
+            module_ignore_errors=True
+        )
+        rate_limited = is_hitting_start_limit(duthost, service_name)
+    else:
+        logger.info(
+            f"StartLimitHit pre-detected for {service_name}, applying reset-failed and "
+            f"fixed backoff {backoff_seconds}s before start"
+        )
+        # Force the recovery path below without attempting an immediate restart.
+        ret = {"rc": 1}
+        rate_limited = True
+
+    # 2/3) Recovery path: reset-failed + backoff + start if needed
+    if ret.get("rc", 1) != 0 or rate_limited or not is_container_running(duthost, service_name):
+        duthost.shell(
+            f"sudo systemctl reset-failed {service_name}.service",
+            module_ignore_errors=True
+        )
+        time.sleep(backoff_seconds if rate_limited else 1)
+        duthost.shell(
+            f"sudo systemctl start {service_name}.service",
+            module_ignore_errors=True
+        )
+        pytest_assert(
+            wait_until(verify_timeout, 1, 0, check_container_state, duthost, service_name, True),
+            f"{service_name} container did not become running after recovery start"
+        )
+
+    return True
+
+
 def get_group_program_info(duthost, container_name, group_name):
     """Gets program names, running status and their pids by analyzing the command
        output of "docker exec <container_name> supervisorctl status". Program name
diff --git a/tests/common/macsec/__init__.py b/tests/common/macsec/__init__.py
@@ -122,7 +122,27 @@ def macsec_setup(self, startup_macsec, shutdown_macsec, macsec_feature):
 
     @pytest.fixture(scope="module", autouse=True)
     def load_macsec_info(self, request, macsec_duthost, ctrl_links, macsec_profile, tbinfo):
+        """Pre-load MACsec session info for all control links.
+
+        If MACsec is enabled and configured for this DUT/profile, wait for
+        MKA establishment (APP/STATE DB populated with SC/SA, including SAK)
+        before calling ``load_all_macsec_info``. This avoids races where
+        ``get_macsec_attr`` hits APP_DB before the egress SA row (and ``sak``)
+        has been written by wpa_supplicant.
+        """
+
         if get_macsec_enable_status(macsec_duthost) and get_macsec_profile(macsec_duthost):
+            # Ensure MKA sessions are established (SC/SA present in DB) if the
+            # test environment provides the wait_mka_establish fixture
+            # (defined in tests/macsec/conftest.py). For environments that do
+            # not define it, fall back to the original behaviour.
+            try:
+                request.getfixturevalue('wait_mka_establish')
+            except pytest.FixtureLookupError:
+                # Some environments do not define wait_mka_establish; fall back
+                # to the original behaviour when the fixture is missing.
+                pass
+
             if is_macsec_configured(macsec_duthost, macsec_profile, ctrl_links):
                 load_all_macsec_info(macsec_duthost, ctrl_links, tbinfo)
             else:
diff --git a/tests/common/macsec/macsec_config_helper.py b/tests/common/macsec/macsec_config_helper.py
@@ -1,6 +1,5 @@
 import logging
 import time
-
 from tests.common.macsec.macsec_helper import get_mka_session, getns_prefix, wait_all_complete, \
      submit_async_task, load_all_macsec_info
 from tests.common.macsec.macsec_platform_helper import global_cmd, find_portchannel_from_member, get_portchannel
@@ -17,7 +16,8 @@
     'enable_macsec_port',
     'disable_macsec_port',
     'get_macsec_enable_status',
-    'get_macsec_profile'
+    'get_macsec_profile',
+    'wait_for_macsec_cleanup'
 ]
 
 logger = logging.getLogger(__name__)
@@ -219,9 +219,21 @@ def cleanup_macsec_configuration(duthost, ctrl_links, profile_name):
         submit_async_task(delete_macsec_profile, (d, None, profile_name))
     wait_all_complete(timeout=300)
 
+    logger.info("Cleanup macsec configuration step3: wait for automatic cleanup")
+
+    # Extract DUT interface names from ctrl_links and wait for automatic
+    # MACsec cleanup on the DUT side.
+    interfaces = list(ctrl_links.keys())
+    wait_for_macsec_cleanup(duthost, interfaces)
+
+    # Also wait for neighbor devices to complete automatic cleanup for their
+    # corresponding ports.
+    for dut_port, nbr in list(ctrl_links.items()):
+        wait_for_macsec_cleanup(nbr["host"], [nbr["port"]])
+
     logger.info("Cleanup macsec configuration finished")
 
-    # Waiting for all mka session were cleared in all devices
+    # Waiting for all MKA sessions to be cleared on neighbor devices.
     for d in devices:
         if isinstance(d, EosHost):
             continue
@@ -268,3 +280,93 @@ def setup_macsec_configuration(duthost, ctrl_links, profile_name, default_priori
 
     # Load the MACSEC_INFO, to have data of all macsec sessions
     load_all_macsec_info(duthost, ctrl_links, tbinfo)
+
+
+def wait_for_macsec_cleanup(host, interfaces, timeout=90):
+    """Wait for MACsec daemon to automatically clean up all MACsec entries.
+
+    This function implements proper synchronization to wait for the automatic
+    cleanup process to complete, preserving the intended MACsec cleanup behavior.
+
+    Args:
+        host: SONiC DUT or neighbor host object
+        interfaces: List of interface names to check
+        timeout: Maximum time to wait in seconds for MACsec cleanup to finish (default: 90).
+
+    Returns:
+        bool: True if cleanup completed, False if timeout
+    """
+    if isinstance(host, EosHost):
+        # EOS hosts don't use Redis databases
+        logger.info("EOS host detected, skipping Redis cleanup verification")
+        return True
+
+    logger.info(f"Waiting for automatic MACsec cleanup (timeout: {timeout}s)")
+
+    start_time = time.time()
+    # Poll at most ~10 times over the full timeout, capped at 10 seconds between checks.
+    poll_interval = min(10, max(1, timeout / 10.0))
+
+    # We only care about APPL_DB and STATE_DB for MACsec tables. Instead of
+    # trying to reverse-engineer numeric DB IDs from CONFIG_DB, rely on
+    # sonic-db-cli with logical DB names and the same namespace logic used
+    # elsewhere in MACsec helpers.
+
+    while time.time() - start_time < timeout:
+        all_clean = True
+        remaining_entries = {}
+
+        for interface in interfaces:
+            ns_prefix = getns_prefix(host, interface)
+
+            for db_name, sep in (("APPL_DB", ":"), ("STATE_DB", "|")):
+                pattern = f"MACSEC_*{sep}{interface}*"
+                cmd = f"sonic-db-cli {ns_prefix} {db_name} KEYS '{pattern}'"
+
+                try:
+                    result = host.command(cmd, verbose=False)
+                    out_lines = result.get("stdout_lines", [])
+                except Exception as e:
+                    logger.warning(
+                        "Failed to query MACsec keys on host %s, DB %s, interface %s: %r",
+                        getattr(host, 'hostname', host),
+                        db_name,
+                        interface,
+                        e,
+                    )
+                    # If we cannot query Redis for this DB/interface, be
+                    # conservative and assume cleanup is not complete yet.
+                    all_clean = False
+                    continue
+
+                keys = [k.strip() for k in out_lines if k.strip()]
+                if keys:
+                    all_clean = False
+                    remaining_entries.setdefault((db_name, interface), []).extend(keys)
+
+        elapsed = time.time() - start_time
+
+        if all_clean:
+            logger.info(
+                f"Automatic MACsec cleanup completed successfully in {elapsed:.1f}s"
+            )
+            return True
+
+        # Log progress every 30 seconds to reduce verbosity
+        if int(elapsed) % 30 == 0 and elapsed > 0:
+            logger.info(f"Still waiting for cleanup... ({elapsed:.0f}s elapsed)")
+
+        time.sleep(poll_interval)
+
+    # Timeout reached
+    elapsed = time.time() - start_time
+    logger.warning(f"Automatic MACsec cleanup timeout after {elapsed:.1f}s")
+
+    # Log summary of remaining entries
+    total_remaining = sum(len(entries) for entries in remaining_entries.values())
+    if total_remaining > 0:
+        logger.warning(
+            f"  {total_remaining} MACsec entries still remain after timeout"
+        )
+
+    return False
diff --git a/tests/macsec/test_docker_restart.py b/tests/macsec/test_docker_restart.py
@@ -3,6 +3,8 @@
 
 from tests.common.utilities import wait_until
 from tests.common.macsec.macsec_helper import check_appl_db
+from tests.common.helpers.dut_utils import restart_service_with_startlimit_guard
+
 
 logger = logging.getLogger(__name__)
 
@@ -17,6 +19,6 @@ def test_restart_macsec_docker(duthosts, ctrl_links, policy, cipher_suite, send_
     duthost = duthosts[enum_rand_one_per_hwsku_macsec_frontend_hostname]
 
     logger.info(duthost.shell(cmd="docker ps", module_ignore_errors=True)['stdout'])
-    duthost.restart_service("macsec")
+    restart_service_with_startlimit_guard(duthost, "macsec", backoff_seconds=35, verify_timeout=180)
     logger.info(duthost.shell(cmd="docker ps", module_ignore_errors=True)['stdout'])
     assert wait_until(300, 6, 12, check_appl_db, duthost, ctrl_links, policy, cipher_suite, send_sci)

Original file line number	Diff line number	Diff line change
`@@ -23,8 +23,7 @@`
`23`	`23`	`{% endif %}`
`24`	`24`	`{%- endfor -%}`
`25`	`25`	`{% else %}`
`26`		`- {`
`27`		`- "MACSEC_PROFILE": {`
	`26`	`+ "MACSEC_PROFILE": {`
`28`	`27`	`"{{macsec_profile}}": {`
`29`	`28`	`"priority": "{{priority}}",`
`30`	`29`	`"cipher_suite": "{{cipher_suite}}",`
`@@ -34,6 +33,5 @@`
`34`	`33`	`"send_sci": "{{send_sci}}"`
`35`	`34`	`}`
`36`	`35`	`}`
`37`		`- },`
`38`	`36`	`{%- endif -%}`
`39`	`37`	`}`