diff --git a/scripts/sonic-kdump-config b/scripts/sonic-kdump-config index 6ece6769d3..720bf5c3c6 100755 --- a/scripts/sonic-kdump-config +++ b/scripts/sonic-kdump-config @@ -41,6 +41,21 @@ machine_cfg = "/host/machine.conf" def print_err(*args): sys.stderr.write(' '.join(map(str,args)) + '\n') +## Validate remote argument - only accept 'enable' or 'disable' +def validate_remote_action(value): + """Validate remote argument - only accepts 'enable' or 'disable'""" + if isinstance(value, bool): + return value + if value.lower() == 'enable': + return True + elif value.lower() == 'disable': + return False + else: + raise argparse.ArgumentTypeError( + "Invalid action. Use 'enable' or 'disable'." + ) + + ## Run an external command, either from the shell or not # The function capture the output of stdout and stderr, # and return then a tupple with exit code, stdout, stderr @@ -871,19 +886,20 @@ def cmd_kdump_num_dumps(verbose, num_dumps): kdump_enabled = get_kdump_administrative_mode() kdump_memory = get_kdump_memory() -def cmd_kdump_remote(verbose): - remote = get_kdump_remote() # Retrieve the remote value from the config database +def cmd_kdump_remote(verbose, remote): + """Enable or disable remote kdump feature and save to CONFIG_DB""" + # Save the remote setting to CONFIG_DB + config_db = ConfigDBConnector(use_unix_socket_path=True) + if config_db is not None: + config_db.connect() + # Convert boolean to string for CONFIG_DB + remote_str = "true" if remote else "false" + config_db.mod_entry("KDUMP", "config", {"remote": remote_str}) - if remote: - # Uncomment SSH and SSH_KEY in the /etc/default/kdump-tools file - run_command("/bin/sed -i 's/^#SSH/SSH/' /etc/default/kdump-tools", use_shell=False) - if verbose: - print("SSH and SSH_KEY uncommented for remote configuration.") - else: - # Comment out SSH and SSH_KEY in the /etc/default/kdump-tools file - run_command("/bin/sed -i 's/^SSH/#SSH/' /etc/default/kdump-tools", use_shell=False) - if verbose: - print("SSH and SSH_KEY commented out for local configuration.") + if remote: + print("Remote kdump feature enabled.") + else: + print("Remote kdump feature disabled.") def cmd_kdump_ssh_string(verbose, ssh_string): if ssh_string is None: @@ -945,9 +961,8 @@ def main(): parser.add_argument('--num_dumps', nargs='?', type=int, action='store', default=False, help='Maximum number of kernel dump files stored') - parser.add_argument('--remote', action='store_true', default=False, - help='Update remote kdump via SSH based on CONFIG_DB') - + parser.add_argument('--remote', type=validate_remote_action, nargs='?', const=True, default=None, + help='Enable or disable remote kdump via SSH. Use: enable or disable') parser.add_argument('--ssh_string', nargs='?', type=str, action='store', default=False, help='ssh_string for remote kdump') @@ -987,8 +1002,8 @@ def main(): changed = cmd_kdump_disable(options.verbose) elif options.memory != False: cmd_kdump_memory(options.verbose, options.memory) - elif options.remote: - cmd_kdump_remote(options.verbose) + elif options.remote is not None: + cmd_kdump_remote(options.verbose, options.remote) elif options.ssh_string: cmd_kdump_ssh_string(options.verbose, options.ssh_string) elif options.ssh_path: diff --git a/show/reboot_cause.py b/show/reboot_cause.py index 07ea94e714..36a869b9e7 100644 --- a/show/reboot_cause.py +++ b/show/reboot_cause.py @@ -26,7 +26,7 @@ def read_reboot_cause_file(): # Function to fetch reboot cause data from database -def fetch_data_from_db(module_name, fetch_history=False, use_chassis_db=False): +def fetch_data_from_db(module_name, fetch_history=False, use_chassis_db=False, verbose=False): if module_name is None: prefix = 'REBOOT_CAUSE|2' elif "DPU" in module_name: @@ -90,6 +90,10 @@ def fetch_data_from_db(module_name, fetch_history=False, use_chassis_db=False): table.append(r) elif fetch_history: r.append(entry['comment'] if 'comment' in entry else "") + # Add verbose fields if requested + if verbose: + r.append(entry['power_cycle'] if 'power_cycle' in entry else "N/A") + r.append(entry['reboot_type'] if 'reboot_type' in entry else "N/A") if module_name is None or module_name == 'all' or "DPU" in module: table.append(r) @@ -120,16 +124,16 @@ def fetch_reboot_cause_from_db(module_name): # Function to fetch reboot cause history data from database REBOOT_CAUSE table -def fetch_reboot_cause_history_from_db(module_name): +def fetch_reboot_cause_history_from_db(module_name, verbose=False): if module_name == "all": # Combine data from both Redis containers for "all" modules - data_switch = fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=False) - data_dpu = fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=True) + data_switch = fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=False, verbose=verbose) + data_dpu = fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=True, verbose=verbose) return data_switch + data_dpu elif module_name is None: - return fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=False) + return fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=False, verbose=verbose) else: - return fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=True) + return fetch_data_from_db(module_name, fetch_history=True, use_chassis_db=True, verbose=verbose) # # 'reboot-cause' group ("show reboot-cause") @@ -188,16 +192,23 @@ def all(): required=False, type=click.Choice(get_all_dpu_options(), case_sensitive=False) if is_smartswitch() else None ) -def history(module_name=None): +@click.option('-v', '--verbose', is_flag=True, help='Show verbose reboot-cause details') +def history(module_name=None, verbose=False): """Show history of reboot-cause""" if not is_smartswitch() and module_name: click.echo("module option is supported only for smartswitch platform") return - reboot_cause_history = fetch_reboot_cause_history_from_db(module_name) + reboot_cause_history = fetch_reboot_cause_history_from_db(module_name, verbose) if is_smartswitch() and module_name: - header = ['Device', 'Name', 'Cause', 'Time', 'User', 'Comment'] + if verbose: + header = ['Device', 'Name', 'Cause', 'Time', 'User', 'Comment', 'Power Cycle', 'Reboot Type'] + else: + header = ['Device', 'Name', 'Cause', 'Time', 'User', 'Comment'] else: - header = ['Name', 'Cause', 'Time', 'User', 'Comment'] + if verbose: + header = ['Name', 'Cause', 'Time', 'User', 'Comment', 'Power Cycle', 'Reboot Type'] + else: + header = ['Name', 'Cause', 'Time', 'User', 'Comment'] if reboot_cause_history: click.echo(tabulate(reboot_cause_history, header, numalign="left")) diff --git a/tests/sonic_kdump_config_test.py b/tests/sonic_kdump_config_test.py index ea4cd7c0f3..0921e3e323 100644 --- a/tests/sonic_kdump_config_test.py +++ b/tests/sonic_kdump_config_test.py @@ -26,25 +26,51 @@ class TestRemoteFlag(unittest.TestCase): def setUp(self): # Create a new ArgumentParser for each test - self.parser = argparse.ArgumentParser(description="kdump configuration and status tool") - self.parser.add_argument('--remote', action='store_true', default=False, - help='Enable the Kdump remote SSH mechanism') + self.parser = argparse.ArgumentParser( + description="kdump configuration and status tool" + ) + self.parser.add_argument( + '--remote', + type=sonic_kdump_config.validate_remote_action, + nargs='?', + const=True, + default=None, + help='Enable or disable remote kdump via SSH. Use: enable or disable' + ) - def test_remote_flag_provided(self): - """Test that the --remote flag sets the remote attribute to True.""" - with patch.object(sys, 'argv', ['script.py', '--remote']): + def test_remote_flag_with_enable(self): + """Test that --remote enable sets the remote attribute to True.""" + with patch.object(sys, 'argv', ['script.py', '--remote', 'enable']): args = self.parser.parse_args() self.assertTrue(args.remote) + def test_remote_flag_with_disable(self): + """Test that --remote disable sets the remote attribute to False.""" + with patch.object(sys, 'argv', ['script.py', '--remote', 'disable']): + args = self.parser.parse_args() + self.assertFalse(args.remote) + def test_remote_flag_not_provided(self): - """Test that the --remote flag defaults to False when not provided.""" + """Test that the --remote flag defaults to None when not provided.""" with patch.object(sys, 'argv', ['script.py']): args = self.parser.parse_args() - self.assertFalse(args.remote) + self.assertIsNone(args.remote) + + def test_remote_flag_with_invalid_value_true(self): + """Test that providing 'true' to the --remote flag raises an error.""" + with patch.object(sys, 'argv', ['script.py', '--remote', 'true']): + with self.assertRaises(SystemExit): + self.parser.parse_args() + + def test_remote_flag_with_invalid_value_false(self): + """Test that providing 'false' to the --remote flag raises an error.""" + with patch.object(sys, 'argv', ['script.py', '--remote', 'false']): + with self.assertRaises(SystemExit): + self.parser.parse_args() - def test_remote_flag_with_value(self): - """Test that providing a value to the --remote flag raises an error.""" - with patch.object(sys, 'argv', ['script.py', '--remote', 'some_value']): + def test_remote_flag_with_invalid_value(self): + """Test that providing an invalid value to the --remote flag raises an error.""" + with patch.object(sys, 'argv', ['script.py', '--remote', 'invalid_value']): with self.assertRaises(SystemExit): self.parser.parse_args() @@ -410,33 +436,35 @@ def test_write_kdump_remote_false(self, mock_read_remote, mock_run_command): mock_run_command.assert_any_call("/bin/sed -i 's/^SSH/#SSH/' /etc/default/kdump-tools", use_shell=False) self.assertEqual(mock_run_command.call_count, 1) - @patch("sonic_kdump_config.get_kdump_remote") - @patch("sonic_kdump_config.run_command") - def test_cmd_kdump_remote(self, mock_run_command, mock_read_remote): + @patch("sonic_kdump_config.ConfigDBConnector") + @patch("builtins.print") + def test_cmd_kdump_remote(self, mock_print, mock_config_db_class): """Tests the function `cmd_kdump_remote(...)` in script `sonic-kdump-config`.""" - # Test case: Remote is True - mock_read_remote.return_value = True - sonic_kdump_config.cmd_kdump_remote(verbose=True) + # Create a mock ConfigDBConnector instance + mock_config_db = Mock() + mock_config_db_class.return_value = mock_config_db - # Ensure the correct commands are being run - mock_run_command.assert_any_call("/bin/sed -i 's/^#SSH/SSH/' /etc/default/kdump-tools", use_shell=False) + # Test case: Remote is True (enable) + sonic_kdump_config.cmd_kdump_remote(verbose=True, remote=True) - # Test case: Remote is False - mock_read_remote.return_value = False - sonic_kdump_config.cmd_kdump_remote(verbose=True) + # Verify CONFIG_DB was updated with "true" + mock_config_db.connect.assert_called_once() + mock_config_db.mod_entry.assert_called_with("KDUMP", "config", {"remote": "true"}) + mock_print.assert_called_with("Remote kdump feature enabled.") - # Ensure the correct commands are being run - mock_run_command.assert_any_call("/bin/sed -i 's/^SSH/#SSH/' /etc/default/kdump-tools", use_shell=False) + # Reset mocks for next test + mock_config_db.reset_mock() + mock_print.reset_mock() + mock_config_db_class.return_value = mock_config_db - # Test case: Checking output messages - with patch("builtins.print") as mock_print: - sonic_kdump_config.cmd_kdump_remote(verbose=True) - mock_print.assert_called_with("SSH and SSH_KEY commented out for local configuration.") + # Test case: Remote is False (disable) + sonic_kdump_config.cmd_kdump_remote(verbose=True, remote=False) - mock_read_remote.return_value = False - sonic_kdump_config.cmd_kdump_remote(verbose=True) - mock_print.assert_called_with("SSH and SSH_KEY commented out for local configuration.") + # Verify CONFIG_DB was updated with "false" + mock_config_db.connect.assert_called_once() + mock_config_db.mod_entry.assert_called_with("KDUMP", "config", {"remote": "false"}) + mock_print.assert_called_with("Remote kdump feature disabled.") @patch('sonic_kdump_config.getstatusoutput_noshell_pipe') def test_read_ssh_string(self, mock_run_cmd):