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.

Sign up for GitHub

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

Jump to bottom

sonic-host-services changes for gNOI Warm Reboot #191

Merged
merged 2 commits into from
Mar 3, 2025
Merged

sonic-host-services changes for gNOI Warm Reboot #191

merged 2 commits into from
Mar 3, 2025
+504 −85

Conversation

rkavitha-hcl
Copy link
Contributor

@rkavitha-hcl rkavitha-hcl commented Nov 29, 2024 •
edited
Loading

Adding sonic-host-services changes for warm reboot .
Adding HALT method support for sonic-host-services

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 29, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

kishanps
kishanps reviewed Dec 6, 2024
View reviewed changes
@kishanps
Copy link

kishanps commented Dec 6, 2024

@github76543 Joh, can you PTAL and signoff.

@rkavitha-hcl rkavitha-hcl force-pushed the gnoi_wr_hs_new branch 2 times, most recently from a230e6e to f71371e Compare December 6, 2024 06:23
@rkavitha-hcl rkavitha-hcl mentioned this pull request Dec 13, 2024
Merged
@hdwhdw hdwhdw self-requested a review December 26, 2024 00:56
hdwhdw
hdwhdw requested changes Dec 26, 2024
View reviewed changes
Copy link
Contributor

@hdwhdw hdwhdw left a comment

Choose a reason for hiding this comment

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

Can you please look at ff73070 and see if this is something you can reuse?

@kishanps
Copy link

Can you please look at ff73070 and see if this is something you can reuse?

Thanks @hdwhdw for the reference. The reboot dbus service also needs a request/response framework which is what this PR does and IIUC @vvolam went with the other one as a stop gap solution.

Adding @github76543 (John) for additional inputs.

@hdwhdw
Copy link
Contributor

hdwhdw commented Dec 27, 2024

@kishanps thanks for clarifying. If so consider renaming the service to something more general than gnoi_reboot. Maybe 'async_system'? Having one module for each gnoi service can clutter the dbus codebase.

Also does it make sense to add your api to systemd service and call it async reboot, alongside @vvolam API?

@hdwhdw hdwhdw requested a review from vvolam December 27, 2024 07:14
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kishanps
Copy link

@kishanps thanks for clarifying. If so consider renaming the service to something more general than gnoi_reboot. Maybe 'async_system'? Having one module for each gnoi service can clutter the dbus codebase.

Also does it make sense to add your api to systemd service and call it async reboot, alongside @vvolam API?

@hdwhdw @github76543 @rkavitha-hcl @jaanah-hcl

I discussed with John, Dawei Huang & @vvolam and we all agree that reboot will be a separate dbus service and hence rename gnoi_reboot to just reboot. And remove the commit id ff73070 alongwith this PR to avoid the duplication.

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Pull request contains merge conflicts.

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hdwhdw hdwhdw mentioned this pull request Jan 31, 2025
Draft
11 tasks
kishanps
kishanps reviewed Feb 3, 2025
View reviewed changes
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rkavitha-hcl rkavitha-hcl mentioned this pull request Feb 7, 2025
Open
vvolam
vvolam reviewed Feb 7, 2025
View reviewed changes
@mssonicbld
Copy link

/azp run

@rkavitha-hcl rkavitha-hcl reopened this Feb 27, 2025
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rkavitha-hcl
Copy link
Contributor Author

@kishanps @rkavitha-hcl Could you fix build failures as well?

Build is fixed and branch is rebased

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vvolam
vvolam reviewed Feb 28, 2025
View reviewed changes
Copy link
Contributor

@vvolam vvolam left a comment

Choose a reason for hiding this comment

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

Other than those minor changes, remaining LGTM. Thank you.

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vvolam
vvolam approved these changes Mar 3, 2025
View reviewed changes
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hdwhdw hdwhdw merged commit 84c7d72 into sonic-net:master Mar 3, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kishanps kishanps kishanps left review comments

@bibhuprasad-hcl bibhuprasad-hcl bibhuprasad-hcl left review comments

@hdwhdw hdwhdw hdwhdw approved these changes

@vvolam vvolam vvolam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rkavitha-hcl @kishanps @hdwhdw @mssonicbld @bibhuprasad-hcl @vvolam