From eae7b910777cd7a750d9705c4115592f288325ac Mon Sep 17 00:00:00 2001 From: Sonic Build Admin Date: Wed, 1 Jul 2026 05:36:44 +0000 Subject: [PATCH] [BMC] Improvements in show chassis/platform leak_profile commands #### What I did * Fix to read leak_profile table from STATE_DB * default admin_status as down for switch-host * add Power-On-Delay (sec) Shutdown-Timeout (sec) to show chassis module status * Remove the Physical-Slot column in show chassis module status #### How I did it #### How to verify it ``` admin@host1-remote-bmc:~$ sudo config chassis modules startup SWITCH-HOST Starting up chassis module SWITCH-HOST admin@host1-remote-bmc:~$ show chassis modules status Name Description Physical-Slot Oper-Status Admin-Status Serial ----------- --------------------------------------- --------------- ------------- -------------- -------- SWITCH-HOST Main x86 Switch Host CPU managed by BMC N/A ONLINE up N/A admin@host1-remote-bmc:~$ sudo config chassis modules shutdown SWITCH-HOST Module SWITCH-HOST is already in down state admin@host1-remote-bmc:~$ show chassis modules status Name Description Physical-Slot Oper-Status Admin-Status Serial ----------- --------------------------------------- --------------- ------------- -------------- -------- SWITCH-HOST Main x86 Switch Host CPU managed by BMC N/A OFFLINE down N/A dmin@host1-remote-bmc:~$ show platform leak profiles Sensor-Type Max-Minor-Duration-Sec ------------- ------------------------ default 300 o/p after removing the Physical Slot admin@host1-remote-bmc:~$ show chassis module status Name Description Oper Status Admin Status Serial Power-On-Delay (sec) Shutdown-Timeout (sec) -------------------------------------------------------------------------------------------------------------------------------- SWITCH-HOST Switch Host online up ABC123 300 120 ``` Signed-off-by: Sonic Build Admin #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed) --- config/chassis_modules.py | 4 + show/chassis_modules.py | 24 +++++ show/platform.py | 12 +-- tests/chassis_modules_test.py | 157 +++++++++++++++++++++++++++++++ tests/show_platform_leak_test.py | 23 ++--- 5 files changed, 199 insertions(+), 21 deletions(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 724d44683..b5f40257b 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -252,6 +252,8 @@ def set_power_on_delay(db, chassis_module_name, seconds): config_db = db.cfgdb fvs = config_db.get_entry('CHASSIS_MODULE', chassis_module_name) or {} fvs['power_on_delay'] = str(seconds) + # Seed admin_status of SWITCH-HOST with default if not already set + fvs.setdefault('admin_status', 'down') config_db.set_entry('CHASSIS_MODULE', chassis_module_name, fvs) click.echo(f"Power-on-delay for {chassis_module_name} set to {seconds} seconds") @@ -272,5 +274,7 @@ def set_graceful_shutdown_timeout(db, chassis_module_name, seconds): config_db = db.cfgdb fvs = config_db.get_entry('CHASSIS_MODULE', chassis_module_name) or {} fvs['graceful_shutdown_timeout'] = str(seconds) + # Seed admin_status of SWITCH-HOST with default if not already set + fvs.setdefault('admin_status', 'down') config_db.set_entry('CHASSIS_MODULE', chassis_module_name, fvs) click.echo(f"Shutdown-timeout for {chassis_module_name} set to {seconds} seconds") diff --git a/show/chassis_modules.py b/show/chassis_modules.py index d32176726..f2b7b76db 100644 --- a/show/chassis_modules.py +++ b/show/chassis_modules.py @@ -48,9 +48,16 @@ def status(db, chassis_module_name): """Show chassis-modules status""" smartswitch = is_smartswitch() + bmc = is_bmc() header = ['Name', 'Description', 'Physical-Slot', 'Oper-Status', 'Admin-Status', 'Serial'] if smartswitch: header.append('Ready-Status') + if bmc: + # Physical-Slot is not meaningful on BMC; drop it and add the + # BMC-only timing fields configured via 'config chassis modules + # power-on-delay' / 'shutdown-timeout' for SWITCH-HOST modules. + header.remove('Physical-Slot') + header.extend(['Power-On-Delay (sec)', 'Shutdown-Timeout (sec)']) chassis_cfg_table = db.cfgdb.get_table('CHASSIS_MODULE') @@ -124,6 +131,9 @@ def status(db, chassis_module_name): # Determine admin_status if smartswitch: admin_status = 'down' + elif is_bmc() and key_list[1].startswith("SWITCH-HOST"): + # On BMC, SWITCH-HOST default is 'down' (kept powered off on boot) + admin_status = 'down' else: admin_status = 'up' config_data = chassis_cfg_table.get(key_list[1]) @@ -131,12 +141,26 @@ def status(db, chassis_module_name): admin_status = config_data.get(CHASSIS_MODULE_INFO_ADMINSTATUS_FIELD, admin_status) row = [key_list[1], desc, slot, oper_status, admin_status, serial] + if bmc: + # Physical-Slot column omitted from header on BMC; drop matching value + row.pop(2) if smartswitch: dpu_info = dpu_state_data.get(key_list[1], {}) ready_status = dpu_info.get(DPU_STATE_READY_STATUS_FIELD, 'N/A') row.append(ready_status) + if bmc: + # Only meaningful for SWITCH-HOST modules; other module types show N/A. + if key_list[1].startswith("SWITCH-HOST"): + cfg = config_data or {} + power_on_delay = cfg.get('power_on_delay', '0') + shutdown_timeout = cfg.get('graceful_shutdown_timeout', '120') + else: + power_on_delay = 'N/A' + shutdown_timeout = 'N/A' + row.extend([power_on_delay, shutdown_timeout]) + table.append(tuple(row)) if chassis_state_db: diff --git a/show/platform.py b/show/platform.py index 693d5a6dc..5b0604811 100644 --- a/show/platform.py +++ b/show/platform.py @@ -381,14 +381,14 @@ def leak_rack_manager_alerts(): def leak_profiles(): """Show leak sensor profiles""" try: - from utilities_common.db import Db - db = Db() - keys = db.cfgdb.get_keys(LEAK_PROFILE_TABLE) or [] + state_db = _get_state_db() + keys = state_db.keys(state_db.STATE_DB, f"{LEAK_PROFILE_TABLE}|*") or [] header = ['Sensor-Type', 'Max-Minor-Duration-Sec'] rows = [] - for sensor_type in sorted(keys): - entry = db.cfgdb.get_entry(LEAK_PROFILE_TABLE, sensor_type) - max_dur = entry.get('max_minor_duration_sec', 'N/A') + for key in sorted(keys): + sensor_type = key.split('|', 1)[1] + data = state_db.get_all(state_db.STATE_DB, key) or {} + max_dur = data.get('max_minor_duration_sec', 'N/A') rows.append((sensor_type, max_dur)) if rows: click.echo(tabulate(rows, header, tablefmt='simple')) diff --git a/tests/chassis_modules_test.py b/tests/chassis_modules_test.py index 1d546f340..04fe8ec36 100755 --- a/tests/chassis_modules_test.py +++ b/tests/chassis_modules_test.py @@ -708,6 +708,69 @@ def test_both_fields_preserved_in_db(self): assert entry.get("power_on_delay") == "60" assert entry.get("graceful_shutdown_timeout") == "30" + def test_power_on_delay_seeds_admin_status_when_missing(self): + """First-time 'power-on-delay' on a fresh DB must seed admin_status='down' + so the entry stays valid for downstream consumers (startup/shutdown).""" + runner = CliRunner() + db = Db() + # Sanity: no entry exists + assert db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") == {} + result = runner.invoke( + self.modules.commands["power-on-delay"], + ["SWITCH-HOST", "300"], + obj=db + ) + assert result.exit_code == 0 + entry = db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") + assert entry.get("power_on_delay") == "300" + assert entry.get("admin_status") == "down" + + def test_power_on_delay_does_not_overwrite_existing_admin_status(self): + """If admin_status is already configured (e.g. 'up'), 'power-on-delay' + must NOT downgrade it back to 'down'.""" + runner = CliRunner() + db = Db() + db.cfgdb.mod_entry('CHASSIS_MODULE', 'SWITCH-HOST', {'admin_status': 'up'}) + result = runner.invoke( + self.modules.commands["power-on-delay"], + ["SWITCH-HOST", "300"], + obj=db + ) + assert result.exit_code == 0 + entry = db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") + assert entry.get("power_on_delay") == "300" + assert entry.get("admin_status") == "up", "existing admin_status must be preserved" + + def test_shutdown_timeout_seeds_admin_status_when_missing(self): + """First-time 'shutdown-timeout' on a fresh DB must seed admin_status='down'.""" + runner = CliRunner() + db = Db() + assert db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") == {} + result = runner.invoke( + self.modules.commands["shutdown-timeout"], + ["SWITCH-HOST", "120"], + obj=db + ) + assert result.exit_code == 0 + entry = db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") + assert entry.get("graceful_shutdown_timeout") == "120" + assert entry.get("admin_status") == "down" + + def test_shutdown_timeout_does_not_overwrite_existing_admin_status(self): + """If admin_status='up' already, 'shutdown-timeout' must not downgrade it.""" + runner = CliRunner() + db = Db() + db.cfgdb.mod_entry('CHASSIS_MODULE', 'SWITCH-HOST', {'admin_status': 'up'}) + result = runner.invoke( + self.modules.commands["shutdown-timeout"], + ["SWITCH-HOST", "120"], + obj=db + ) + assert result.exit_code == 0 + entry = db.cfgdb.get_entry("CHASSIS_MODULE", "SWITCH-HOST") + assert entry.get("graceful_shutdown_timeout") == "120" + assert entry.get("admin_status") == "up", "existing admin_status must be preserved" + @classmethod def teardown_class(cls): print("TEARDOWN") @@ -864,6 +927,100 @@ def test_show_status_bmc_module_helper_init_failure_falls_back_to_db(self): assert result.exit_code == 0 assert "Empty" in result.output + def test_show_status_bmc_header_drops_physical_slot_adds_timing_columns(self): + """On BMC, Physical-Slot column is removed and Power-On-Delay / + Shutdown-Timeout columns are appended.""" + runner = CliRunner() + with mock.patch('show.chassis_modules.is_bmc', return_value=True), \ + mock.patch('show.chassis_modules.ModuleHelper', side_effect=Exception("no chassis")): + result = runner.invoke( + show.cli.commands["chassis"].commands["modules"].commands["status"], + ["LINE-CARD0"] + ) + print(result.output) + assert result.exit_code == 0 + assert "Physical-Slot" not in result.output + assert "Power-On-Delay" in result.output + assert "Shutdown-Timeout" in result.output + + def test_show_status_bmc_non_switch_host_shows_na_for_timing(self): + """On BMC, non-SWITCH-HOST rows display N/A for the timing columns.""" + runner = CliRunner() + with mock.patch('show.chassis_modules.is_bmc', return_value=True), \ + mock.patch('show.chassis_modules.ModuleHelper', side_effect=Exception("no chassis")): + result = runner.invoke( + show.cli.commands["chassis"].commands["modules"].commands["status"], + ["LINE-CARD0"] + ) + print(result.output) + assert result.exit_code == 0 + assert "N/A" in result.output + + def test_show_status_non_bmc_output_unchanged(self): + """Regression: non-BMC output must still contain Physical-Slot and + must NOT contain the new BMC-only timing columns.""" + runner = CliRunner() + with mock.patch('show.chassis_modules.is_bmc', return_value=False): + result = runner.invoke( + show.cli.commands["chassis"].commands["modules"].commands["status"], + [] + ) + print(result.output) + assert result.exit_code == 0 + assert "Physical-Slot" in result.output + assert "Power-On-Delay" not in result.output + assert "Shutdown-Timeout" not in result.output + + def test_show_status_bmc_switch_host_shows_configured_timing_values(self): + """On BMC, SWITCH-HOST row shows the configured power-on-delay and + graceful-shutdown-timeout values from CONFIG_DB.""" + runner = CliRunner() + db = Db() + db.cfgdb.mod_entry('CHASSIS_MODULE', 'SWITCH-HOST', { + 'admin_status': 'up', + 'power_on_delay': '300', + 'graceful_shutdown_timeout': '90', + }) + + # Mock state_db to return a SWITCH-HOST entry (mock_tables doesn't have one) + switch_host_state = { + 'desc': 'Switch Host', + 'slot': '1', + 'oper_status': 'Online', + 'serial': 'SH1000101', + } + + def fake_keys(_db_id, pattern): + if 'SWITCH-HOST' in pattern: + return ['CHASSIS_MODULE_TABLE|SWITCH-HOST'] + return [] + + def fake_get_all(_db_id, key): + if key.endswith('|SWITCH-HOST'): + return switch_host_state + return {} + + with mock.patch('show.chassis_modules.is_bmc', return_value=True), \ + mock.patch('show.chassis_modules.ModuleHelper', side_effect=Exception("no chassis")), \ + mock.patch('show.chassis_modules.SonicV2Connector') as mock_conn_cls: + mock_conn = mock.MagicMock() + mock_conn.STATE_DB = 'STATE_DB' + mock_conn.keys.side_effect = fake_keys + mock_conn.get_all.side_effect = fake_get_all + mock_conn_cls.return_value = mock_conn + + result = runner.invoke( + show.cli.commands["chassis"].commands["modules"].commands["status"], + ["SWITCH-HOST"], + obj=db + ) + print(result.output) + assert result.exit_code == 0 + assert "SWITCH-HOST" in result.output + assert "300" in result.output + assert "90" in result.output + assert "Physical-Slot" not in result.output + @classmethod def teardown_class(cls): print("TEARDOWN") diff --git a/tests/show_platform_leak_test.py b/tests/show_platform_leak_test.py index 4993cf5b0..7137c7bd9 100644 --- a/tests/show_platform_leak_test.py +++ b/tests/show_platform_leak_test.py @@ -130,17 +130,13 @@ def setup_class(cls): def test_leak_profiles_with_data(self): runner = CliRunner() - mock_cfgdb = MagicMock() - mock_cfgdb.get_keys.return_value = ['rope', 'spot', 'flex_pcb'] - mock_cfgdb.get_entry.side_effect = lambda t, k: { - 'rope': {'max_minor_duration_sec': '300'}, - 'spot': {'max_minor_duration_sec': '600'}, - 'flex_pcb': {'max_minor_duration_sec': '180'}, - }[k] - mock_db = MagicMock() - mock_db.cfgdb = mock_cfgdb + state_db = _make_state_db({ + 'LEAK_PROFILE|rope': {'type': 'rope', 'max_minor_duration_sec': '300'}, + 'LEAK_PROFILE|spot': {'type': 'spot', 'max_minor_duration_sec': '600'}, + 'LEAK_PROFILE|flex_pcb': {'type': 'flex_pcb', 'max_minor_duration_sec': '180'}, + }) - with patch('utilities_common.db.Db', return_value=mock_db): + with patch('show.platform._get_state_db', return_value=state_db): result = runner.invoke( show_platform.platform.commands['leak'].commands['profiles'] ) @@ -153,12 +149,9 @@ def test_leak_profiles_with_data(self): def test_leak_profiles_empty(self): runner = CliRunner() - mock_cfgdb = MagicMock() - mock_cfgdb.get_keys.return_value = [] - mock_db = MagicMock() - mock_db.cfgdb = mock_cfgdb + state_db = _make_state_db({}) - with patch('utilities_common.db.Db', return_value=mock_db): + with patch('show.platform._get_state_db', return_value=state_db): result = runner.invoke( show_platform.platform.commands['leak'].commands['profiles'] )