Skip to content
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

Kdump Remote ssh yang enhancements #19540

Merged
merged 11 commits into from
Feb 10, 2025
Merged

Conversation

ridahanif96
Copy link
Contributor

Why I did it

Added support for Kdump remote SSH feature in existing sonic-kdump.yang

How I did it

Modified sonic-kdump.yang file to add support for ssh , ssh_connection and ssh_path strings

How to verify it

Updated yang model with required changes for feature

@ridahanif96
Copy link
Contributor Author

@venkatmahalingam @ganglyu pls review this code PR. Thanks

type string;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

would you please add unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oky will add

@ridahanif96
Copy link
Contributor Author

@ganglyu can you pls help review this PR.

Copy link

@muhammadalihussnain muhammadalihussnain left a comment

Choose a reason for hiding this comment

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

@venkatmahalingam Please review the Code PR

@muhammadalihussnain
Copy link

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

leaf ssh_path {
description "Remote ssh private key path";
type string;
}
Copy link
Contributor

@maipbui maipbui Oct 11, 2024

Choose a reason for hiding this comment

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

Could you update these new changes to doc/Configuration.md? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, please review

"memory": "0M-2G:256M,2G-4G:256M,4G-8G:384M,8G-:448M"
"memory": "0M-2G:256M,2G-4G:256M,4G-8G:384M,8G-:448M",
"remote": "true",
"ssh_string" : "username@ipadress",
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Oct 21, 2024

Choose a reason for hiding this comment

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

This can't be the sample config, please use the actual configuration e.g [email protected] and actual private key path, this is the problem of using the generic string type, see if you can use string pattern for user name and ip address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Updating as per suggestion

@muhammadalihussnain
Copy link

/azpw run Azure.sonic-buildimage

@ridahanif96
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ridahanif96
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ridahanif96
Copy link
Contributor Author

@venkatmahalingam please help review updated changes. Thanks in advance!

@ridahanif96
Copy link
Contributor Author

@qiluo-msft please help merge this PR

@wajahatrazi
Copy link

Hi @qiluo-msft, please review to 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 Author

@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
Collaborator

@liushilongbuaa can you please check the conflict? Thanks.

@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

1 similar comment
@muhammadalihussnain
Copy link

/azpw ms_conflict

"Enable or Disable the Kdump remote ssh mechanism";
}

leaf ssh_string {

Choose a reason for hiding this comment

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

Default key added in hostcfgd doesn't match the Yang model (https://github.com/muhammadalihussnain/sonic-host-services/blob/3417b5f8a010901131a4646685efb3e61822ef2a/scripts/hostcfgd#L1142) for both ssh_string and ssh_key. I would suggest to update the default keys in hostcfgd both to smaller case.

Choose a reason for hiding this comment

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

Thanks for sharing your suggestion.

As we have hostcfg code merged to master, we have two options available to modify in the YANG model.

  • Replace names in Upper case, leaf SSH_KEY & leaf SSH_PATH, with the current ones leaf ssh_string & leaf ssh_path.
    or
  • Add deviations ensuring compatibility with hostcfgd by setting defaults, keeping lowercase of leaf ssh_string & leaf ssh_path

What would you suggest? Thanks

Choose a reason for hiding this comment

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

I would suggest to change both SSH_KEY and SSH_PATH to lowercase in hostcfgd to make it consistent with other config variables.

@muhammadalihussnain
Copy link

HI @ram25794 , we have made the changes in the new PR per your suggestions. Kindly review and help to merge PR

@muhammadalihussnain
Copy link

@ram25794, the changes have been made in the PR here as per your suggestion. Please review, and I would appreciate your approval to merge it

@qiluo-msft qiluo-msft merged commit 01518cd into sonic-net:master Feb 10, 2025
22 checks passed
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.