Skip to content

Conversation

@arista-hpandya
Copy link
Contributor

@arista-hpandya arista-hpandya commented Nov 3, 2025

Fixes #4115

What I did

Fixed bugs in the upstream kdump remote SSH feature:

  1. Fixed regex patterns in sed commands to prevent unintended replacements
  2. Removed redundant SSH_KEY operations
  3. Fixed typo: options.verbosoptions.verbose
  4. Simplified SSH path update logic by removing unnecessary comparison check
  5. Updated help text for --remote flag to better reflect its functionality
  6. Changed delimiter in write_ssh_path() sed command from / to | to handle paths correctly

How I did it

1. Fixed sed regex patterns:

  • Added ^ anchor to sed patterns (s/#SSH/SSH/s/^#SSH/SSH/) to ensure only lines starting with #SSH are matched, preventing false matches in comments or other parts of the configuration file

2. Removed redundant SSH_KEY operations:

  • The SSH_KEY line doesn't need separate toggling since it's controlled by the same sed command. Removed duplicate sed commands for SSH_KEY in both write_kdump_remote() and cmd_kdump_remote() functions

3. Fixed typo:

  • Corrected options.verbos to options.verbose in the main argument parsing section

4. Simplified SSH path update:

  • Removed unnecessary check comparing current SSH path with new path in cmd_kdump_ssh_path(). The function now always writes the path when provided, simplifying the logic

5. Updated sed delimiter:

  • Changed delimiter in write_ssh_path() from / to | (s/#*SSH_KEY=.*/s|#*SSH_KEY=.*|) to properly handle SSH key paths that may contain forward slashes

6. Updated tests:

  • Modified unit tests to reflect the simplified implementation
  • Removed obsolete test case test_cmd_kdump_ssh_path_no_update that tested the removed comparison logic

How to verify it

Initial state:
Manually uncommented SSH and SSH_KEY config in /etc/default/kdump-tools
After reboot we expect the hostcfgd service to start and uncomment it because KDUMP|config|remote is false

After reboot:
#SSH="user@localhost"
#SSH_KEY="/a/b/c"

>>> root@device:~# sonic-kdump-config --ssh_string "trial@localhost"
SSH string updated. Changes will take effect after the next reboot.
>>> root@device:~# sonic-kdump-config --ssh_path "/var/"
SSH path updated. Changes will take effect after reboot.

State of /etc/default/kdump-tools:
SSH="trial@localhost"
SSH_KEY="/var/"

>>> root@device:~# sonic-db-cli CONFIG_DB HGETALL "KDUMP|config"
{'enabled': 'true', 'memory': '0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M', 'num_dumps': '3', 'remote': 'false', 'ssh_string': 'user@localhost', 'ssh_path': '/a/b/c'}
>>> root@device:~# sonic-kdump-config --remote 

State of /etc/default/kdump-tools:
#SSH="trial@localhost"
#SSH_KEY="/var/"

After another reboot:

State of /etc/default/kdump-tools:
#SSH="trial@localhost"
#SSH_KEY="/var/"

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@judyjoseph judyjoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@judyjoseph judyjoseph requested a review from Copilot November 21, 2025 18:54
Copilot finished reviewing on behalf of judyjoseph November 21, 2025 18:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes several syntax and semantic errors in the kdump remote SSH feature. The changes correct regex patterns to prevent unintended replacements, remove redundant operations, fix a typo, and simplify logic for better maintainability.

Key Changes:

  • Fixed sed regex patterns by adding ^ anchors to match only at line start, preventing false matches in comments or other configuration sections
  • Corrected typo options.verbosoptions.verbose in command-line argument handling
  • Changed sed delimiter from / to | in SSH path updates to properly handle paths containing forward slashes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
scripts/sonic-kdump-config Fixed sed regex patterns with ^ anchors, removed redundant SSH_KEY sed commands (single command now handles both SSH and SSH_KEY lines), fixed typo in argument parsing, simplified SSH path update logic, and changed sed delimiter to handle paths correctly
tests/sonic_kdump_config_test.py Updated test assertions to verify single sed command execution instead of two, corrected expected sed patterns to include ^ anchors and pipe delimiters, and removed obsolete test case for comparison logic that was simplified

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@arista-hpandya
Copy link
Contributor Author

@rlhui @arlakshm Can we merge this PR? All check has passed and the code has been reviewed.

@judyjoseph judyjoseph merged commit 15af8f1 into sonic-net:master Nov 22, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Kdump non-functional due to syntax and semantic errors in kdump remote feature

3 participants