From 63fba71f0f1d06900ab96a689bbbcb06496d1d28 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Tue, 2 Jun 2026 21:18:59 +0000 Subject: [PATCH 01/10] [chassisd]: Enhance DPU auto-recovery robustness and race condition handling - Read dpu_auto_recovery from DEVICE_METADATA instead of FEATURE table - Default to disabled when dpu_auto_recovery field is missing - Direct ManualIntervention when auto-recovery disabled (skip WaitForSelfRecovery) - PCIe detach/reattach around power-cycle for clean hardware reset - Check reset_limit before issuing power-cycle to avoid wasted resets - Acquire state transition lock during power-cycle to prevent races - Re-check transition_in_progress after lock to close TOCTOU window - Abort power-cycle if lock cannot be acquired (another operation in progress) - Unify field name: use transition_in_progress (drop legacy state_transition_in_progress) - Ignore transition_type == 'recovery' so chassisd does not suppress itself - Refactor: extract _process_single_dpu_recovery to reduce update_dpu_recovery_state size - Refactor: extract _enter_booting/_enter_ready/_enter_wait_for_self_recovery helpers - Refactor: extract _should_handle_hw_offline for offline-state guard clarity - Refactor: extract _handle_boot_timeout_expired shared by Booting and PowerCycle - Add comprehensive unit tests for all new behaviors Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 520 ++++++++++-------- sonic-chassisd/tests/mock_platform.py | 10 +- .../tests/test_dpu_auto_recovery.py | 341 ++++++++---- .../tests/testbed/test_dpu_auto_recovery.py | 249 ++++++++- 4 files changed, 765 insertions(+), 355 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index a7a363109..ac2c914a0 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -18,6 +18,7 @@ try: import json import glob from datetime import datetime, timezone + from typing import NamedTuple from sonic_py_common import daemon_base, logger, device_info from sonic_py_common.task_base import ProcessTaskBase @@ -105,13 +106,16 @@ MODULE_ADMIN_UP = 1 MODULE_REBOOT_CAUSE_DIR = "/host/reboot-cause/module/" MAX_HISTORY_FILES = 10 +# DPU_STATE table in CHASSIS_STATE_DB +DPU_STATE_TABLE = 'DPU_STATE' + DP_STATE = 'dpu_data_plane_state' DP_UPDATE_TIME = 'dpu_data_plane_time' CP_STATE = 'dpu_control_plane_state' CP_UPDATE_TIME = 'dpu_control_plane_time' MP_STATE = 'dpu_midplane_link_state' -# New DPU recovery DB fields (CHASSIS_STATE_DB DPU_STATE table) +# DPU recovery DB fields (CHASSIS_STATE_DB DPU_STATE table) READY_STATUS = 'ready_status' RECOVERY_STATUS = 'recovery_status' RESET_COUNT = 'reset_count' @@ -122,11 +126,16 @@ LAST_READY_TIME = 'last_ready_time' RECOVERY_RECOVERABLE = 'recoverable' RECOVERY_UNRECOVERABLE = 'unrecoverable' -# DPU auto-recovery feature name in CONFIG_DB FEATURE table -DPU_AUTO_RECOVERY_FEATURE = 'dpu-auto-recovery' -DPU_AUTO_RECOVERY_STATE_ENABLED = 'enabled' -DPU_AUTO_RECOVERY_STATE_DISABLED = 'disabled' -DPU_AUTO_RECOVERY_STATE_ALWAYS_DISABLED = 'always_disabled' +# DPU auto-recovery field in CONFIG_DB DEVICE_METADATA|localhost +DPU_AUTO_RECOVERY_FIELD = 'dpu_auto_recovery' +DPU_AUTO_RECOVERY_ENABLED = 'enabled' +DPU_AUTO_RECOVERY_DISABLED = 'disabled' + +# Platform.json field names for recovery thresholds +PLATFORM_CFG_DPU_REBOOT_TIMEOUT = 'dpu_reboot_timeout' +PLATFORM_CFG_DPU_RESET_LIMIT = 'dpu_reset_limit' +PLATFORM_CFG_DPU_BOOT_TIMEOUT = 'dpu_boot_timeout' +PLATFORM_CFG_DPU_SELF_RECOVERY_TIMEOUT = 'dpu_self_recovery_timeout' # Default thresholds (configurable via platform.json) DEFAULT_DPU_BOOT_TIMEOUT = 600 @@ -139,8 +148,8 @@ MINIMUM_SELF_RECOVERY_POLL_COUNT = 3 REBOOT_CAUSE_FILE = "/host/reboot-cause/reboot-cause.txt" NPU_CRASH_REBOOT_CAUSE_KEYWORDS = ("kernel panic", "unknown") -# Planned operation tracking (STATE_DB CHASSIS_MODULE_TABLE) -STATE_TRANSITION_IN_PROGRESS = 'state_transition_in_progress' +# Internal recovery dict key for tracking prior Unrecoverable state +WAS_UNRECOVERABLE_KEY = 'was_unrecoverable' # DPU recovery states DPU_STATE_BOOTING = 'Booting' @@ -148,10 +157,17 @@ DPU_STATE_READY = 'Ready' DPU_STATE_WAIT_FOR_SELF_RECOVERY = 'WaitForSelfRecovery' DPU_STATE_POWER_CYCLE = 'PowerCycle' DPU_STATE_MANUAL_INTERVENTION = 'ManualIntervention' -DPU_STATE_OFFLINE = 'Offline' +DPU_STATE_ADMIN_DOWN = 'AdminDown' DPU_STATE_UNRECOVERABLE = 'Unrecoverable' +class DpuStates(NamedTuple): + """Container for DPU plane states to avoid positional confusion.""" + mp_state: str + cp_state: str + dp_state: str + + # This daemon should return non-zero exit code so that supervisord will # restart it automatically. exit_code = 0 @@ -752,10 +768,14 @@ class SmartSwitchModuleUpdater(ModuleUpdater): self.hostname_table = swsscommon.Table(self.chassis_state_db, CHASSIS_MODULE_HOSTNAME_TABLE) self.module_reboot_table = swsscommon.Table(self.chassis_state_db, CHASSIS_MODULE_REBOOT_INFO_TABLE) - self.dpu_state_table = swsscommon.Table(self.chassis_state_db, 'DPU_STATE') + self.dpu_state_table = swsscommon.Table(self.chassis_state_db, DPU_STATE_TABLE) self.down_modules = {} self.chassis_app_db_clean_sha = None + # Cache CONFIG_DB DEVICE_METADATA for auto-recovery checks + self.config_db = daemon_base.db_connect("CONFIG_DB") + self.device_metadata_table = swsscommon.Table(self.config_db, 'DEVICE_METADATA') + self.midplane_initialized = try_get(chassis.init_midplane_switch, default=False) if not self.midplane_initialized: self.log_error("Chassisd midplane intialization failed") @@ -771,10 +791,10 @@ class SmartSwitchModuleUpdater(ModuleUpdater): try: with open(PLATFORM_JSON_FILE, 'r') as f: platform_cfg = json.load(f) - self.dpu_reboot_timeout = int(platform_cfg.get("dpu_reboot_timeout", DEFAULT_DPU_REBOOT_TIMEOUT)) - self.reset_limit = int(platform_cfg.get("dpu_reset_limit", DEFAULT_DPU_RESET_LIMIT)) - self.dpu_boot_timeout = int(platform_cfg.get("dpu_boot_timeout", DEFAULT_DPU_BOOT_TIMEOUT)) - self.dpu_self_recovery_timeout = int(platform_cfg.get("dpu_self_recovery_timeout", DEFAULT_DPU_SELF_RECOVERY_TIMEOUT)) + self.dpu_reboot_timeout = int(platform_cfg.get(PLATFORM_CFG_DPU_REBOOT_TIMEOUT, DEFAULT_DPU_REBOOT_TIMEOUT)) + self.reset_limit = int(platform_cfg.get(PLATFORM_CFG_DPU_RESET_LIMIT, DEFAULT_DPU_RESET_LIMIT)) + self.dpu_boot_timeout = int(platform_cfg.get(PLATFORM_CFG_DPU_BOOT_TIMEOUT, DEFAULT_DPU_BOOT_TIMEOUT)) + self.dpu_self_recovery_timeout = int(platform_cfg.get(PLATFORM_CFG_DPU_SELF_RECOVERY_TIMEOUT, DEFAULT_DPU_SELF_RECOVERY_TIMEOUT)) except (json.JSONDecodeError, ValueError) as e: self.log_error("Error parsing {}: {}".format(PLATFORM_JSON_FILE, e)) except Exception as e: @@ -1164,7 +1184,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): self.log_notice("Module {} midplane connectivity is up".format(module_key)) # Update midplane state in the chassisStateDB DPU_STATE table - key = "DPU_STATE|" + module_key + key = DPU_STATE_TABLE + "|" + module_key dpu_mp_state = self.get_dpu_midplane_state(key) if midplane_access and dpu_mp_state != 'up': self.update_dpu_state(key, 'up') @@ -1188,37 +1208,33 @@ class SmartSwitchModuleUpdater(ModuleUpdater): keys = self.chassis_state_db.keys(pattern) if keys: for key in keys: - if not "DPU_STATE" in key and not "REBOOT_CAUSE" in key: + if DPU_STATE_TABLE not in key and not "REBOOT_CAUSE" in key: self.chassis_state_db.delete(key) return def _is_auto_recovery_enabled(self): - """Check if dpu-auto-recovery feature is enabled in CONFIG_DB FEATURE table. + """Check if dpu_auto_recovery is enabled in CONFIG_DB DEVICE_METADATA|localhost. - Returns True only when ``state == 'enabled'``. Both ``'disabled'`` and - ``'always_disabled'`` suppress automatic power-cycle recovery; chassisd - still updates DPU state fields in CHASSIS_STATE_DB but will not - initiate a power-cycle. + Returns True when the field is 'enabled'. + Returns False when the field is 'disabled' (or absent, defaulting to disabled). """ try: - config_db = daemon_base.db_connect("CONFIG_DB") - feature_table = swsscommon.Table(config_db, 'FEATURE') - fvs = feature_table.get(DPU_AUTO_RECOVERY_FEATURE) + fvs = self.device_metadata_table.get('localhost') if isinstance(fvs, list) and fvs[0] is True: fvs = dict(fvs[-1]) - state = fvs.get('state', DPU_AUTO_RECOVERY_STATE_ENABLED) - return state == DPU_AUTO_RECOVERY_STATE_ENABLED + state = fvs.get(DPU_AUTO_RECOVERY_FIELD, DPU_AUTO_RECOVERY_DISABLED) + return state == DPU_AUTO_RECOVERY_ENABLED except Exception as e: - self.log_error("Failed to read dpu-auto-recovery feature state: {}".format(e)) + self.log_error("Failed to read dpu_auto_recovery from DEVICE_METADATA: {}".format(e)) return False # Default to disabled def _get_dpu_states(self, module_name): """Get current DPU states from CHASSIS_STATE_DB.""" - dpu_key = "DPU_STATE|" + module_name + dpu_key = DPU_STATE_TABLE + "|" + module_name mp_state = self.get_dpu_midplane_state(dpu_key) _, cp_state = self.dpu_state_table.hget(module_name, CP_STATE) _, dp_state = self.dpu_state_table.hget(module_name, DP_STATE) - return mp_state, cp_state, dp_state + return DpuStates(mp_state=mp_state, cp_state=cp_state, dp_state=dp_state) def _set_ready_status(self, module_name, status): """Set ready_status for a DPU in CHASSIS_STATE_DB.""" @@ -1240,45 +1256,127 @@ class SmartSwitchModuleUpdater(ModuleUpdater): """Set last_ready_time for a DPU in CHASSIS_STATE_DB.""" self.dpu_state_table.hset(module_name, LAST_READY_TIME, get_formatted_time()) - def _is_planned_transition_in_progress(self, module_name): + def _is_planned_transition_in_progress(self, module_name, module_index): """Check if a planned operation (shutdown/reboot) is in progress for this DPU. - Reads ``state_transition_in_progress`` from STATE_DB CHASSIS_MODULE_TABLE. - Returns True when a planned transition is active, suppressing auto-recovery. + Uses the platform API get_module_state_transition() which handles + timeout-based auto-clearing. Returns True when a non-recovery + transition is active, suppressing auto-recovery. + Ignores transition_type == 'recovery' so chassisd does not suppress itself. """ + module = self.chassis.get_module(module_index) + in_progress = try_get(module.get_module_state_transition, module_name, default=False) + if not in_progress: + return False + # Check transition_type — only suppress for non-recovery transitions try: fvs = self.module_table.get(module_name) if isinstance(fvs, list) and fvs[0] is True: fvs = dict(fvs[-1]) - return fvs.get(STATE_TRANSITION_IN_PROGRESS, 'False') == 'True' - except Exception as e: - self.log_warning("{}: Failed to read state_transition_in_progress: {}".format( - module_name, e)) - return False + return fvs.get('transition_type') != 'recovery' + except Exception: + pass + return True + + def _enter_power_cycle_or_unrecoverable(self, module_name, module_index): + """Attempt to power-cycle a DPU, or mark it unrecoverable if reset_limit reached. - def _power_cycle_dpu(self, module_name, module_index): - """Power-cycle a DPU and increment reset_count.""" + Checks reset_limit BEFORE issuing the power-cycle to avoid a + wasted reset that would immediately be marked unrecoverable. + """ recovery = self.dpu_recovery_state[module_name] + + # Check limit before issuing power-cycle + if recovery['reset_count'] >= self.reset_limit: + recovery['state'] = DPU_STATE_UNRECOVERABLE + self._set_recovery_status(module_name, RECOVERY_UNRECOVERABLE) + self.log_error("{}: Reached reset_limit ({}). Marking as unrecoverable.".format( + module_name, self.reset_limit)) + return + recovery['reset_count'] += 1 self._set_reset_count(module_name, recovery['reset_count']) - self.log_warning("{}: Initiating power-cycle (reset_count={})".format( - module_name, recovery['reset_count'])) + self.log_warning("{}: Initiating power-cycle (reset_count={}/{})".format( + module_name, recovery['reset_count'], self.reset_limit)) + + module = self.chassis.get_module(module_index) + + # Acquire state transition lock to prevent concurrent planned + # shutdown/reboot from operating on this DPU during power-cycle. + # If the lock cannot be acquired (another operation holds it), + # abort the power-cycle — the other operation owns this DPU. + lock_acquired = try_get(module.set_module_state_transition, module_name, "recovery", default=False) + if not lock_acquired: + self.log_warning("{}: Cannot acquire state transition lock; " + "another operation in progress — skipping power-cycle".format(module_name)) + recovery['reset_count'] -= 1 + self._set_reset_count(module_name, recovery['reset_count']) + return + + # Re-check transition_in_progress after acquiring our lock to close + # the TOCTOU window where gnoi/CLI sets transition_in_progress + # between _is_planned_transition_in_progress() and here. + if self._is_planned_transition_in_progress(module_name, module_index): + self.log_warning("{}: transition_in_progress detected " + "after lock acquisition — aborting power-cycle".format(module_name)) + try_get(module.clear_module_state_transition, module_name, default=False) + recovery['reset_count'] -= 1 + self._set_reset_count(module_name, recovery['reset_count']) + return try: - module = self.chassis.get_module(module_index) + try_get(module.pci_detach, default=False) try_get(module.set_admin_state, MODULE_ADMIN_DOWN, default=False) try_get(module.set_admin_state, MODULE_ADMIN_UP, default=False) + try_get(module.pci_reattach, default=False) except Exception as e: self.log_error("{}: Power-cycle failed: {}".format(module_name, e)) - - if recovery['reset_count'] >= self.reset_limit: - recovery['state'] = DPU_STATE_UNRECOVERABLE - self._set_recovery_status(module_name, RECOVERY_UNRECOVERABLE) - self.log_error("{}: Reached reset_limit ({}). Marking as unrecoverable.".format( - module_name, self.reset_limit)) + finally: + try_get(module.clear_module_state_transition, module_name, default=False) + + recovery['state'] = DPU_STATE_POWER_CYCLE + recovery['boot_start_time'] = time.time() + + def _enter_booting(self, name, recovery): + """Transition a DPU into the Booting state.""" + recovery['state'] = DPU_STATE_BOOTING + recovery['boot_start_time'] = time.time() + recovery.pop('boot_dp_warning_logged', None) + + def _enter_ready(self, name, recovery, context_msg): + """Transition a DPU into the Ready state.""" + recovery['state'] = DPU_STATE_READY + self._set_ready_status(name, 'true') + self._set_last_ready_time(name) + self.log_info("{}: {}".format(name, context_msg)) + + def _enter_wait_for_self_recovery(self, name, recovery): + """Transition a DPU into the WaitForSelfRecovery state.""" + recovery['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + recovery['self_recovery_start_time'] = time.time() + recovery['self_recovery_poll_count'] = 0 + + def _should_handle_hw_offline(self, current_state): + """Return True if the DPU should react to oper_status Offline. + + States already handling failure (Booting, PowerCycle, + WaitForSelfRecovery, ManualIntervention) should not re-trigger. + """ + return current_state not in ( + DPU_STATE_BOOTING, DPU_STATE_POWER_CYCLE, + DPU_STATE_WAIT_FOR_SELF_RECOVERY, DPU_STATE_MANUAL_INTERVENTION) + + def _handle_boot_timeout_expired(self, name, module_index, recovery, + auto_recovery_enabled, context): + """Handle boot timeout expiry — power-cycle or manual intervention.""" + self.log_warning("{}: Boot timeout ({}s) expired {}".format( + name, self.dpu_boot_timeout, context)) + self._set_ready_status(name, 'false') + self._set_last_down_time(name) + if auto_recovery_enabled: + self._enter_power_cycle_or_unrecoverable(name, module_index) else: - recovery['state'] = DPU_STATE_POWER_CYCLE - recovery['boot_start_time'] = time.time() + recovery['state'] = DPU_STATE_MANUAL_INTERVENTION def init_dpu_recovery_state(self): """Initialize recovery DB fields for all DPUs on chassisd startup. @@ -1300,9 +1398,8 @@ class SmartSwitchModuleUpdater(ModuleUpdater): self._set_recovery_status(name, RECOVERY_RECOVERABLE) self._set_reset_count(name, 0) self._set_last_down_time(name) - self.dpu_recovery_state[name]['state'] = DPU_STATE_BOOTING + self._enter_booting(name, self.dpu_recovery_state[name]) self.dpu_recovery_state[name]['reset_count'] = 0 - self.dpu_recovery_state[name]['boot_start_time'] = time.time() if not npu_crash: continue @@ -1316,12 +1413,14 @@ class SmartSwitchModuleUpdater(ModuleUpdater): if not self._is_auto_recovery_enabled(): self.dpu_recovery_state[name]['state'] = DPU_STATE_MANUAL_INTERVENTION continue - self._power_cycle_dpu(name, module_index) + self._enter_power_cycle_or_unrecoverable(name, module_index) def _npu_crash_on_last_boot(self): """Return True if the last NPU reboot cause looks like a crash/OOM/unknown.""" try: if not os.path.isfile(REBOOT_CAUSE_FILE): + # No reboot-cause file means this is likely a fresh boot or the + # file was not written — assume no crash to avoid false positives. return False with open(REBOOT_CAUSE_FILE, 'r') as f: cause = f.read().strip().lower() @@ -1340,7 +1439,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): DPU transitions to ManualIntervention and waits for operator action. Recovery is suppressed while a planned operation (shutdown/reboot) is - in progress for a DPU (state_transition_in_progress == True). + in progress for a DPU (transition_in_progress == True). """ auto_recovery_enabled = self._is_auto_recovery_enabled() @@ -1348,181 +1447,170 @@ class SmartSwitchModuleUpdater(ModuleUpdater): name = try_get(self.chassis.get_module(module_index).get_name) if name not in self.dpu_recovery_state: continue + self._process_single_dpu_recovery(name, module_index, auto_recovery_enabled) + + def _process_single_dpu_recovery(self, name, module_index, auto_recovery_enabled): + """Process recovery state machine for a single DPU.""" + recovery = self.dpu_recovery_state[name] + current_state = recovery['state'] + states = self._get_dpu_states(name) + oper_status = self.get_module_current_status(name) + admin_status = self.get_module_admin_status(name) + + all_up = (states.mp_state == 'up' and states.cp_state == 'up' and states.dp_state == 'up') + + # DPU is administratively down — no automatic recovery; just + # reflect the offline state and wait for `module startup`. + # This check must precede the Unrecoverable check so that an + # operator can shut down an unrecoverable DPU and then restart it. + if admin_status == 'down': + if current_state != DPU_STATE_ADMIN_DOWN: + if current_state == DPU_STATE_UNRECOVERABLE: + recovery[WAS_UNRECOVERABLE_KEY] = True + recovery['state'] = DPU_STATE_ADMIN_DOWN + self._set_ready_status(name, 'false') + return - recovery = self.dpu_recovery_state[name] - current_state = recovery['state'] - mp_state, cp_state, dp_state = self._get_dpu_states(name) - oper_status = self.get_module_current_status(name) - admin_status = self.get_module_admin_status(name) - - all_up = (mp_state == 'up' and cp_state == 'up' and dp_state == 'up') - - # DPU is administratively down — no automatic recovery; just - # reflect the offline state and wait for `module startup`. - # This check must precede the Unrecoverable check so that an - # operator can shut down an unrecoverable DPU and then restart it. - if admin_status == 'down': - if current_state != DPU_STATE_OFFLINE: - if current_state == DPU_STATE_UNRECOVERABLE: - recovery['was_unrecoverable'] = True - recovery['state'] = DPU_STATE_OFFLINE - self._set_ready_status(name, 'false') - continue + # Terminal: requires operator to set admin-down then admin-up, + # which triggers the admin-down check above and transitions to + # AdminDown state, clearing the Unrecoverable condition. + if current_state == DPU_STATE_UNRECOVERABLE: + return - # Terminal: requires chassisd reset or operator action - if current_state == DPU_STATE_UNRECOVERABLE: - continue + # Suppress recovery while a planned operation (shutdown/reboot) + # is in progress. The control plane goes down during planned + # operations; we must not misinterpret that as a failure. + if self._is_planned_transition_in_progress(name, module_index): + return - # Suppress recovery while a planned operation (shutdown/reboot) - # is in progress. The control plane goes down during planned - # operations; we must not misinterpret that as a failure. - if self._is_planned_transition_in_progress(name): - continue + # Admin-up DPU went Offline at the hardware level — treat as a + # hardware failure. + if oper_status == str(ModuleBase.MODULE_STATUS_OFFLINE) and \ + self._should_handle_hw_offline(current_state): + self.log_warning( + "{}: Hardware failure detected (oper_status: Offline)".format(name)) + self._set_ready_status(name, 'false') + self._set_last_down_time(name) + if auto_recovery_enabled: + self._enter_power_cycle_or_unrecoverable(name, module_index) + else: + recovery['state'] = DPU_STATE_MANUAL_INTERVENTION + return + + # Admin transitioned from down -> up (DPU must be admin-up if + # execution reaches here); restart the recovery FSM. + # If DPU was previously Unrecoverable, operator module startup + # resets recovery_status and reset_count. + if current_state == DPU_STATE_ADMIN_DOWN: + if recovery.pop(WAS_UNRECOVERABLE_KEY, False): + recovery['reset_count'] = 0 + self._set_reset_count(name, 0) + self._set_recovery_status(name, RECOVERY_RECOVERABLE) + self.log_notice("{}: Operator module startup — resetting recovery state".format(name)) + self._enter_booting(name, recovery) + return + + if current_state == DPU_STATE_BOOTING: + if all_up: + self._enter_ready(name, recovery, "DPU is ready (all states up)") + elif time.time() - recovery.get('boot_start_time', time.time()) >= self.dpu_boot_timeout: + # Boot timeout expired. Per HLD, only trigger power-cycle + # if CP or midplane is still down. If CP is up but DP + # hasn't converged, just log a warning — the DPU SONiC + # stack is running but the data plane pipeline hasn't come up. + if states.cp_state == 'up' and states.mp_state == 'up': + # Data plane not ready but control plane is up — warning only. + if not recovery.get('boot_dp_warning_logged'): + self.log_warning("{}: data plane not up after {}s".format( + name, self.dpu_boot_timeout)) + recovery['boot_dp_warning_logged'] = True + else: + # CP or midplane still down — genuine boot failure. + self._handle_boot_timeout_expired( + name, module_index, recovery, + auto_recovery_enabled, "without reaching Ready") + return - # Admin-up DPU went Offline at the hardware level — treat as a - # hardware failure. - if oper_status == str(ModuleBase.MODULE_STATUS_OFFLINE) and \ - current_state not in (DPU_STATE_BOOTING, DPU_STATE_POWER_CYCLE, - DPU_STATE_WAIT_FOR_SELF_RECOVERY, - DPU_STATE_MANUAL_INTERVENTION): - self.log_warning( - "{}: Hardware failure detected (oper_status: Offline)".format(name)) + if current_state == DPU_STATE_READY: + # Per HLD: midplane down or control-plane down triggers + # recovery. Data-plane-down alone does not trigger recovery. + if states.mp_state == 'down' or states.cp_state == 'down': + self.log_warning("{}: failure detected (mp={}, cp={})".format( + name, states.mp_state, states.cp_state)) self._set_ready_status(name, 'false') self._set_last_down_time(name) if auto_recovery_enabled: - self._power_cycle_dpu(name, module_index) + # Enter WaitForSelfRecovery with a grace period. + self._enter_wait_for_self_recovery(name, recovery) else: + # Per HLD: skip self-recovery wait when auto-recovery + # is disabled; go directly to ManualIntervention. recovery['state'] = DPU_STATE_MANUAL_INTERVENTION - continue - - # Admin transitioned from down -> up; restart the recovery FSM. - # If DPU was previously Unrecoverable, operator module startup - # resets recovery_status and reset_count. - if current_state == DPU_STATE_OFFLINE: - if recovery.pop('was_unrecoverable', False): - recovery['reset_count'] = 0 - self._set_reset_count(name, 0) - self._set_recovery_status(name, RECOVERY_RECOVERABLE) - self.log_notice("{}: Operator module startup — resetting recovery state".format(name)) - recovery['state'] = DPU_STATE_BOOTING - recovery['boot_start_time'] = time.time() - continue - - if current_state == DPU_STATE_BOOTING: - if all_up: - recovery['state'] = DPU_STATE_READY + elif states.dp_state == 'down': + # Data plane down only: mark not ready but no recovery. + # Only write to DB on transition to avoid redundant writes. + if recovery.get('dp_was_down') is not True: + self._set_ready_status(name, 'false') + self._set_last_down_time(name) + recovery['dp_was_down'] = True + else: + # All states up — steady state. Only write on transition + # back from dp-down to avoid redundant writes every cycle. + if recovery.get('dp_was_down') is True: self._set_ready_status(name, 'true') self._set_last_ready_time(name) - self.log_info("{}: DPU is ready (all states up)".format(name)) - elif time.time() - recovery.get('boot_start_time', time.time()) >= self.dpu_boot_timeout: - # Boot timeout expired. Per HLD, only trigger power-cycle - # if CP or midplane is still down. If CP is up but DP - # hasn't converged, just log a warning — the DPU SONiC - # stack is running but the data plane pipeline hasn't come up. - if cp_state == 'up' and mp_state == 'up': - # Data plane not ready but control plane is up — warning only. - if not recovery.get('boot_dp_warning_logged'): - self.log_warning("{}: data plane not up after {}s".format( - name, self.dpu_boot_timeout)) - recovery['boot_dp_warning_logged'] = True - else: - # CP or midplane still down — genuine boot failure. - self.log_warning("{}: Boot timeout ({}s) expired without reaching Ready".format( - name, self.dpu_boot_timeout)) - self._set_ready_status(name, 'false') - self._set_last_down_time(name) - if auto_recovery_enabled: - self._power_cycle_dpu(name, module_index) - else: - recovery['state'] = DPU_STATE_MANUAL_INTERVENTION - continue + recovery['dp_was_down'] = False + return - if current_state == DPU_STATE_READY: - # Per HLD: midplane down or control-plane down triggers - # entry into WaitForSelfRecovery with a grace period. - # Data-plane-down alone does not trigger recovery. - if mp_state == 'down' or cp_state == 'down': - self.log_warning("{}: failure detected (mp={}, cp={}); entering WaitForSelfRecovery".format( - name, mp_state, cp_state)) - recovery['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - recovery['self_recovery_start_time'] = time.time() - recovery['self_recovery_poll_count'] = 0 - self._set_ready_status(name, 'false') - self._set_last_down_time(name) - elif dp_state == 'down': - # Data plane down only: mark not ready but no recovery. - # Only write to DB on transition to avoid redundant writes. - if recovery.get('dp_was_down') is not True: - self._set_ready_status(name, 'false') - self._set_last_down_time(name) - recovery['dp_was_down'] = True + if current_state == DPU_STATE_WAIT_FOR_SELF_RECOVERY: + # Grace period: allow the DPU to self-recover from transient + # failures (process restart, HW watchdog reboot) without + # external intervention. + # Minimal grace: wait at least 30s or 3 polling intervals + # before evaluating recovery conditions. + elapsed = time.time() - recovery.get('self_recovery_start_time', time.time()) + recovery['self_recovery_poll_count'] = recovery.get('self_recovery_poll_count', 0) + 1 + if elapsed < MINIMUM_SELF_RECOVERY_GRACE_PERIOD and \ + recovery['self_recovery_poll_count'] < MINIMUM_SELF_RECOVERY_POLL_COUNT: + return + if states.mp_state == 'up' or states.cp_state == 'up': + # DPU is self-recovering — transition to Booting and wait + # for full readiness (all planes up) within dpu_boot_timeout. + self.log_notice("{}: self-recovering (mp={}, cp={}); entering Booting".format( + name, states.mp_state, states.cp_state)) + self._enter_booting(name, recovery) + elif elapsed >= self.dpu_self_recovery_timeout: + # Timeout expired and both CP and midplane still down. + self.log_warning("{}: self-recovery timeout ({}s) expired; " + "both cp and midplane still down".format( + name, self.dpu_self_recovery_timeout)) + if auto_recovery_enabled: + self._enter_power_cycle_or_unrecoverable(name, module_index) else: - # All states up — steady state. Only write on transition - # back from dp-down to avoid redundant writes every cycle. - if recovery.get('dp_was_down') is True: - self._set_ready_status(name, 'true') - self._set_last_ready_time(name) - recovery['dp_was_down'] = False - continue - - if current_state == DPU_STATE_WAIT_FOR_SELF_RECOVERY: - # Grace period: allow the DPU to self-recover from transient - # failures (process restart, HW watchdog reboot) without - # external intervention. - # Minimal grace: wait at least 30s or 3 polling intervals - # before evaluating recovery conditions. - elapsed = time.time() - recovery.get('self_recovery_start_time', time.time()) - recovery['self_recovery_poll_count'] = recovery.get('self_recovery_poll_count', 0) + 1 - if elapsed < MINIMUM_SELF_RECOVERY_GRACE_PERIOD and \ - recovery['self_recovery_poll_count'] < MINIMUM_SELF_RECOVERY_POLL_COUNT: - continue - if mp_state == 'up' or cp_state == 'up': - # DPU is self-recovering — transition to Booting and wait - # for full readiness (all planes up) within dpu_boot_timeout. - self.log_notice("{}: self-recovering (mp={}, cp={}); entering Booting".format( - name, mp_state, cp_state)) - recovery['state'] = DPU_STATE_BOOTING - recovery['boot_start_time'] = time.time() - recovery.pop('boot_dp_warning_logged', None) - elif elapsed >= self.dpu_self_recovery_timeout: - # Timeout expired and both CP and midplane still down. - self.log_warning("{}: self-recovery timeout ({}s) expired; " - "both cp and midplane still down".format( - name, self.dpu_self_recovery_timeout)) - if auto_recovery_enabled: - self._power_cycle_dpu(name, module_index) - else: - recovery['state'] = DPU_STATE_MANUAL_INTERVENTION - continue + recovery['state'] = DPU_STATE_MANUAL_INTERVENTION + return - if current_state == DPU_STATE_POWER_CYCLE: - if all_up: - recovery['state'] = DPU_STATE_READY - self._set_ready_status(name, 'true') - self._set_last_ready_time(name) - self.log_info("{}: DPU recovered after power-cycle".format(name)) - elif time.time() - recovery.get('boot_start_time', time.time()) >= self.dpu_boot_timeout: - # DPU did not recover within dpu_boot_timeout after power-cycle - self.log_warning("{}: Boot timeout ({}s) expired after power-cycle".format( - name, self.dpu_boot_timeout)) - if auto_recovery_enabled: - self._power_cycle_dpu(name, module_index) - else: - recovery['state'] = DPU_STATE_MANUAL_INTERVENTION - continue + if current_state == DPU_STATE_POWER_CYCLE: + if all_up: + self._enter_ready(name, recovery, "DPU recovered after power-cycle") + elif time.time() - recovery.get('boot_start_time', time.time()) >= self.dpu_boot_timeout: + # DPU did not recover within dpu_boot_timeout after power-cycle + self._handle_boot_timeout_expired( + name, module_index, recovery, + auto_recovery_enabled, "after power-cycle") + return - if current_state == DPU_STATE_MANUAL_INTERVENTION: - # Operator may have recovered the DPU manually. - if all_up: - recovery['state'] = DPU_STATE_READY - self._set_ready_status(name, 'true') - self._set_last_ready_time(name) - self.log_info("{}: DPU recovered (manual intervention)".format(name)) - elif auto_recovery_enabled: - # Auto-recovery was re-enabled while the DPU is still - # down — start a power-cycle. - self._power_cycle_dpu(name, module_index) - continue + if current_state == DPU_STATE_MANUAL_INTERVENTION: + # Operator may have recovered the DPU manually, or can + # set admin-down (caught by admin-down check above) to + # transition to AdminDown state. + if all_up: + self._enter_ready(name, recovery, "DPU recovered (manual intervention)") + elif auto_recovery_enabled: + # Auto-recovery was re-enabled while the DPU is still + # down — start a power-cycle. + self._enter_power_cycle_or_unrecoverable(name, module_index) + return # @@ -1661,7 +1749,7 @@ class DpuStateUpdater(logger.Logger): self.id = self.chassis.get_dpu_id() self.name = f'DPU{self.id}' - self.dpu_state_table = swsscommon.Table(self.chassis_state_db, 'DPU_STATE') + self.dpu_state_table = swsscommon.Table(self.chassis_state_db, DPU_STATE_TABLE) def _get_data_plane_state_common(self): port_table = swsscommon.Table(self.app_db, 'PORT_TABLE') @@ -1782,7 +1870,7 @@ class ChassisdDaemon(daemon_base.DaemonBase): op = MODULE_ADMIN_DOWN # Initialize DPU_STATE DB table on bootup - dpu_state_key = "DPU_STATE|" + module_name + dpu_state_key = DPU_STATE_TABLE + "|" + module_name if operational_state == ModuleBase.MODULE_STATUS_ONLINE: op_state = 'up' else: diff --git a/sonic-chassisd/tests/mock_platform.py b/sonic-chassisd/tests/mock_platform.py index 1f7b68f69..f8cf00765 100644 --- a/sonic-chassisd/tests/mock_platform.py +++ b/sonic-chassisd/tests/mock_platform.py @@ -86,6 +86,14 @@ def clear_module_state_transition(self, module_name): """Mock implementation of clear_module_state_transition""" return True + def set_module_state_transition(self, module_name, transition_type): + """Mock implementation of set_module_state_transition""" + return True + + def get_module_state_transition(self, module_name): + """Mock implementation of get_module_state_transition""" + return False + def clear_module_gnoi_halt_in_progress(self): """Mock implementation of clear_module_gnoi_halt_in_progress""" return True @@ -225,7 +233,7 @@ def get_revision(self): def is_smartswitch(self): return self._is_smartswitch - + def get_dataplane_state(self): raise NotImplementedError diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index b80ea84c6..1bbf2133b 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -6,7 +6,7 @@ Offline, ManualIntervention, Unrecoverable) - DPU_STATE DB fields: ready_status, recovery_status, reset_count, last_down_time, last_ready_time -- Auto-recovery feature flag gating (enabled/disabled/always_disabled) +- Auto-recovery DEVICE_METADATA flag gating (enabled/disabled) - Recovery initialization on chassisd startup (init_dpu_recovery_state) - NPU crash detection and forced power-cycle on boot - Reset limit enforcement and unrecoverable marking @@ -39,7 +39,7 @@ DPU_STATE_READY, DPU_STATE_WAIT_FOR_SELF_RECOVERY, DPU_STATE_POWER_CYCLE, - DPU_STATE_OFFLINE, + DPU_STATE_ADMIN_DOWN, DPU_STATE_MANUAL_INTERVENTION, DPU_STATE_UNRECOVERABLE, READY_STATUS, @@ -59,6 +59,7 @@ MODULE_REBOOT_CAUSE_DIR, MAX_HISTORY_FILES, REBOOT_CAUSE_FILE, + WAS_UNRECOVERABLE_KEY, ) @@ -156,7 +157,7 @@ def test_init_npu_crash_power_cycles_admin_up_dpus(self): with patch.object(updater, '_npu_crash_on_last_boot', return_value=True), \ patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.init_dpu_recovery_state() # DPU0 (online) should be power-cycled, DPU1 (offline) should not @@ -216,62 +217,51 @@ def test_missing_file_not_crash(self): # ============================================================================ -# Test: Auto-recovery feature flag +# Test: Auto-recovery DEVICE_METADATA flag # ============================================================================ class TestAutoRecoveryFeatureFlag: - """Test _is_auto_recovery_enabled() feature flag checking.""" + """Test _is_auto_recovery_enabled() reading from DEVICE_METADATA.""" def test_enabled_returns_true(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - mock_table = MagicMock() - mock_table.get.return_value = [True, (('state', 'enabled'),)] - - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_table): - assert updater._is_auto_recovery_enabled() is True + updater.device_metadata_table = MagicMock() + updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + assert updater._is_auto_recovery_enabled() is True def test_disabled_returns_false(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - mock_table = MagicMock() - mock_table.get.return_value = [True, (('state', 'disabled'),)] - - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_table): - assert updater._is_auto_recovery_enabled() is False + updater.device_metadata_table = MagicMock() + updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disabled'),)] + assert updater._is_auto_recovery_enabled() is False - def test_always_disabled_returns_false(self): + def test_missing_field_defaults_to_disabled(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - mock_table = MagicMock() - mock_table.get.return_value = [True, (('state', 'always_disabled'),)] - - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_table): - assert updater._is_auto_recovery_enabled() is False + updater.device_metadata_table = MagicMock() + updater.device_metadata_table.get.return_value = [True, (('hostname', 'sonic'),)] + assert updater._is_auto_recovery_enabled() is False - def test_missing_feature_returns_false(self): + def test_missing_localhost_defaults_to_disabled(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - mock_table = MagicMock() - mock_table.get.return_value = None - - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_table): - assert updater._is_auto_recovery_enabled() is False + updater.device_metadata_table = MagicMock() + updater.device_metadata_table.get.return_value = None + assert updater._is_auto_recovery_enabled() is False - def test_db_exception_returns_false(self): + def test_db_exception_defaults_to_disabled(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - with patch("chassisd.daemon_base.db_connect", side_effect=Exception("DB error")): - assert updater._is_auto_recovery_enabled() is False + updater.device_metadata_table = MagicMock() + updater.device_metadata_table.get.side_effect = Exception("DB error") + assert updater._is_auto_recovery_enabled() is False # ============================================================================ @@ -290,8 +280,10 @@ def test_planned_transition_in_progress_suppresses_recovery(self): set_dpu_states(updater, "DPU0", mp='up', cp='down', dp='up') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - # Mark a planned transition as in progress - updater.module_table.hset("DPU0", "state_transition_in_progress", "True") + # Mark a planned transition as in progress via platform API mock + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=True) + updater.module_table.hset("DPU0", "transition_in_progress", "True") + updater.module_table.hset("DPU0", "transition_type", "shutdown") with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'): @@ -309,8 +301,8 @@ def test_no_planned_transition_allows_recovery(self): set_dpu_states(updater, "DPU0", mp='up', cp='down', dp='up') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - # No state_transition_in_progress set (or set to False) - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") + # No transition_in_progress set (or set to False) + updater.module_table.hset("DPU0", "transition_in_progress", "False") with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'): @@ -329,14 +321,17 @@ def test_planned_transition_completed_allows_recovery(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) # First poll: transition in progress — suppressed - updater.module_table.hset("DPU0", "state_transition_in_progress", "True") + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=True) + updater.module_table.hset("DPU0", "transition_in_progress", "True") + updater.module_table.hset("DPU0", "transition_type", "shutdown") with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_READY # Second poll: transition done — recovery proceeds - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=False) + updater.module_table.hset("DPU0", "transition_in_progress", "False") with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() @@ -351,7 +346,9 @@ def test_planned_transition_midplane_down_suppressed(self): set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - updater.module_table.hset("DPU0", "state_transition_in_progress", "True") + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=True) + updater.module_table.hset("DPU0", "transition_in_progress", "True") + updater.module_table.hset("DPU0", "transition_type", "shutdown") with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'): @@ -360,11 +357,11 @@ def test_planned_transition_midplane_down_suppressed(self): assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_READY def test_is_planned_transition_missing_field_returns_false(self): - """Missing state_transition_in_progress field returns False (no suppression).""" + """Missing transition_in_progress field returns False (no suppression).""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - # Don't set the field at all — module_table.get returns empty - assert updater._is_planned_transition_in_progress("DPU0") is False + # Don't set the field at all — get_module_state_transition returns False + assert updater._is_planned_transition_in_progress("DPU0", 0) is False # ============================================================================ @@ -426,7 +423,7 @@ def test_booting_timeout_triggers_power_cycle(self): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.update_dpu_recovery_state() mock_pc.assert_called_once_with("DPU0", 0) @@ -483,7 +480,7 @@ def test_power_cycle_timeout_triggers_another_cycle(self): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.update_dpu_recovery_state() mock_pc.assert_called_once_with("DPU0", 0) @@ -533,7 +530,7 @@ def test_booting_timeout_cp_up_dp_down_warning_only(self): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.update_dpu_recovery_state() # Should NOT power-cycle — only log warning @@ -579,7 +576,7 @@ def test_booting_timeout_midplane_down_triggers_power_cycle(self): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.update_dpu_recovery_state() mock_pc.assert_called_once_with("DPU0", 0) @@ -610,11 +607,11 @@ def test_ready_midplane_down_enters_wait_for_self_recovery(self): assert get_dpu_state_field(updater, "DPU0", READY_STATUS) == 'false' assert 'self_recovery_start_time' in updater.dpu_recovery_state["DPU0"] - def test_ready_midplane_down_wait_for_self_recovery_regardless_of_flag(self): - """Ready + midplane down + auto-recovery disabled → still WaitForSelfRecovery. + def test_ready_midplane_down_auto_recovery_disabled_goes_to_manual_intervention(self): + """Ready + midplane down + auto-recovery disabled → ManualIntervention. - The auto-recovery flag only gates the power-cycle action after timeout, - not the initial transition to WaitForSelfRecovery.""" + Per HLD: when auto-recovery is disabled, skip WaitForSelfRecovery and + go directly to ManualIntervention.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_READY @@ -627,7 +624,7 @@ def test_ready_midplane_down_wait_for_self_recovery_regardless_of_flag(self): patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_MANUAL_INTERVENTION def test_ready_cp_down_enters_wait_for_self_recovery(self): """Ready + CP down (midplane up) → WaitForSelfRecovery.""" @@ -987,8 +984,8 @@ def test_unrecoverable_admin_down_transitions_to_offline(self): patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE - assert updater.dpu_recovery_state["DPU0"].get('was_unrecoverable') is True + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN + assert updater.dpu_recovery_state["DPU0"].get(WAS_UNRECOVERABLE_KEY) is True def test_unrecoverable_operator_startup_resets_recovery(self): """Unrecoverable → Offline → Booting resets reset_count and recovery_status. @@ -999,8 +996,8 @@ def test_unrecoverable_operator_startup_resets_recovery(self): """ chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_OFFLINE - updater.dpu_recovery_state["DPU0"]['was_unrecoverable'] = True + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_ADMIN_DOWN + updater.dpu_recovery_state["DPU0"][WAS_UNRECOVERABLE_KEY] = True updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 updater.dpu_state_table.hset("DPU0", RESET_COUNT, '2') updater.dpu_state_table.hset("DPU0", RECOVERY_STATUS, RECOVERY_UNRECOVERABLE) @@ -1090,14 +1087,14 @@ def test_admin_down_transitions_to_offline(self): patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN assert get_dpu_state_field(updater, "DPU0", READY_STATUS) == 'false' def test_admin_up_from_offline_transitions_to_booting(self): """Admin-up from Offline → Booting.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_OFFLINE + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_ADMIN_DOWN set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_PRESENT) @@ -1197,10 +1194,10 @@ def test_hardware_offline_manual_intervention_when_disabled(self): # ============================================================================ class TestPowerCycleDpu: - """Test _power_cycle_dpu() method.""" + """Test _enter_power_cycle_or_unrecoverable() method.""" def test_power_cycle_calls_admin_state(self): - """_power_cycle_dpu should call set_admin_state(DOWN) then set_admin_state(UP).""" + """_enter_power_cycle_or_unrecoverable should call set_admin_state(DOWN) then set_admin_state(UP).""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY @@ -1209,7 +1206,7 @@ def test_power_cycle_calls_admin_state(self): module = chassis.module_list[0] module.set_admin_state = MagicMock() - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) # Should be called twice: once with DOWN, once with UP calls = module.set_admin_state.call_args_list @@ -1218,13 +1215,13 @@ def test_power_cycle_calls_admin_state(self): assert calls[1][0][0] == MODULE_ADMIN_UP def test_power_cycle_increments_reset_count(self): - """Each _power_cycle_dpu call increments reset_count.""" + """Each _enter_power_cycle_or_unrecoverable call increments reset_count.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 3 assert get_dpu_state_field(updater, "DPU0", RESET_COUNT) == '3' @@ -1236,7 +1233,7 @@ def test_power_cycle_at_limit_marks_unrecoverable(self): updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 # limit is 3 - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_UNRECOVERABLE assert get_dpu_state_field(updater, "DPU0", RECOVERY_STATUS) == RECOVERY_UNRECOVERABLE @@ -1491,7 +1488,7 @@ def test_full_cycle_admin_down_up(self): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN # Phase 2: Admin-up (DPU starts booting) with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ @@ -1535,7 +1532,7 @@ def test_admin_down_from_any_state_transitions_to_offline(self, initial_state): patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN assert get_dpu_state_field(updater, "DPU0", READY_STATUS) == 'false' def test_admin_down_from_unrecoverable_goes_offline(self): @@ -1552,14 +1549,14 @@ def test_admin_down_from_unrecoverable_goes_offline(self): patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE - assert updater.dpu_recovery_state["DPU0"].get('was_unrecoverable') is True + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN + assert updater.dpu_recovery_state["DPU0"].get(WAS_UNRECOVERABLE_KEY) is True def test_admin_down_already_offline_stays_offline(self): """Already Offline + admin-down → stays Offline (no-op).""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_OFFLINE + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_ADMIN_DOWN set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_OFFLINE) @@ -1569,7 +1566,7 @@ def test_admin_down_already_offline_stays_offline(self): patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN # ============================================================================ @@ -1601,7 +1598,7 @@ def test_hardware_offline_while_in_recovery_no_duplicate(self, recovery_state): with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, 'get_module_admin_status', return_value='up'), \ - patch.object(updater, '_power_cycle_dpu') as mock_pc: + patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.update_dpu_recovery_state() # The hardware-offline code path should be skipped for these states @@ -1653,7 +1650,7 @@ def test_hardware_offline_from_offline_state_triggers_recovery(self): branch fires (Offline is NOT in the exclusion list), triggering power-cycle.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_OFFLINE + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_ADMIN_DOWN set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_OFFLINE) @@ -1759,7 +1756,7 @@ def test_feature_toggle_cycle_disabled_then_reenabled(self): # ============================================================================ class TestPowerCycleFailure: - """Test _power_cycle_dpu() behavior when set_admin_state raises exceptions.""" + """Test _enter_power_cycle_or_unrecoverable() behavior when set_admin_state raises exceptions.""" def test_power_cycle_set_admin_state_exception(self): """If set_admin_state raises, state should still update (best-effort).""" @@ -1771,7 +1768,7 @@ def test_power_cycle_set_admin_state_exception(self): module = chassis.module_list[0] module.set_admin_state = MagicMock(side_effect=Exception("I2C bus error")) - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) # Despite the exception, reset_count should still increment assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 1 @@ -1789,7 +1786,7 @@ def test_power_cycle_exception_at_limit_still_marks_unrecoverable(self): module = chassis.module_list[0] module.set_admin_state = MagicMock(side_effect=RuntimeError("HW error")) - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_UNRECOVERABLE assert get_dpu_state_field(updater, "DPU0", RECOVERY_STATUS) == RECOVERY_UNRECOVERABLE @@ -1812,11 +1809,121 @@ def side_effect_fn(state): module = chassis.module_list[0] module.set_admin_state = MagicMock(side_effect=side_effect_fn) - updater._power_cycle_dpu("DPU0", 0) + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) # Should still transition (try_get catches the exception) assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 1 + def test_power_cycle_calls_pci_detach_and_reattach(self): + """Power-cycle sequence calls pci_detach before admin_down and pci_reattach after admin_up.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 + + module = chassis.module_list[0] + call_order = [] + module.pci_detach = MagicMock(side_effect=lambda: call_order.append('pci_detach')) + module.set_admin_state = MagicMock(side_effect=lambda s: call_order.append(f'set_admin_state({s})')) + module.pci_reattach = MagicMock(side_effect=lambda: call_order.append('pci_reattach')) + + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) + + # Verify pci_detach and pci_reattach are called + module.pci_detach.assert_called_once() + module.pci_reattach.assert_called_once() + # Verify order: pci_detach → admin_down → admin_up → pci_reattach + assert call_order == ['pci_detach', 'set_admin_state(0)', 'set_admin_state(1)', 'pci_reattach'] + + def test_power_cycle_acquires_and_releases_state_transition(self): + """Power-cycle calls set_module_state_transition before and clear after.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 + + module = chassis.module_list[0] + module.set_module_state_transition = MagicMock(return_value=True) + module.clear_module_state_transition = MagicMock(return_value=True) + + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) + + module.set_module_state_transition.assert_called_once_with("DPU0", "recovery") + module.clear_module_state_transition.assert_called_once_with("DPU0") + + def test_power_cycle_clears_state_transition_on_exception(self): + """clear_module_state_transition is called even if power-cycle raises.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 + + module = chassis.module_list[0] + module.set_module_state_transition = MagicMock(return_value=True) + module.clear_module_state_transition = MagicMock(return_value=True) + module.set_admin_state = MagicMock(side_effect=Exception("HW fault")) + + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) + + module.set_module_state_transition.assert_called_once_with("DPU0", "recovery") + module.clear_module_state_transition.assert_called_once_with("DPU0") + + def test_power_cycle_skipped_when_lock_not_acquired(self): + """If set_module_state_transition returns False, power-cycle is aborted.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 + + module = chassis.module_list[0] + module.set_module_state_transition = MagicMock(return_value=False) + module.clear_module_state_transition = MagicMock() + module.set_admin_state = MagicMock() + module.pci_detach = MagicMock() + module.pci_reattach = MagicMock() + + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) + + # Power-cycle should NOT have been executed + module.pci_detach.assert_not_called() + module.set_admin_state.assert_not_called() + module.pci_reattach.assert_not_called() + module.clear_module_state_transition.assert_not_called() + # reset_count should be rolled back to 0 + assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 + # State should NOT have changed to PowerCycle + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY + + def test_power_cycle_aborted_when_legacy_field_set_after_lock(self): + """If transition_in_progress appears after lock acquired, abort.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 + + module = chassis.module_list[0] + module.set_module_state_transition = MagicMock(return_value=True) + module.clear_module_state_transition = MagicMock(return_value=True) + module.set_admin_state = MagicMock() + module.pci_detach = MagicMock() + module.pci_reattach = MagicMock() + + # Simulate legacy field being set (gnoi started a shutdown concurrently) + updater._is_planned_transition_in_progress = MagicMock(return_value=True) + + updater._enter_power_cycle_or_unrecoverable("DPU0", 0) + + # Lock was acquired but then released due to legacy field detection + module.set_module_state_transition.assert_called_once_with("DPU0", "recovery") + module.clear_module_state_transition.assert_called_once_with("DPU0") + # Power-cycle should NOT have been executed + module.pci_detach.assert_not_called() + module.set_admin_state.assert_not_called() + # reset_count should be rolled back to 0 + assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 + # State should NOT have changed to PowerCycle + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY + # ============================================================================ # Test: DB setter methods @@ -2327,25 +2434,37 @@ def test_is_planned_transition_exception_returns_false(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - # Make module_table.get raise an exception - updater.module_table.get = MagicMock(side_effect=Exception("DB error")) - assert updater._is_planned_transition_in_progress("DPU0") is False + # Make get_module_state_transition raise — try_get returns default=False + chassis.module_list[0].get_module_state_transition = MagicMock(side_effect=Exception("DB error")) + assert updater._is_planned_transition_in_progress("DPU0", 0) is False def test_is_planned_transition_true(self): - """_is_planned_transition_in_progress returns True when field is 'True'.""" + """_is_planned_transition_in_progress returns True when field is 'True' and type != recovery.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.module_table.hset("DPU0", "state_transition_in_progress", "True") - assert updater._is_planned_transition_in_progress("DPU0") is True + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=True) + updater.module_table.hset("DPU0", "transition_in_progress", "True") + updater.module_table.hset("DPU0", "transition_type", "shutdown") + assert updater._is_planned_transition_in_progress("DPU0", 0) is True def test_is_planned_transition_false_value(self): - """_is_planned_transition_in_progress returns False when field is 'False'.""" + """_is_planned_transition_in_progress returns False when get_module_state_transition returns False.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=False) + assert updater._is_planned_transition_in_progress("DPU0", 0) is False + + def test_is_planned_transition_recovery_type_returns_false(self): + """_is_planned_transition_in_progress returns False when transition_type is 'recovery'.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") - assert updater._is_planned_transition_in_progress("DPU0") is False + chassis.module_list[0].get_module_state_transition = MagicMock(return_value=True) + updater.module_table.hset("DPU0", "transition_in_progress", "True") + updater.module_table.hset("DPU0", "transition_type", "recovery") + assert updater._is_planned_transition_in_progress("DPU0", 0) is False def test_npu_crash_file_read_exception(self): """Exception reading reboot cause file returns False.""" @@ -2454,12 +2573,12 @@ def test_booting_to_ready_full_path(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) # Mock only the DB access for _is_auto_recovery_enabled - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] # Mock get_module_admin_status since it creates a new DB connection with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table), \ + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() @@ -2473,13 +2592,13 @@ def test_ready_cp_down_enters_wait_for_self_recovery(self): set_dpu_states(updater, "DPU0", mp='up', cp='down', dp='up') updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") + updater.module_table.hset("DPU0", "transition_in_progress", "False") - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table), \ + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() @@ -2494,15 +2613,15 @@ def test_admin_down_to_offline_full_path(self): set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table), \ + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_OFFLINE + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN def test_hardware_offline_triggers_power_cycle_full_path(self): """Hardware offline with auto-recovery enabled triggers power cycle (full path).""" @@ -2513,16 +2632,16 @@ def test_hardware_offline_triggers_power_cycle_full_path(self): set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") + updater.module_table.hset("DPU0", "transition_in_progress", "False") - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] module = chassis.module_list[0] module.set_admin_state = MagicMock() with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table), \ + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() @@ -2542,16 +2661,16 @@ def test_wait_for_self_recovery_timeout_power_cycle_full_path(self): set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - updater.module_table.hset("DPU0", "state_transition_in_progress", "False") + updater.module_table.hset("DPU0", "transition_in_progress", "False") - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] module = chassis.module_list[0] module.set_admin_state = MagicMock() with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table), \ + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() @@ -2567,8 +2686,8 @@ def test_init_recovery_state_npu_crash_full_path(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) updater.module_table.hset("DPU1", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'enabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] module = chassis.module_list[0] module.set_admin_state = MagicMock() @@ -2578,7 +2697,7 @@ def test_init_recovery_state_npu_crash_full_path(self): with patch("os.path.isfile", return_value=True), \ patch("builtins.open", mock_open(read_data="kernel panic - not syncing")), \ patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table): + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table): updater.init_dpu_recovery_state() # DPU0 should be power-cycled (online + NPU crash + auto-recovery enabled) @@ -2593,13 +2712,13 @@ def test_init_recovery_state_npu_crash_disabled_full_path(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - mock_feature_table = MagicMock() - mock_feature_table.get.return_value = [True, (('state', 'disabled'),)] + mock_device_metadata_table = MagicMock() + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disabled'),)] with patch("os.path.isfile", return_value=True), \ patch("builtins.open", mock_open(read_data="Unknown")), \ patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_feature_table): + patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table): updater.init_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_MANUAL_INTERVENTION diff --git a/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py b/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py index 4019fbbc1..71b6bbe6c 100644 --- a/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py @@ -57,8 +57,8 @@ RECOVERY_RECOVERABLE = 'recoverable' RECOVERY_UNRECOVERABLE = 'unrecoverable' -# Feature flag -DPU_AUTO_RECOVERY_FEATURE = 'dpu-auto-recovery' +# DEVICE_METADATA field for auto-recovery +DPU_AUTO_RECOVERY_FIELD = 'dpu_auto_recovery' # Timeouts DPU_BOOT_TIMEOUT = 420 # 7 minutes for DPU to come up after power-cycle @@ -134,19 +134,19 @@ def get_module_oper_status(dpu_name): return result if result else None -def get_feature_state(feature_name): - """Get feature state from CONFIG_DB FEATURE table.""" +def get_auto_recovery_state(): + """Get dpu_auto_recovery from CONFIG_DB DEVICE_METADATA|localhost.""" result = run_cmd( - f"sonic-db-cli CONFIG_DB HGET 'FEATURE|{feature_name}' 'state'", + "sonic-db-cli CONFIG_DB HGET 'DEVICE_METADATA|localhost' 'dpu_auto_recovery'", check=False ) return result if result else None -def set_feature_state(feature_name, state): - """Set feature state in CONFIG_DB FEATURE table.""" - sonic_db_cli("CONFIG_DB", f"HSET 'FEATURE|{feature_name}' 'state' '{state}'") - logger.info(f"Set feature {feature_name} state to '{state}'") +def set_auto_recovery_state(state): + """Set dpu_auto_recovery in CONFIG_DB DEVICE_METADATA|localhost.""" + sonic_db_cli("CONFIG_DB", f"HSET 'DEVICE_METADATA|localhost' 'dpu_auto_recovery' '{state}'") + logger.info(f"Set dpu_auto_recovery to '{state}'") def get_chassis_module_admin_status(dpu_name): @@ -361,15 +361,15 @@ def test_reset_count_field(results, dpu_name): def test_auto_recovery_feature_flag(results): - """Verify dpu-auto-recovery feature exists in CONFIG_DB (or defaults to enabled).""" - logger.info("Test: dpu-auto-recovery feature flag") - state = get_feature_state(DPU_AUTO_RECOVERY_FEATURE) - valid_states = ('enabled', 'disabled', 'always_disabled') - # Feature may not be explicitly configured (defaults to enabled in chassisd) + """Verify dpu_auto_recovery exists in CONFIG_DB DEVICE_METADATA (or defaults to enabled).""" + logger.info("Test: dpu_auto_recovery in DEVICE_METADATA") + state = get_auto_recovery_state() + valid_states = ('enabled', 'disabled') + # Field may not be explicitly configured (defaults to enabled in chassisd) results.record( "auto_recovery_feature_exists", state is None or state in valid_states, - f"Feature state: '{state}' (expected None/default or one of {valid_states})" + f"dpu_auto_recovery: '{state}' (expected None/default or one of {valid_states})" ) @@ -425,12 +425,12 @@ def test_disable_auto_recovery_no_power_cycle(results, dpu_name): """With auto-recovery disabled, DPU failure should not trigger power-cycle.""" logger.info(f"Test: disabled auto-recovery prevents power-cycle on {dpu_name}") - original_feature_state = get_feature_state(DPU_AUTO_RECOVERY_FEATURE) + original_feature_state = get_auto_recovery_state() original_reset_count = get_dpu_state_field(dpu_name, RESET_COUNT) try: # Disable auto-recovery - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, 'disabled') + set_auto_recovery_state('disabled') time.sleep(POLL_INTERVAL) # Record current reset count @@ -451,9 +451,9 @@ def test_disable_auto_recovery_no_power_cycle(results, dpu_name): f"even with auto-recovery disabled" ) finally: - # Restore original feature state + # Restore original state if original_feature_state: - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, original_feature_state) + set_auto_recovery_state(original_feature_state) # Ensure DPU comes back set_module_admin_status(dpu_name, 'up') wait_for_dpu_ready(dpu_name) @@ -491,14 +491,14 @@ def test_enable_auto_recovery_feature(results, dpu_name): """Enable auto-recovery and verify DPU state reflects it.""" logger.info(f"Test: enable auto-recovery feature for {dpu_name}") - original_state = get_feature_state(DPU_AUTO_RECOVERY_FEATURE) + original_state = get_auto_recovery_state() try: - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, 'enabled') + set_auto_recovery_state('enabled') time.sleep(POLL_INTERVAL) - # Verify the feature is reflected (chassisd should read it next poll) - state = get_feature_state(DPU_AUTO_RECOVERY_FEATURE) + # Verify the field is reflected (chassisd should read it next poll) + state = get_auto_recovery_state() results.record( f"auto_recovery_enabled_{dpu_name}", state == 'enabled', @@ -516,7 +516,7 @@ def test_enable_auto_recovery_feature(results, dpu_name): ) finally: if original_state: - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, original_state) + set_auto_recovery_state(original_state) def test_dpu_power_cycle_recovery(results, dpu_name): @@ -527,8 +527,8 @@ def test_dpu_power_cycle_recovery(results, dpu_name): logger.info(f"Test: DPU power-cycle recovery for {dpu_name}") # Ensure auto-recovery is enabled - original_state = get_feature_state(DPU_AUTO_RECOVERY_FEATURE) - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, 'enabled') + original_state = get_auto_recovery_state() + set_auto_recovery_state('enabled') try: # Record initial reset count @@ -567,7 +567,7 @@ def test_dpu_power_cycle_recovery(results, dpu_name): ) finally: if original_state: - set_feature_state(DPU_AUTO_RECOVERY_FEATURE, original_state) + set_auto_recovery_state(original_state) # Ensure DPU is admin-up set_module_admin_status(dpu_name, 'up') @@ -601,6 +601,192 @@ def test_chassisd_stop_marks_dpus_not_ready(results, dpu_name): logger.info("chassisd restarted after deinit test") +# --------------------------------------------------------------------------- +# Race condition tests +# --------------------------------------------------------------------------- + +def get_transition_in_progress(dpu_name): + """Read transition_in_progress from STATE_DB CHASSIS_MODULE_TABLE.""" + result = run_cmd( + f"sonic-db-cli STATE_DB HGET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_in_progress'", + check=False + ) + return result if result else None + + +def get_transition_type(dpu_name): + """Read transition_type from STATE_DB CHASSIS_MODULE_TABLE.""" + result = run_cmd( + f"sonic-db-cli STATE_DB HGET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_type'", + check=False + ) + return result if result else None + + +def set_transition_in_progress(dpu_name, transition_type): + """Simulate a planned operation by setting transition_in_progress in STATE_DB.""" + sonic_db_cli("STATE_DB", + f"HSET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_in_progress' 'True'") + sonic_db_cli("STATE_DB", + f"HSET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_type' '{transition_type}'") + sonic_db_cli("STATE_DB", + f"HSET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_start_time' '{int(time.time())}'") + logger.info(f"Set transition_in_progress=True, type={transition_type} for {dpu_name}") + + +def clear_transition_in_progress(dpu_name): + """Clear transition_in_progress fields from STATE_DB.""" + sonic_db_cli("STATE_DB", + f"HDEL 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_in_progress'") + sonic_db_cli("STATE_DB", + f"HDEL 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_type'") + sonic_db_cli("STATE_DB", + f"HDEL 'CHASSIS_MODULE_TABLE|{dpu_name}' 'transition_start_time'") + logger.info(f"Cleared transition_in_progress for {dpu_name}") + + +def test_transition_lock_suppresses_recovery(results, dpu_name): + """Setting transition_in_progress=True should suppress power-cycle recovery. + + Race condition coverage: chassisd must not initiate power-cycle when + another operation (gnoi shutdown/reboot) holds the transition lock. + """ + logger.info(f"Test: transition lock suppresses recovery on {dpu_name}") + + original_state = get_auto_recovery_state() + set_auto_recovery_state('enabled') + + try: + # Wait for DPU to be ready first + wait_for_dpu_ready(dpu_name, timeout=120) + initial_reset_count = get_dpu_state_field(dpu_name, RESET_COUNT) + + # Simulate a planned shutdown (gnoi sets the lock) + set_transition_in_progress(dpu_name, "shutdown") + time.sleep(POLL_INTERVAL * 2) + + # Now admin-down the DPU — this would normally trigger recovery + set_module_admin_status(dpu_name, 'down') + time.sleep(POLL_INTERVAL * 3) + + # Recovery should be suppressed — reset_count should not increase + reset_count = get_dpu_state_field(dpu_name, RESET_COUNT) + results.record( + f"lock_suppresses_recovery_{dpu_name}", + reset_count == initial_reset_count, + f"Reset count changed from {initial_reset_count} to {reset_count} " + f"despite transition lock being held" + ) + finally: + clear_transition_in_progress(dpu_name) + set_module_admin_status(dpu_name, 'up') + if original_state: + set_auto_recovery_state(original_state) + wait_for_dpu_ready(dpu_name, timeout=DPU_BOOT_TIMEOUT) + + +def test_recovery_type_does_not_suppress(results, dpu_name): + """transition_type='recovery' should NOT suppress chassisd recovery. + + Race condition coverage: chassisd sets its own lock with type='recovery' + during power-cycle. On the next poll, this must not self-suppress. + """ + logger.info(f"Test: recovery type does not suppress on {dpu_name}") + + try: + wait_for_dpu_ready(dpu_name, timeout=120) + + # Set transition_in_progress with type='recovery' (simulating chassisd's own lock) + set_transition_in_progress(dpu_name, "recovery") + time.sleep(POLL_INTERVAL * 2) + + # DPU should still be tracked as ready (not suppressed) + ready = get_dpu_state_field(dpu_name, READY_STATUS) + results.record( + f"recovery_type_no_suppress_{dpu_name}", + ready == 'true', + f"Expected ready_status=true (not suppressed), got '{ready}'" + ) + finally: + clear_transition_in_progress(dpu_name) + + +def test_lock_cleared_after_power_cycle(results, dpu_name): + """After chassisd power-cycles a DPU, the transition lock must be cleared. + + Race condition coverage: stale locks would permanently suppress + future planned operations (gnoi shutdown/reboot). + """ + logger.info(f"Test: lock cleared after power-cycle on {dpu_name}") + + original_state = get_auto_recovery_state() + set_auto_recovery_state('enabled') + + try: + wait_for_dpu_ready(dpu_name, timeout=120) + + # Force a power-cycle by admin-down/up (which triggers boot → ready) + set_module_admin_status(dpu_name, 'down') + time.sleep(30) + set_module_admin_status(dpu_name, 'up') + + # Wait for DPU to recover + wait_for_dpu_ready(dpu_name, timeout=DPU_BOOT_TIMEOUT) + time.sleep(POLL_INTERVAL) + + # After recovery, transition_in_progress should NOT be set + flag = get_transition_in_progress(dpu_name) + results.record( + f"lock_cleared_after_cycle_{dpu_name}", + flag is None or flag != 'True', + f"transition_in_progress still set to '{flag}' after recovery" + ) + finally: + clear_transition_in_progress(dpu_name) + set_module_admin_status(dpu_name, 'up') + if original_state: + set_auto_recovery_state(original_state) + + +def test_concurrent_shutdown_during_recovery(results, dpu_name): + """Verify chassisd aborts power-cycle if a planned operation starts concurrently. + + Race condition coverage: TOCTOU between _is_planned_transition_in_progress() + check and the actual power-cycle. Chassisd should detect the lock conflict + and back off. + """ + logger.info(f"Test: concurrent shutdown during recovery on {dpu_name}") + + original_state = get_auto_recovery_state() + set_auto_recovery_state('enabled') + + try: + wait_for_dpu_ready(dpu_name, timeout=120) + initial_reset_count = int(get_dpu_state_field(dpu_name, RESET_COUNT) or '0') + + # Admin-down then immediately set transition lock (simulating gnoi + # starting a shutdown just as chassisd detects the failure) + set_module_admin_status(dpu_name, 'down') + time.sleep(5) # Short wait, then set lock before chassisd can react + set_transition_in_progress(dpu_name, "shutdown") + time.sleep(POLL_INTERVAL * 3) + + # Chassisd should have been suppressed by the lock + reset_count = int(get_dpu_state_field(dpu_name, RESET_COUNT) or '0') + results.record( + f"concurrent_shutdown_aborts_recovery_{dpu_name}", + reset_count == initial_reset_count, + f"Reset count changed ({initial_reset_count} → {reset_count}); " + f"chassisd did not back off from concurrent shutdown" + ) + finally: + clear_transition_in_progress(dpu_name) + set_module_admin_status(dpu_name, 'up') + if original_state: + set_auto_recovery_state(original_state) + wait_for_dpu_ready(dpu_name, timeout=DPU_BOOT_TIMEOUT) + + # --------------------------------------------------------------------------- # Main # --------------------------------------------------------------------------- @@ -696,6 +882,15 @@ def main(): test_chassisd_restart_reinitializes_state(results, dpu) test_dpu_power_cycle_recovery(results, dpu) test_chassisd_stop_marks_dpus_not_ready(results, dpu) + + # --- Race condition tests --- + logger.info("\n=== RACE CONDITION TESTS ===\n") + for dpu in target_dpus: + logger.info(f"\n--- Race condition tests for {dpu} ---") + test_transition_lock_suppresses_recovery(results, dpu) + test_recovery_type_does_not_suppress(results, dpu) + test_lock_cleared_after_power_cycle(results, dpu) + test_concurrent_shutdown_during_recovery(results, dpu) else: logger.info("\n(Skipping destructive tests as requested)\n") From 5110cbad08234ae55bfc27f3339f26d0dadf7664 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Wed, 3 Jun 2026 16:35:05 +0000 Subject: [PATCH 02/10] [chassisd]: Use 'enable'/'disable' for dpu_auto_recovery ConfigDB values Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index ac2c914a0..849217e19 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -128,8 +128,8 @@ RECOVERY_UNRECOVERABLE = 'unrecoverable' # DPU auto-recovery field in CONFIG_DB DEVICE_METADATA|localhost DPU_AUTO_RECOVERY_FIELD = 'dpu_auto_recovery' -DPU_AUTO_RECOVERY_ENABLED = 'enabled' -DPU_AUTO_RECOVERY_DISABLED = 'disabled' +DPU_AUTO_RECOVERY_ENABLED = 'enable' +DPU_AUTO_RECOVERY_DISABLED = 'disable' # Platform.json field names for recovery thresholds PLATFORM_CFG_DPU_REBOOT_TIMEOUT = 'dpu_reboot_timeout' From e856803900d42fa91ff21d3dbd463d2b28ca441d Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Wed, 3 Jun 2026 17:32:44 +0000 Subject: [PATCH 03/10] Fix unittests Signed-off-by: Vasundhara Volam --- sonic-chassisd/tests/mock_platform.py | 8 ++ sonic-chassisd/tests/mock_swsscommon.py | 14 +++ .../tests/test_dpu_auto_recovery.py | 102 +++++++++--------- 3 files changed, 72 insertions(+), 52 deletions(-) diff --git a/sonic-chassisd/tests/mock_platform.py b/sonic-chassisd/tests/mock_platform.py index f8cf00765..a4e0a82e4 100644 --- a/sonic-chassisd/tests/mock_platform.py +++ b/sonic-chassisd/tests/mock_platform.py @@ -98,6 +98,14 @@ def clear_module_gnoi_halt_in_progress(self): """Mock implementation of clear_module_gnoi_halt_in_progress""" return True + def pci_detach(self): + """Mock implementation of pci_detach""" + return True + + def pci_reattach(self): + """Mock implementation of pci_reattach""" + return True + def is_midplane_reachable(self): return self.midplane_access diff --git a/sonic-chassisd/tests/mock_swsscommon.py b/sonic-chassisd/tests/mock_swsscommon.py index 8b76a7ef0..dde4a8a82 100644 --- a/sonic-chassisd/tests/mock_swsscommon.py +++ b/sonic-chassisd/tests/mock_swsscommon.py @@ -100,3 +100,17 @@ def connect(*args, **kwargs): def get_table(*args, **kwargs): pass + +class SonicV2Connector: + + def __init__(self, *args, **kwargs): + pass + + def connect(self, *args, **kwargs): + pass + + def get(self, *args, **kwargs): + pass + + def keys(self, *args, **kwargs): + return [] diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index 1bbf2133b..21c7537ea 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -228,7 +228,7 @@ def test_enabled_returns_true(self): updater = create_updater(chassis) updater.device_metadata_table = MagicMock() - updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] assert updater._is_auto_recovery_enabled() is True def test_disabled_returns_false(self): @@ -236,7 +236,7 @@ def test_disabled_returns_false(self): updater = create_updater(chassis) updater.device_metadata_table = MagicMock() - updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disabled'),)] + updater.device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disable'),)] assert updater._is_auto_recovery_enabled() is False def test_missing_field_defaults_to_disabled(self): @@ -937,7 +937,7 @@ def test_reaches_reset_limit_becomes_unrecoverable(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis, platform_json={"dpu_reset_limit": 3}) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 # one below limit + updater.dpu_recovery_state["DPU0"]['reset_count'] = 3 # at limit import time as time_module updater.dpu_recovery_state["DPU0"]['self_recovery_start_time'] = time_module.time() - 400 @@ -951,7 +951,6 @@ def test_reaches_reset_limit_becomes_unrecoverable(self): assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_UNRECOVERABLE assert get_dpu_state_field(updater, "DPU0", RECOVERY_STATUS) == RECOVERY_UNRECOVERABLE - assert get_dpu_state_field(updater, "DPU0", RESET_COUNT) == '3' def test_unrecoverable_is_terminal(self): """Once unrecoverable, state doesn't change even if DPU comes back.""" @@ -1050,7 +1049,7 @@ def test_multiple_failures_increment_reset_count(self): patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() - # 5th failure: Ready → WaitForSelfRecovery → timeout → Unrecoverable + # 5th failure: Ready → WaitForSelfRecovery → timeout → PowerCycle (count=5) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_READY set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ @@ -1062,6 +1061,16 @@ def test_multiple_failures_increment_reset_count(self): patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() + assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 5 + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE + + # 6th attempt: PowerCycle timeout → Unrecoverable (count=5 >= limit=5) + import time as time_module + updater.dpu_recovery_state["DPU0"]['boot_start_time'] = time_module.time() - 700 + with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ + patch.object(updater, 'get_module_admin_status', return_value='up'): + updater.update_dpu_recovery_state() + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_UNRECOVERABLE assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 5 @@ -1217,7 +1226,7 @@ def test_power_cycle_calls_admin_state(self): def test_power_cycle_increments_reset_count(self): """Each _enter_power_cycle_or_unrecoverable call increments reset_count.""" chassis = create_chassis_with_dpus(1) - updater = create_updater(chassis) + updater = create_updater(chassis, platform_json={"dpu_reset_limit": 5}) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 @@ -1231,7 +1240,7 @@ def test_power_cycle_at_limit_marks_unrecoverable(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis, platform_json={"dpu_reset_limit": 3}) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 # limit is 3 + updater.dpu_recovery_state["DPU0"]['reset_count'] = 3 # at limit updater._enter_power_cycle_or_unrecoverable("DPU0", 0) @@ -1367,7 +1376,7 @@ def test_one_dpu_unrecoverable_others_continue(self): # DPU0: at limit, in WaitForSelfRecovery with expired timeout → will become unrecoverable updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - updater.dpu_recovery_state["DPU0"]['reset_count'] = 1 + updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 updater.dpu_recovery_state["DPU0"]['self_recovery_start_time'] = time.time() - 400 set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) @@ -1777,11 +1786,11 @@ def test_power_cycle_set_admin_state_exception(self): assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE def test_power_cycle_exception_at_limit_still_marks_unrecoverable(self): - """Exception during power-cycle at reset_limit still marks unrecoverable.""" + """At reset_limit, DPU is marked unrecoverable without attempting power-cycle.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis, platform_json={"dpu_reset_limit": 2}) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - updater.dpu_recovery_state["DPU0"]['reset_count'] = 1 + updater.dpu_recovery_state["DPU0"]['reset_count'] = 2 module = chassis.module_list[0] module.set_admin_state = MagicMock(side_effect=RuntimeError("HW error")) @@ -2330,7 +2339,7 @@ def test_all_dpus_fail_simultaneously(self): def test_reset_count_independent_per_dpu(self): """Each DPU's reset_count increments independently.""" chassis = create_chassis_with_dpus(2) - updater = create_updater(chassis) + updater = create_updater(chassis, platform_json={"dpu_reset_limit": 10}) # DPU0: WaitForSelfRecovery with expired timeout, reset_count=3 updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY @@ -2434,8 +2443,8 @@ def test_is_planned_transition_exception_returns_false(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - # Make get_module_state_transition raise — try_get returns default=False - chassis.module_list[0].get_module_state_transition = MagicMock(side_effect=Exception("DB error")) + # Make get_module_state_transition raise NotImplementedError — try_get returns default=False + chassis.module_list[0].get_module_state_transition = MagicMock(side_effect=NotImplementedError("not supported")) assert updater._is_planned_transition_in_progress("DPU0", 0) is False def test_is_planned_transition_true(self): @@ -2511,18 +2520,15 @@ def test_auto_recovery_feature_not_found_in_table(self): patch("chassisd.swsscommon.Table", return_value=mock_table): assert updater._is_auto_recovery_enabled() is False - def test_auto_recovery_missing_state_field_defaults_enabled(self): - """_is_auto_recovery_enabled with no 'state' field defaults to enabled.""" + def test_auto_recovery_missing_state_field_defaults_disabled(self): + """_is_auto_recovery_enabled with no 'dpu_auto_recovery' field defaults to disabled.""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) - mock_table = MagicMock() - # Feature exists but has no 'state' field - mock_table.get.return_value = [True, (('description', 'DPU Auto Recovery'),)] - - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_table): - assert updater._is_auto_recovery_enabled() is True + updater.device_metadata_table = MagicMock() + # Feature exists but has no 'dpu_auto_recovery' field + updater.device_metadata_table.get.return_value = [True, (('description', 'DPU Auto Recovery'),)] + assert updater._is_auto_recovery_enabled() is False class TestGetModuleAdminStatus: @@ -2572,14 +2578,12 @@ def test_booting_to_ready_full_path(self): set_dpu_states(updater, "DPU0", mp='up', cp='up', dp='up') updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) - # Mock only the DB access for _is_auto_recovery_enabled + # Set device_metadata_table directly for _is_auto_recovery_enabled mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table - # Mock get_module_admin_status since it creates a new DB connection - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ - patch.object(updater, 'get_module_admin_status', return_value='up'): + with patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_READY @@ -2595,11 +2599,10 @@ def test_ready_cp_down_enters_wait_for_self_recovery(self): updater.module_table.hset("DPU0", "transition_in_progress", "False") mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ - patch.object(updater, 'get_module_admin_status', return_value='up'): + with patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY @@ -2614,11 +2617,10 @@ def test_admin_down_to_offline_full_path(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ - patch.object(updater, 'get_module_admin_status', return_value='down'): + with patch.object(updater, 'get_module_admin_status', return_value='down'): updater.update_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_ADMIN_DOWN @@ -2635,14 +2637,13 @@ def test_hardware_offline_triggers_power_cycle_full_path(self): updater.module_table.hset("DPU0", "transition_in_progress", "False") mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table module = chassis.module_list[0] module.set_admin_state = MagicMock() - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ - patch.object(updater, 'get_module_admin_status', return_value='up'): + with patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() # Should have power-cycled (set_admin_state called) @@ -2664,14 +2665,13 @@ def test_wait_for_self_recovery_timeout_power_cycle_full_path(self): updater.module_table.hset("DPU0", "transition_in_progress", "False") mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table module = chassis.module_list[0] module.set_admin_state = MagicMock() - with patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table), \ - patch.object(updater, 'get_module_admin_status', return_value='up'): + with patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() # Should have power-cycled @@ -2687,7 +2687,8 @@ def test_init_recovery_state_npu_crash_full_path(self): updater.module_table.hset("DPU1", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'enable'),)] + updater.device_metadata_table = mock_device_metadata_table module = chassis.module_list[0] module.set_admin_state = MagicMock() @@ -2695,9 +2696,7 @@ def test_init_recovery_state_npu_crash_full_path(self): module1.set_admin_state = MagicMock() with patch("os.path.isfile", return_value=True), \ - patch("builtins.open", mock_open(read_data="kernel panic - not syncing")), \ - patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table): + patch("builtins.open", mock_open(read_data="kernel panic - not syncing")): updater.init_dpu_recovery_state() # DPU0 should be power-cycled (online + NPU crash + auto-recovery enabled) @@ -2713,12 +2712,11 @@ def test_init_recovery_state_npu_crash_disabled_full_path(self): updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) mock_device_metadata_table = MagicMock() - mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disabled'),)] + mock_device_metadata_table.get.return_value = [True, (('dpu_auto_recovery', 'disable'),)] + updater.device_metadata_table = mock_device_metadata_table with patch("os.path.isfile", return_value=True), \ - patch("builtins.open", mock_open(read_data="Unknown")), \ - patch("chassisd.daemon_base.db_connect") as mock_db, \ - patch("chassisd.swsscommon.Table", return_value=mock_device_metadata_table): + patch("builtins.open", mock_open(read_data="Unknown")): updater.init_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_MANUAL_INTERVENTION From 971d78dc94905f703ce2b676ce2357453746c4c7 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Fri, 5 Jun 2026 17:30:25 +0000 Subject: [PATCH 04/10] [chassisd]: Align DPU recovery startup/admin flow and tests Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 17 +++++--- sonic-chassisd/tests/mock_platform.py | 4 +- .../tests/test_dpu_auto_recovery.py | 4 +- .../tests/testbed/test_dpu_auto_recovery.py | 43 ++++++++++++------- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 849217e19..6f71cc842 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1325,10 +1325,17 @@ class SmartSwitchModuleUpdater(ModuleUpdater): return try: - try_get(module.pci_detach, default=False) + # Keep the same graceful sequence as module_base: + # pre-shutdown -> admin down -> admin up -> post-startup. + # Treat NotImplemented pre/post hooks as no-op success. + if not try_get(module.module_pre_shutdown, default=True): + self.log_warning("{}: module_pre_shutdown() failed".format(module_name)) + try_get(module.set_admin_state, MODULE_ADMIN_DOWN, default=False) try_get(module.set_admin_state, MODULE_ADMIN_UP, default=False) - try_get(module.pci_reattach, default=False) + + if not try_get(module.module_post_startup, default=True): + self.log_warning("{}: module_post_startup() failed".format(module_name)) except Exception as e: self.log_error("{}: Power-cycle failed: {}".format(module_name, e)) finally: @@ -1406,9 +1413,9 @@ class SmartSwitchModuleUpdater(ModuleUpdater): # NPU kernel crash recovery: unconditionally power-cycle # admin-up DPUs to guarantee a known-good starting state. - oper_status = self.get_module_current_status(name) - if oper_status == str(ModuleBase.MODULE_STATUS_OFFLINE): - # Admin-down DPU; leave powered off + admin_status = self.get_module_admin_status(name) + if admin_status == 'down': + # Admin-down DPU; leave powered off. continue if not self._is_auto_recovery_enabled(): self.dpu_recovery_state[name]['state'] = DPU_STATE_MANUAL_INTERVENTION diff --git a/sonic-chassisd/tests/mock_platform.py b/sonic-chassisd/tests/mock_platform.py index a4e0a82e4..fb157f21d 100644 --- a/sonic-chassisd/tests/mock_platform.py +++ b/sonic-chassisd/tests/mock_platform.py @@ -73,10 +73,10 @@ def set_midplane_ip(self): self.midplane_ip = '192.168.1.{}'.format(self.get_slot()) def module_pre_shutdown(self): - pass + return self.pci_detach() def module_post_startup(self): - pass + return self.pci_reattach() def set_admin_state_gracefully(self, up): """Mock implementation of set_admin_state_gracefully""" diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index 21c7537ea..21e1e2dba 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -156,6 +156,7 @@ def test_init_npu_crash_power_cycles_admin_up_dpus(self): updater.module_table.hset("DPU1", "oper_status", str(ModuleBase.MODULE_STATUS_OFFLINE)) with patch.object(updater, '_npu_crash_on_last_boot', return_value=True), \ + patch.object(updater, 'get_module_admin_status', side_effect=lambda name: 'up' if name == 'DPU0' else 'down'), \ patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ patch.object(updater, '_enter_power_cycle_or_unrecoverable') as mock_pc: updater.init_dpu_recovery_state() @@ -2696,7 +2697,8 @@ def test_init_recovery_state_npu_crash_full_path(self): module1.set_admin_state = MagicMock() with patch("os.path.isfile", return_value=True), \ - patch("builtins.open", mock_open(read_data="kernel panic - not syncing")): + patch("builtins.open", mock_open(read_data="kernel panic - not syncing")), \ + patch.object(updater, 'get_module_admin_status', side_effect=lambda name: 'up' if name == 'DPU0' else 'down'): updater.init_dpu_recovery_state() # DPU0 should be power-cycled (online + NPU crash + auto-recovery enabled) diff --git a/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py b/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py index 71b6bbe6c..df0f67643 100644 --- a/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/testbed/test_dpu_auto_recovery.py @@ -134,6 +134,12 @@ def get_module_oper_status(dpu_name): return result if result else None +def set_module_oper_status(dpu_name, status): + """Set oper_status in STATE_DB CHASSIS_MODULE_TABLE (test-only fault injection).""" + sonic_db_cli("STATE_DB", f"HSET 'CHASSIS_MODULE_TABLE|{dpu_name}' 'oper_status' '{status}'") + logger.info(f"Set {dpu_name} oper_status to '{status}'") + + def get_auto_recovery_state(): """Get dpu_auto_recovery from CONFIG_DB DEVICE_METADATA|localhost.""" result = run_cmd( @@ -361,11 +367,11 @@ def test_reset_count_field(results, dpu_name): def test_auto_recovery_feature_flag(results): - """Verify dpu_auto_recovery exists in CONFIG_DB DEVICE_METADATA (or defaults to enabled).""" + """Verify dpu_auto_recovery exists in CONFIG_DB DEVICE_METADATA (or defaults to disable).""" logger.info("Test: dpu_auto_recovery in DEVICE_METADATA") state = get_auto_recovery_state() - valid_states = ('enabled', 'disabled') - # Field may not be explicitly configured (defaults to enabled in chassisd) + valid_states = ('enable', 'disable') + # Field may not be explicitly configured (defaults to disable in chassisd) results.record( "auto_recovery_feature_exists", state is None or state in valid_states, @@ -430,7 +436,7 @@ def test_disable_auto_recovery_no_power_cycle(results, dpu_name): try: # Disable auto-recovery - set_auto_recovery_state('disabled') + set_auto_recovery_state('disable') time.sleep(POLL_INTERVAL) # Record current reset count @@ -494,15 +500,15 @@ def test_enable_auto_recovery_feature(results, dpu_name): original_state = get_auto_recovery_state() try: - set_auto_recovery_state('enabled') + set_auto_recovery_state('enable') time.sleep(POLL_INTERVAL) # Verify the field is reflected (chassisd should read it next poll) state = get_auto_recovery_state() results.record( f"auto_recovery_enabled_{dpu_name}", - state == 'enabled', - f"Expected state=enabled, got '{state}'" + state == 'enable', + f"Expected state=enable, got '{state}'" ) # If DPU is healthy, it should be ready @@ -528,7 +534,7 @@ def test_dpu_power_cycle_recovery(results, dpu_name): # Ensure auto-recovery is enabled original_state = get_auto_recovery_state() - set_auto_recovery_state('enabled') + set_auto_recovery_state('enable') try: # Record initial reset count @@ -654,7 +660,8 @@ def test_transition_lock_suppresses_recovery(results, dpu_name): logger.info(f"Test: transition lock suppresses recovery on {dpu_name}") original_state = get_auto_recovery_state() - set_auto_recovery_state('enabled') + set_auto_recovery_state('enable') + original_oper_status = get_module_oper_status(dpu_name) try: # Wait for DPU to be ready first @@ -665,8 +672,8 @@ def test_transition_lock_suppresses_recovery(results, dpu_name): set_transition_in_progress(dpu_name, "shutdown") time.sleep(POLL_INTERVAL * 2) - # Now admin-down the DPU — this would normally trigger recovery - set_module_admin_status(dpu_name, 'down') + # Inject hardware-offline while admin stays up. + set_module_oper_status(dpu_name, 'Offline') time.sleep(POLL_INTERVAL * 3) # Recovery should be suppressed — reset_count should not increase @@ -679,6 +686,8 @@ def test_transition_lock_suppresses_recovery(results, dpu_name): ) finally: clear_transition_in_progress(dpu_name) + if original_oper_status is not None: + set_module_oper_status(dpu_name, original_oper_status) set_module_admin_status(dpu_name, 'up') if original_state: set_auto_recovery_state(original_state) @@ -720,7 +729,7 @@ def test_lock_cleared_after_power_cycle(results, dpu_name): logger.info(f"Test: lock cleared after power-cycle on {dpu_name}") original_state = get_auto_recovery_state() - set_auto_recovery_state('enabled') + set_auto_recovery_state('enable') try: wait_for_dpu_ready(dpu_name, timeout=120) @@ -758,15 +767,15 @@ def test_concurrent_shutdown_during_recovery(results, dpu_name): logger.info(f"Test: concurrent shutdown during recovery on {dpu_name}") original_state = get_auto_recovery_state() - set_auto_recovery_state('enabled') + set_auto_recovery_state('enable') + original_oper_status = get_module_oper_status(dpu_name) try: wait_for_dpu_ready(dpu_name, timeout=120) initial_reset_count = int(get_dpu_state_field(dpu_name, RESET_COUNT) or '0') - # Admin-down then immediately set transition lock (simulating gnoi - # starting a shutdown just as chassisd detects the failure) - set_module_admin_status(dpu_name, 'down') + # Inject a hardware-offline failure while keeping admin up. + set_module_oper_status(dpu_name, 'Offline') time.sleep(5) # Short wait, then set lock before chassisd can react set_transition_in_progress(dpu_name, "shutdown") time.sleep(POLL_INTERVAL * 3) @@ -781,6 +790,8 @@ def test_concurrent_shutdown_during_recovery(results, dpu_name): ) finally: clear_transition_in_progress(dpu_name) + if original_oper_status is not None: + set_module_oper_status(dpu_name, original_oper_status) set_module_admin_status(dpu_name, 'up') if original_state: set_auto_recovery_state(original_state) From c9f3a8f086342b1d3a14ab56aa0077cb265f7a0a Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Fri, 12 Jun 2026 00:50:46 +0000 Subject: [PATCH 05/10] [chassisd]: Address PR review nits and reboot-cause handling Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 6f71cc842..85adf968f 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -146,7 +146,7 @@ MINIMUM_SELF_RECOVERY_POLL_COUNT = 3 # Reboot cause file used to detect NPU kernel crash on chassisd startup REBOOT_CAUSE_FILE = "/host/reboot-cause/reboot-cause.txt" -NPU_CRASH_REBOOT_CAUSE_KEYWORDS = ("kernel panic", "unknown") +NPU_CRASH_REBOOT_CAUSE_KEYWORDS = ("kernel panic") # Internal recovery dict key for tracking prior Unrecoverable state WAS_UNRECOVERABLE_KEY = 'was_unrecoverable' @@ -1208,15 +1208,15 @@ class SmartSwitchModuleUpdater(ModuleUpdater): keys = self.chassis_state_db.keys(pattern) if keys: for key in keys: - if DPU_STATE_TABLE not in key and not "REBOOT_CAUSE" in key: + if DPU_STATE_TABLE not in key and "REBOOT_CAUSE" not in key: self.chassis_state_db.delete(key) return def _is_auto_recovery_enabled(self): """Check if dpu_auto_recovery is enabled in CONFIG_DB DEVICE_METADATA|localhost. - Returns True when the field is 'enabled'. - Returns False when the field is 'disabled' (or absent, defaulting to disabled). + Returns True when the field is 'enable'. + Returns False otherwise. """ try: fvs = self.device_metadata_table.get('localhost') @@ -1225,7 +1225,8 @@ class SmartSwitchModuleUpdater(ModuleUpdater): state = fvs.get(DPU_AUTO_RECOVERY_FIELD, DPU_AUTO_RECOVERY_DISABLED) return state == DPU_AUTO_RECOVERY_ENABLED except Exception as e: - self.log_error("Failed to read dpu_auto_recovery from DEVICE_METADATA: {}".format(e)) + self.log_error("Failed to read {} from DEVICE_METADATA: {}".format( + DPU_AUTO_RECOVERY_FIELD, e)) return False # Default to disabled def _get_dpu_states(self, module_name): @@ -1279,11 +1280,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): return True def _enter_power_cycle_or_unrecoverable(self, module_name, module_index): - """Attempt to power-cycle a DPU, or mark it unrecoverable if reset_limit reached. - - Checks reset_limit BEFORE issuing the power-cycle to avoid a - wasted reset that would immediately be marked unrecoverable. - """ + """Attempt power-cycle; mark unrecoverable if reset_limit is reached.""" recovery = self.dpu_recovery_state[module_name] # Check limit before issuing power-cycle From cbce8b035a938152affc991711711a60f4eea9c8 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Fri, 12 Jun 2026 01:01:53 +0000 Subject: [PATCH 06/10] [chassisd]: Preserve initial down timestamp on retries Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 1 - .../tests/test_dpu_auto_recovery.py | 39 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 85adf968f..8ea72c4d4 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1376,7 +1376,6 @@ class SmartSwitchModuleUpdater(ModuleUpdater): self.log_warning("{}: Boot timeout ({}s) expired {}".format( name, self.dpu_boot_timeout, context)) self._set_ready_status(name, 'false') - self._set_last_down_time(name) if auto_recovery_enabled: self._enter_power_cycle_or_unrecoverable(name, module_index) else: diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index 21e1e2dba..db1e84c28 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -429,6 +429,25 @@ def test_booting_timeout_triggers_power_cycle(self): mock_pc.assert_called_once_with("DPU0", 0) + def test_booting_timeout_does_not_refresh_last_down_time(self): + """Boot timeout retry should not overwrite initial outage timestamp.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_BOOTING + import time as time_module + updater.dpu_recovery_state["DPU0"]['boot_start_time'] = time_module.time() - 700 + + set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') + chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) + updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) + + with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ + patch.object(updater, 'get_module_admin_status', return_value='up'), \ + patch.object(updater, '_set_last_down_time') as mock_set_down: + updater.update_dpu_recovery_state() + + mock_set_down.assert_not_called() + def test_booting_timeout_manual_intervention_when_disabled(self): """Booting timeout + auto-recovery disabled → ManualIntervention.""" chassis = create_chassis_with_dpus(1) @@ -486,6 +505,26 @@ def test_power_cycle_timeout_triggers_another_cycle(self): mock_pc.assert_called_once_with("DPU0", 0) + def test_power_cycle_timeout_does_not_refresh_last_down_time(self): + """PowerCycle timeout retry should not overwrite initial outage timestamp.""" + chassis = create_chassis_with_dpus(1) + updater = create_updater(chassis) + updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_POWER_CYCLE + updater.dpu_recovery_state["DPU0"]['reset_count'] = 1 + import time as time_module + updater.dpu_recovery_state["DPU0"]['boot_start_time'] = time_module.time() - 700 + + set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') + chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_ONLINE) + updater.module_table.hset("DPU0", "oper_status", str(ModuleBase.MODULE_STATUS_ONLINE)) + + with patch.object(updater, '_is_auto_recovery_enabled', return_value=True), \ + patch.object(updater, 'get_module_admin_status', return_value='up'), \ + patch.object(updater, '_set_last_down_time') as mock_set_down: + updater.update_dpu_recovery_state() + + mock_set_down.assert_not_called() + def test_power_cycle_timeout_manual_intervention_when_disabled(self): """PowerCycle state exceeds dpu_boot_timeout + auto-recovery disabled → ManualIntervention.""" chassis = create_chassis_with_dpus(1) From 6c30b296c63b3e3ecd8f3067cca02ddaacca141d Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Mon, 15 Jun 2026 21:50:11 +0000 Subject: [PATCH 07/10] [chassisd]: Address PR feedback on recovery FSM handling Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 13 ++++++++++--- sonic-chassisd/tests/test_dpu_auto_recovery.py | 13 ++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 8ea72c4d4..460c06972 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -151,6 +151,9 @@ NPU_CRASH_REBOOT_CAUSE_KEYWORDS = ("kernel panic") # Internal recovery dict key for tracking prior Unrecoverable state WAS_UNRECOVERABLE_KEY = 'was_unrecoverable' +# Transition type value used by chassisd when acquiring the state-transition lock +TRANSITION_TYPE_RECOVERY = 'recovery' + # DPU recovery states DPU_STATE_BOOTING = 'Booting' DPU_STATE_READY = 'Ready' @@ -1263,7 +1266,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): Uses the platform API get_module_state_transition() which handles timeout-based auto-clearing. Returns True when a non-recovery transition is active, suppressing auto-recovery. - Ignores transition_type == 'recovery' so chassisd does not suppress itself. + Ignores transition_type == TRANSITION_TYPE_RECOVERY so chassisd does not suppress itself. """ module = self.chassis.get_module(module_index) in_progress = try_get(module.get_module_state_transition, module_name, default=False) @@ -1274,7 +1277,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): fvs = self.module_table.get(module_name) if isinstance(fvs, list) and fvs[0] is True: fvs = dict(fvs[-1]) - return fvs.get('transition_type') != 'recovery' + return fvs.get('transition_type') != TRANSITION_TYPE_RECOVERY except Exception: pass return True @@ -1302,12 +1305,14 @@ class SmartSwitchModuleUpdater(ModuleUpdater): # shutdown/reboot from operating on this DPU during power-cycle. # If the lock cannot be acquired (another operation holds it), # abort the power-cycle — the other operation owns this DPU. - lock_acquired = try_get(module.set_module_state_transition, module_name, "recovery", default=False) + lock_acquired = try_get(module.set_module_state_transition, module_name, TRANSITION_TYPE_RECOVERY, default=False) if not lock_acquired: self.log_warning("{}: Cannot acquire state transition lock; " "another operation in progress — skipping power-cycle".format(module_name)) recovery['reset_count'] -= 1 self._set_reset_count(module_name, recovery['reset_count']) + recovery['state'] = DPU_STATE_POWER_CYCLE + recovery['boot_start_time'] = time.time() return # Re-check transition_in_progress after acquiring our lock to close @@ -1319,6 +1324,8 @@ class SmartSwitchModuleUpdater(ModuleUpdater): try_get(module.clear_module_state_transition, module_name, default=False) recovery['reset_count'] -= 1 self._set_reset_count(module_name, recovery['reset_count']) + recovery['state'] = DPU_STATE_POWER_CYCLE + recovery['boot_start_time'] = time.time() return try: diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index db1e84c28..5000bcbe6 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -60,6 +60,7 @@ MAX_HISTORY_FILES, REBOOT_CAUSE_FILE, WAS_UNRECOVERABLE_KEY, + TRANSITION_TYPE_RECOVERY, ) @@ -1940,8 +1941,9 @@ def test_power_cycle_skipped_when_lock_not_acquired(self): module.clear_module_state_transition.assert_not_called() # reset_count should be rolled back to 0 assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 - # State should NOT have changed to PowerCycle - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY + # State should have entered POWER_CYCLE to wait for the in-progress + # operation (gnoi shutdown/reboot) to complete. + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE def test_power_cycle_aborted_when_legacy_field_set_after_lock(self): """If transition_in_progress appears after lock acquired, abort.""" @@ -1963,15 +1965,16 @@ def test_power_cycle_aborted_when_legacy_field_set_after_lock(self): updater._enter_power_cycle_or_unrecoverable("DPU0", 0) # Lock was acquired but then released due to legacy field detection - module.set_module_state_transition.assert_called_once_with("DPU0", "recovery") + module.set_module_state_transition.assert_called_once_with("DPU0", TRANSITION_TYPE_RECOVERY) module.clear_module_state_transition.assert_called_once_with("DPU0") # Power-cycle should NOT have been executed module.pci_detach.assert_not_called() module.set_admin_state.assert_not_called() # reset_count should be rolled back to 0 assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 - # State should NOT have changed to PowerCycle - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_WAIT_FOR_SELF_RECOVERY + # State should have entered POWER_CYCLE to wait for the in-progress + # operation (gnoi shutdown/reboot) to complete. + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE # ============================================================================ From 8714ebe5f1447a495ed82026ed59ece42bea60f5 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Wed, 24 Jun 2026 16:12:31 +0000 Subject: [PATCH 08/10] [chassisd]: Harden NPU reboot-cause crash detection Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 460c06972..54aba1d13 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1434,7 +1434,15 @@ class SmartSwitchModuleUpdater(ModuleUpdater): return False with open(REBOOT_CAUSE_FILE, 'r') as f: cause = f.read().strip().lower() - return any(kw in cause for kw in NPU_CRASH_REBOOT_CAUSE_KEYWORDS) + + # Match explicit crash signals only. This avoids classifying + # normal operator-initiated reboot strings as crashes. + if 'kernel panic' in cause: + return True + + # Keep supporting legacy "unknown" reboot-cause content, but only + # as the full value/prefix (not arbitrary substring matches). + return cause == 'unknown' or cause.startswith('unknown:') or cause.startswith('unknown ') except Exception as e: self.log_warning("Failed to read {}: {}".format(REBOOT_CAUSE_FILE, e)) return False From ab47050c44e3624f5c6bf4022c3554728982f018 Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Fri, 26 Jun 2026 19:33:33 +0000 Subject: [PATCH 09/10] [chassisd]: Refine DPU hw-offline gating and NPU crash detection Address PR #829 review feedback: - _should_handle_hw_offline(): gate on a Ready-only allow-list instead of an exclusion list. A stale Offline snapshot seen while still AdminDown now takes the clean restart path (-> Booting) rather than power-cycling, avoiding a `module startup` race. Fail-safe for future states. - _npu_crash_on_last_boot(): treat only "kernel panic" as a crash. "unknown" is reported on first boot and must not force a power-cycle; drop the unused NPU_CRASH_REBOOT_CAUSE_KEYWORDS constant. - Update unit tests to cover the new behavior. Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 26 +++++++------------ .../tests/test_dpu_auto_recovery.py | 25 +++++++++++------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 54aba1d13..bb3af8072 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -146,7 +146,6 @@ MINIMUM_SELF_RECOVERY_POLL_COUNT = 3 # Reboot cause file used to detect NPU kernel crash on chassisd startup REBOOT_CAUSE_FILE = "/host/reboot-cause/reboot-cause.txt" -NPU_CRASH_REBOOT_CAUSE_KEYWORDS = ("kernel panic") # Internal recovery dict key for tracking prior Unrecoverable state WAS_UNRECOVERABLE_KEY = 'was_unrecoverable' @@ -1368,14 +1367,14 @@ class SmartSwitchModuleUpdater(ModuleUpdater): recovery['self_recovery_poll_count'] = 0 def _should_handle_hw_offline(self, current_state): - """Return True if the DPU should react to oper_status Offline. + """Return True only if a previously-Ready DPU should treat + oper_status Offline as a new hardware failure. - States already handling failure (Booting, PowerCycle, - WaitForSelfRecovery, ManualIntervention) should not re-trigger. + Allow-list (Ready only) is fail-safe for future states and avoids a + `module startup` race where a stale Offline snapshot in AdminDown + would power-cycle instead of cleanly restarting. """ - return current_state not in ( - DPU_STATE_BOOTING, DPU_STATE_POWER_CYCLE, - DPU_STATE_WAIT_FOR_SELF_RECOVERY, DPU_STATE_MANUAL_INTERVENTION) + return current_state == DPU_STATE_READY def _handle_boot_timeout_expired(self, name, module_index, recovery, auto_recovery_enabled, context): @@ -1426,7 +1425,7 @@ class SmartSwitchModuleUpdater(ModuleUpdater): self._enter_power_cycle_or_unrecoverable(name, module_index) def _npu_crash_on_last_boot(self): - """Return True if the last NPU reboot cause looks like a crash/OOM/unknown.""" + """Return True if the last NPU reboot cause looks like a kernel crash.""" try: if not os.path.isfile(REBOOT_CAUSE_FILE): # No reboot-cause file means this is likely a fresh boot or the @@ -1435,14 +1434,9 @@ class SmartSwitchModuleUpdater(ModuleUpdater): with open(REBOOT_CAUSE_FILE, 'r') as f: cause = f.read().strip().lower() - # Match explicit crash signals only. This avoids classifying - # normal operator-initiated reboot strings as crashes. - if 'kernel panic' in cause: - return True - - # Keep supporting legacy "unknown" reboot-cause content, but only - # as the full value/prefix (not arbitrary substring matches). - return cause == 'unknown' or cause.startswith('unknown:') or cause.startswith('unknown ') + # Match kernel crashes only. "unknown" is reported on first boot + # and must not trigger a power-cycle. + return 'kernel panic' in cause except Exception as e: self.log_warning("Failed to read {}: {}".format(REBOOT_CAUSE_FILE, e)) return False diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index 5000bcbe6..a507dc992 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -194,13 +194,15 @@ def test_detects_kernel_panic(self): patch("builtins.open", mock_open(read_data="Kernel Panic - not syncing")): assert updater._npu_crash_on_last_boot() is True - def test_detects_unknown_cause(self): + def test_unknown_cause_not_crash(self): chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) + # An "unknown" reboot cause is reported on first boot and must NOT be + # classified as a crash, otherwise every fresh boot would power-cycle. with patch("os.path.isfile", return_value=True), \ patch("builtins.open", mock_open(read_data="Unknown")): - assert updater._npu_crash_on_last_boot() is True + assert updater._npu_crash_on_last_boot() is False def test_normal_reboot_not_crash(self): chassis = create_chassis_with_dpus(1) @@ -1695,12 +1697,16 @@ def test_hardware_offline_from_ready_triggers_power_cycle(self): assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE - def test_hardware_offline_from_offline_state_triggers_recovery(self): - """oper_status=Offline from Offline state + admin-up → hardware-offline - branch fires (Offline is NOT in the exclusion list), triggering power-cycle.""" + def test_hardware_offline_from_admin_down_restarts_booting(self): + """admin-up + current_state still AdminDown + stale oper_status=Offline + must take the clean AdminDown -> Booting restart path, NOT the + hardware-offline power-cycle path. Only a previously-Ready DPU treats + Offline as a new hardware failure, so this avoids the `module startup` + race that would otherwise power-cycle (and bump reset_count).""" chassis = create_chassis_with_dpus(1) updater = create_updater(chassis) updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_ADMIN_DOWN + updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 set_dpu_states(updater, "DPU0", mp='down', cp='down', dp='down') chassis.module_list[0].set_oper_status(ModuleBase.MODULE_STATUS_OFFLINE) @@ -1710,9 +1716,10 @@ def test_hardware_offline_from_offline_state_triggers_recovery(self): patch.object(updater, 'get_module_admin_status', return_value='up'): updater.update_dpu_recovery_state() - # Offline is not in the hardware-offline exclusion list, so the - # hardware-offline branch fires and triggers a power-cycle - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE + # AdminDown falls through to the clean restart path instead of being + # treated as a hardware failure; reset_count is left untouched. + assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_BOOTING + assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 # ============================================================================ @@ -2760,7 +2767,7 @@ def test_init_recovery_state_npu_crash_disabled_full_path(self): updater.device_metadata_table = mock_device_metadata_table with patch("os.path.isfile", return_value=True), \ - patch("builtins.open", mock_open(read_data="Unknown")): + patch("builtins.open", mock_open(read_data="kernel panic - not syncing")): updater.init_dpu_recovery_state() assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_MANUAL_INTERVENTION From e98f98d8efc9179bb49797132d7a9beca63668aa Mon Sep 17 00:00:00 2001 From: Vasundhara Volam Date: Mon, 13 Jul 2026 22:45:43 +0000 Subject: [PATCH 10/10] [chassisd]: Drop redundant post-lock transition re-check in power-cycle set_module_state_transition() is an atomic compare-and-acquire under a cross-process fcntl flock, and every writer of transition_in_progress goes through that same API. So the `if not lock_acquired` abort already closes the TOCTOU window against a concurrent planned shutdown/reboot: whoever grabs the lock first wins and the other's set returns False. Once recovery holds the lock, transition_type == 'recovery', so _is_planned_transition_in_progress() returns False by construction and the subsequent re-check is unreachable in real execution (only reachable by mocking). Remove the re-check block in _enter_power_cycle_or_unrecoverable() and the test_power_cycle_aborted_when_legacy_field_set_after_lock test that only exercised it via mocking. The earlier suppression gate and the lock-not-acquired abort are retained. Signed-off-by: Vasundhara Volam --- sonic-chassisd/scripts/chassisd | 13 -------- .../tests/test_dpu_auto_recovery.py | 31 ------------------- 2 files changed, 44 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index bb3af8072..deb409c38 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1314,19 +1314,6 @@ class SmartSwitchModuleUpdater(ModuleUpdater): recovery['boot_start_time'] = time.time() return - # Re-check transition_in_progress after acquiring our lock to close - # the TOCTOU window where gnoi/CLI sets transition_in_progress - # between _is_planned_transition_in_progress() and here. - if self._is_planned_transition_in_progress(module_name, module_index): - self.log_warning("{}: transition_in_progress detected " - "after lock acquisition — aborting power-cycle".format(module_name)) - try_get(module.clear_module_state_transition, module_name, default=False) - recovery['reset_count'] -= 1 - self._set_reset_count(module_name, recovery['reset_count']) - recovery['state'] = DPU_STATE_POWER_CYCLE - recovery['boot_start_time'] = time.time() - return - try: # Keep the same graceful sequence as module_base: # pre-shutdown -> admin down -> admin up -> post-startup. diff --git a/sonic-chassisd/tests/test_dpu_auto_recovery.py b/sonic-chassisd/tests/test_dpu_auto_recovery.py index a507dc992..12536d50a 100644 --- a/sonic-chassisd/tests/test_dpu_auto_recovery.py +++ b/sonic-chassisd/tests/test_dpu_auto_recovery.py @@ -1952,37 +1952,6 @@ def test_power_cycle_skipped_when_lock_not_acquired(self): # operation (gnoi shutdown/reboot) to complete. assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE - def test_power_cycle_aborted_when_legacy_field_set_after_lock(self): - """If transition_in_progress appears after lock acquired, abort.""" - chassis = create_chassis_with_dpus(1) - updater = create_updater(chassis) - updater.dpu_recovery_state["DPU0"]['state'] = DPU_STATE_WAIT_FOR_SELF_RECOVERY - updater.dpu_recovery_state["DPU0"]['reset_count'] = 0 - - module = chassis.module_list[0] - module.set_module_state_transition = MagicMock(return_value=True) - module.clear_module_state_transition = MagicMock(return_value=True) - module.set_admin_state = MagicMock() - module.pci_detach = MagicMock() - module.pci_reattach = MagicMock() - - # Simulate legacy field being set (gnoi started a shutdown concurrently) - updater._is_planned_transition_in_progress = MagicMock(return_value=True) - - updater._enter_power_cycle_or_unrecoverable("DPU0", 0) - - # Lock was acquired but then released due to legacy field detection - module.set_module_state_transition.assert_called_once_with("DPU0", TRANSITION_TYPE_RECOVERY) - module.clear_module_state_transition.assert_called_once_with("DPU0") - # Power-cycle should NOT have been executed - module.pci_detach.assert_not_called() - module.set_admin_state.assert_not_called() - # reset_count should be rolled back to 0 - assert updater.dpu_recovery_state["DPU0"]['reset_count'] == 0 - # State should have entered POWER_CYCLE to wait for the in-progress - # operation (gnoi shutdown/reboot) to complete. - assert updater.dpu_recovery_state["DPU0"]['state'] == DPU_STATE_POWER_CYCLE - # ============================================================================ # Test: DB setter methods