Skip to content

kdump-remote feature in hostcfgd #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

muhammadalihussnain
Copy link
Contributor

@muhammadalihussnain muhammadalihussnain commented Sep 18, 2024

we want to add three new features to the KDUMP table and enable the KDUMP feature remotely.

@muhammadalihussnain
Copy link
Contributor Author

This PR is related to or dependent on sonic-net/SONiC#1714.

@muhammadalihussnain
Copy link
Contributor Author

@venkatmahalingam Hi! we have updated the code. Please Help us review the Code PRs. Thanks

@ridahanif96
Copy link
Contributor

@venkatmahalingam Hi! we have updated the code. Please Help us review the Code PRs. Thanks in advance , target is 202411

@@ -1147,7 +1152,7 @@ class KdumpCfg(object):
for row in self.kdump_defaults:
value = self.kdump_defaults.get(row)
if not kdump_conf.get(row):
self.config_db.mod_entry("KDUMP", "config", {row : value})

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is still there, it is not removed. Added Detail for remote configs here in kdump table
image

scripts/hostcfgd Outdated
ssh_path = self.kdump_defaults["SSH_PATH"]
if data.get("SSH_PATH") is not None:
ssh_path = data.get("SSH_PATH")
run_cmd(["sonic-kdump-config", "--ssh_path", ssh_path])

Choose a reason for hiding this comment

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

Why should we execute run_cmd with following defaults when KDUMP remote is not desired by the user?
SSH_KEY": "user@server", # New feature: SSH key, default value
"SSH_PATH": ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @venkatmahalingam i have update the code as suggested above .

@wajahatrazi
Copy link

Hi @venkatmahalingam , please help merge this PR. Regards

@wajahatrazi
Copy link

Hi @qiluo-msft

PR has been reviewed by the reviewer with all checks passed, please help merge this.

@ridahanif96
Copy link
Contributor

@qiluo-msft help merge this PR, pending for long

Copy link

@wajahatrazi wajahatrazi left a comment

Choose a reason for hiding this comment

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

LGTM

@zhangyanzhao
Copy link

Can we merge this PR? Thanks.

@FengPan-Frank FengPan-Frank merged commit ca9d329 into sonic-net:master Jan 21, 2025
5 checks passed
@hdwhdw
Copy link
Contributor

hdwhdw commented Feb 6, 2025

This commit is not able to be merged into sonic-buildimage:
https://elastictest.org/scheduler/testplan/67a4e6f5f5a74203a8e1b8e8?testcase=show_techsupport%2Ftest_auto_techsupport.py&type=console

Match Messages:
E               2025 Feb  6 17:30:13.904639 vlab-03 ERR sonic_yang: All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])
E               
E               2025 Feb  6 17:30:13.904667 vlab-03 ERR sonic_yang: exceptionList:[]
E               
E               2025 Feb  6 17:30:13.904691 vlab-03 ERR sonic_yang: Data Loading Failed:All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])#012exceptionList:[]
E               
E               2025 Feb  6 17:30:13.904715 vlab-03 ERR ConfigMgmt: Data Loading Failed#012All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])#012exceptionList:[]

Merge PR sonic-net/sonic-buildimage#21565 I think the test fails on KVM.

@hdwhdw
Copy link
Contributor

hdwhdw commented Feb 6, 2025

@FengPan-Frank @qiluo-msft for viz

@wen587
Copy link

wen587 commented Feb 7, 2025

Hi @muhammadalihussnain , the error message indicates that ['SSH_KEY', 'SSH_PATH', 'remote'] missing in YANG fails the validation.
The YANG model file need to be updated first before modification on the related table: sonic-kdump.yang does not have these leaf
sonic-buildimage/src/sonic-yang-models/yang-models/sonic-kdump.yang at master · sonic-net/sonic-buildimage

This commit is not able to be merged into sonic-buildimage: https://elastictest.org/scheduler/testplan/67a4e6f5f5a74203a8e1b8e8?testcase=show_techsupport%2Ftest_auto_techsupport.py&type=console

Match Messages:
E               2025 Feb  6 17:30:13.904639 vlab-03 ERR sonic_yang: All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])
E               
E               2025 Feb  6 17:30:13.904667 vlab-03 ERR sonic_yang: exceptionList:[]
E               
E               2025 Feb  6 17:30:13.904691 vlab-03 ERR sonic_yang: Data Loading Failed:All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])#012exceptionList:[]
E               
E               2025 Feb  6 17:30:13.904715 vlab-03 ERR ConfigMgmt: Data Loading Failed#012All Keys are not parsed in KDUMP#012dict_keys(['SSH_KEY', 'SSH_PATH', 'remote'])#012exceptionList:[]

Merge PR sonic-net/sonic-buildimage#21565 I think the test fails on KVM.

@wajahatrazi
Copy link

Hi @wen587

@muhammadalihussnain

@hdwhdw
Copy link
Contributor

hdwhdw commented Feb 10, 2025

@wajahatrazi @wen587 We should probably revert this since it is blocking integration of other sonic-host-services features. Once the dependencies are in, we can revert the revert.

Or does #209 fix the aforementioned error?

