Fix sonic-kdump-config --remote parameter handling

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:

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"))

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):