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

gNOI Cold Reboot #20429

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rkavitha-hcl
Copy link

Why I did it

Work item tracking
  • Microsoft ADO (number only):

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@rkavitha-hcl rkavitha-hcl force-pushed the branch_cold_reboot1 branch 26 times, most recently from bca09c3 to 35ec5fd Compare October 8, 2024 10:27
@rkavitha-hcl rkavitha-hcl force-pushed the branch_cold_reboot1 branch 7 times, most recently from 3c797ec to b99f545 Compare October 14, 2024 04:52
@kishanps
Copy link

@vvolam
@rkavitha-hcl is working on fixing the test failures but otherwise the PR has passed the presubmit checks and ready for integration and review.
Adding @github76543 @qiluo-msft & @hdwhdw as well for review.

@vvolam vvolam requested a review from hdwhdw October 18, 2024 18:22
@hdwhdw
Copy link

hdwhdw commented Oct 18, 2024

Honest question: What does this PR really do? It contains 143 files. Is it just too large for be in a single PR? What if we somehow want to roll it back?

@rkavitha-hcl rkavitha-hcl force-pushed the branch_cold_reboot1 branch 2 times, most recently from 03475cb to 50c141e Compare October 21, 2024 06:50
@kishanps
Copy link

/azpw run

@kishanps
Copy link

/AzurePipelines run

Copy link

Commenter does not have sufficient privileges for PR 20429 in repo sonic-net/sonic-buildimage

@kishanps
Copy link

@vvolam Can you trigger the pipeline (build & test) to run, seems like I don't have permissions to trigger them ?

@mint570
Copy link
Contributor

mint570 commented Oct 21, 2024

/AzurePipelines run

Copy link

Commenter does not have sufficient privileges for PR 20429 in repo sonic-net/sonic-buildimage

@vvolam
Copy link
Contributor

vvolam commented Oct 22, 2024

/azpw run

@kishanps
Copy link

Honest question: What does this PR really do? It contains 143 files. Is it just too large for be in a single PR? What if we somehow want to roll it back?

@hdwhdw Fair question, let me add some high level details here that might help the review. Although there are 143 files, the major changes are related to adding the new framework docker.

  1. New changes to add framework docker to the system
  2. Inside the framework docker
  • rebootbackend process listens for notifications on redis db (gNOI frontend sends notification on any of the reboot set operation) and invokes sonic-host-services for the corresponding method
  • Add openconfig gNOI sub-module

image

@kishanps
Copy link

@wangxin Is the pipeline failure in this PR expected given your upcoming changes in PR #20567 ?

@yutongzhang-microsoft
Copy link
Contributor

@wangxin Is the pipeline failure in this PR expected given your upcoming changes in PR #20567 ?

Yes, it is expected.

@rkavitha-hcl rkavitha-hcl changed the title GNOI Cold Reboot gNOI Cold Reboot Oct 23, 2024
Copy link

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

@kishanps Thanks for the breakdown! Now it makes much more sense.

I can see many of these changes can be in its independent PR, such as one for just introducing an GNOI definition as a submodule (in fact this can benefit a lot more than just the reboot). If those can be in their separate PR we can get them in quickly.

@kishanps
Copy link

@kishanps Thanks for the breakdown! Now it makes much more sense.

I can see many of these changes can be in its independent PR, such as one for just introducing an GNOI definition as a submodule (in fact this can benefit a lot more than just the reboot). If those can be in their separate PR we can get them in quickly.

Here are the breakdown PR's

Adding @vvolam, @qiluo-msft for viz.

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.

6 participants