@wajahatrazi
Copy link

wajahatrazi commented Feb 11, 2025

Hi @hdwhdw @wen587 . Following are the details:

We have tried to fix the problem, with hope that no errors comes up now :)

Details:

  • There were variables error in the HostCfgd for KDUMP, which were not reflecting with the YANG model.
  • @muhammadalihussnain created the PR#209, to fix the above reflection of variables, but there was a typo.
  • Now to fix everything as a whole, this new PR#216 has been created. This last PR has been created by to sync YANG model with the hostcfgd, replacing "ssh_key" with "ssh_string" as per YANG model.

Call 2 Action:

  • If we merge this PR#216, hostcfgd will fully reflect YANG, which may fix the errors.

Kindly suggest

Link to all PRs for the feature & HLD:
KDUMP_remote_ssh_enhancement

@hdwhdw
Copy link
Contributor

hdwhdw commented Feb 11, 2025

@wajahatrazi I merged #216 and have a new PR sonic-net/sonic-buildimage#21708 to track if the merge to sonic-buildimage:master is successful.

@hdwhdw
Copy link
Contributor

hdwhdw commented Feb 12, 2025

@wajahatrazi the build has failed with the following message:

12/02/2025 17:53:01 base._run                                L0108 DEBUG  | /var/src/sonic-mgmt/tests/common/devices/multi_asic.py::_run_on_asics#136: [vlab-01] AnsibleModule::shell Result => {"failed": true, "changed": true, "stdout": "sonic_yang(3):Data Loading Failed:Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).\nData Loading Failed\nValue \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).\nError: Failed to validate configuration: ConfigMgmt Class creation failed", "stderr": "libyang[0]: Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern). (path: /sonic-kdump:sonic-kdump/KDUMP/config/ssh_path)\nAborted!", "rc": 1, "cmd": "sudo config auto-techsupport global  rate-limit-interval 0", "start": "2025-02-12 17:53:00.240895", "end": "2025-02-12 17:53:01.298306", "delta": "0:00:01.057411", "msg": "non-zero return code", "invocation": {"module_args": {"_raw_params": "sudo config auto-techsupport global  rate-limit-interval 0", "_uses_shell": true, "warn": false, "stdin_add_newline": true, "strip_empty_ends": true, "argv": null, "chdir": null, "executable": null, "creates": null, "removes": null, "stdin": null}}, "stdout_lines": ["sonic_yang(3):Data Loading Failed:Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).", "Data Loading Failed", "Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).", "Error: Failed to validate configuration: ConfigMgmt Class creation failed"], "stderr_lines": ["libyang[0]: Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern). (path: /sonic-kdump:sonic-kdump/KDUMP/config/ssh_path)", "Aborted!"], "_ansible_no_log": null}

In particular:

"sonic_yang(3):Data Loading Failed:Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).", "Data Loading Failed", "Value \"<path>\" does not satisfy the constraint \"(/([a-zA-Z0-9._-]+|\\.)+)+\" (range, length, or pattern).",

This is from these two lines:

            "ssh_string": "<user@server>",   # New feature: SSH key, default value
            "ssh_path": "<path>"          # New feature: SSH path, default value

Can you fix it? Say leave the default value empty?

@wajahatrazi
Copy link

wajahatrazi commented Feb 13, 2025

@hdwhdw As per the YANG model, we can set the default value as follows:

  • ssh_string : user@localhost
  • ssh_path : a/b/c

If there is some alternative name that can is preferable, kindly suggest

We cannot set default values as empty because of constraints

           ` leaf ssh_string {
                type string {
                    pattern "([a-zA-Z0-9._%+-]+@([a-zA-Z0-9.-]+|[0-9]{1,3}(\\.[0-9]{1,3}){3}))";
                }
                description  "Remote ssh connection string";

            }

            leaf ssh_path {
                type string {
                    pattern "(/([a-zA-Z0-9._-]+|\\.)+)+";
                }
                description  "Remote ssh private key path";

            }`

@bingwang-ms
Copy link

Hi @wajahatrazi @muhammadalihussnain , we saw some new error logs in virtual testbed. The errors are blocking PR test. Can you help take a look?

E               2025 Apr 16 02:31:12.765084 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--disable'] - failed: return code - 1, output:#012None
E               
E               2025 Apr 16 02:31:13.437314 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--memory', '0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M'] - failed: return code - 1, output:#012None
E               
E               2025 Apr 16 02:31:14.202462 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--num_dumps', '3'] - failed: return code - 1, output:#012None
E               
E               2025 Apr 16 02:31:15.166080 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--remote', 'false'] - failed: return code - 2, output:#012None
E               
E               2025 Apr 16 02:31:16.601173 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--ssh_string', 'user@localhost'] - failed: return code - 1, output:#012None
E               
E               2025 Apr 16 02:31:19.068750 vlab-01 ERR hostcfgd: ['sonic-kdump-config', '--ssh_path', '/a/b/c'] - failed: return code - 1, output:#012None

@wajahatrazi
Copy link

@muhammadalihussnain

@wajahatrazi
Copy link

Hi @bingwang-ms,

We have made changes to fix the errors. Can you please review these?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants