gNOI System APIs (Reboot, RebootStatus, CancelReboot) changes#308
gNOI System APIs (Reboot, RebootStatus, CancelReboot) changes#308qiluo-msft merged 33 commits intosonic-net:masterfrom
Conversation
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hdwhdw
left a comment
hdwhdw
left a comment
There was a problem hiding this comment.
No major objections. Just some nits.
|
/azp run |
|
Pull request contains merge conflicts. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@ndas7 could you please merge latest repo changes for easy review? Thank you! |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
vvolam
left a comment
vvolam
left a comment
There was a problem hiding this comment.
Other than moving test file to test dir, rest looks good to me.
|
@ndas7 Could you please resolve conflicts? |
There was a problem hiding this comment.
Copilot reviewed 6 out of 14 changed files in this pull request and generated no comments.
Files not reviewed (8)
- go.mod: Language not supported
- gnmi_server/gnoi.go: Evaluated as low risk
- gnmi_server/server_test.go: Evaluated as low risk
- sonic_service_client/dbus_client.go: Evaluated as low risk
- gnmi_server/server.go: Evaluated as low risk
- common_utils/context.go: Evaluated as low risk
- test/test_gnoi.py: Evaluated as low risk
- swsscommon/empty.go: Evaluated as low risk
Comments suppressed due to low confidence (9)
common_utils/notification_producer_test.go:11
- [nitpick] The test name should be more descriptive. Suggest changing to: func TestNotificationProducer_Send_SucceedsWithEmptyOp(t *testing.T) {
func TestNotificationProducerSucceedsWithEmptyOp(t *testing.T) {
common_utils/notification_producer_test.go:19
- [nitpick] The test name should be more descriptive. Suggest changing to: func TestNotificationProducer_Send_SucceedsWithEmptyData(t *testing.T) {
func TestNotificationProducerSucceedsWithEmptyData(t *testing.T) {
common_utils/notification_producer_test.go:27
- [nitpick] The test name should be more descriptive. Suggest changing to: func TestNotificationProducer_Send_SucceedsWithEmptyOpAndData(t *testing.T) {
func TestNotificationProducerSucceedsWithEmptyOpAndData(t *testing.T) {
common_utils/notification_producer_test.go:35
- [nitpick] The test name should be more descriptive. Suggest changing to: func TestNotificationProducer_Send_SucceedsWithEmptyKeyValues(t *testing.T) {
func TestNotificationProducerSucceedsWithEmptyKeyValues(t *testing.T) {
common_utils/notification_producer_test.go:43
- [nitpick] The test name should be more descriptive. Suggest changing to: func TestNotificationProducer_Send_Succeeds(t *testing.T) {
func TestNotificationProducerSucceeds(t *testing.T) {
gnmi_server/gnoi_system.go:32
- [nitpick] The function name 'ValidateRebootRequest' should be unexported and follow Go's naming convention for unexported functions. Consider renaming it to 'validateRebootRequest'.
func ValidateRebootRequest(req *syspb.RebootRequest) error {
gnmi_server/gnoi_system.go:36
- [nitpick] The error message 'Invalid request: reboot method is not supported.' could be more specific about which methods are supported. Consider updating it to 'Invalid request: supported reboot methods are COLD, POWERDOWN, HALT, WARM, and NSF.'
log.Error("Invalid request: reboot method is not supported.")
gnmi_server/gnoi_system.go:131
- Type assertion without checking if it is successful can lead to a runtime panic. Consider using a type switch or checking if the assertion is successful.
req = req.(*syspb.RebootRequest)
gnmi_server/gnoi_system.go:214
- Ensure that the new behavior introduced by 'sendRebootReqOnNotifCh' is adequately covered by tests.
resp, err, _ := sendRebootReqOnNotifCh(ctx, req, rclient, rebootKey)
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
vvolam
left a comment
vvolam
left a comment
There was a problem hiding this comment.
Looks good to me, other than Qi's comment for a future change.
|
@ndas7 could you fix the build failures? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
It's fixed now, could you please take a look and approve? Thanks! |
|
@qiluo-msft Can you pls take the final look and merge the PR |
| return "DBUS restart service" | ||
| case DBUS_FILE_STAT: | ||
| return "DBUS file stat" | ||
| case DBUS_HALT_SYSTEM: |
There was a problem hiding this comment.
@vvolam your code is deleted. Could you double check?
There was a problem hiding this comment.
Hi Qi, the DBUS handling for HALT method was removed since HALT will now use the Reboot Notification Channel similar to the other Reboot Methods. Hence, this code was removed through Vasundhara's commit https://github.com/ndas7/sonic-gnmi/pull/1/files merged into this PR
Why I did it
Add support for gNOI System APIs for Reboot, RebootStatus, CancelReboot to forward requests to the Reboot Backend through Redis DB Notification channel.
How I did it
Created separate files for the System APIs under gnoi_system.go
How to verify it
Unit tests and build infrastructure checks
Which release branch to backport (provide reason below if selected)
Description for the changelog
Add support for gNOI System APIs for Reboot, RebootStatus, CancelReboot
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)