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 Warm Reboot - Added tests #20801

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

Conversation

rkavitha-hcl
Copy link

@rkavitha-hcl rkavitha-hcl commented Nov 14, 2024

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

Added test cases for warm reboot.

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 gnoi_wr_PR2 branch 2 times, most recently from b5f6845 to 9dd208a Compare November 15, 2024 12:49
@rkavitha-hcl rkavitha-hcl force-pushed the gnoi_wr_PR2 branch 3 times, most recently from 165ac73 to 2bfdab7 Compare November 26, 2024 11:22
@rkavitha-hcl rkavitha-hcl force-pushed the gnoi_wr_PR2 branch 6 times, most recently from 4194e72 to c3b6888 Compare December 4, 2024 07:39
@rkavitha-hcl rkavitha-hcl force-pushed the gnoi_wr_PR2 branch 2 times, most recently from c11590c to ad96646 Compare December 5, 2024 10:44
@kishanps
Copy link

kishanps commented Dec 6, 2024

@github76543 Joh, can you PTAL and signoff.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Pull request contains merge conflicts.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Pull request contains merge conflicts.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rkavitha-hcl rkavitha-hcl force-pushed the gnoi_wr_PR2 branch 2 times, most recently from 0918d69 to b7f84ed Compare January 27, 2025 10:37
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Pull request contains merge conflicts.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Pull request contains merge conflicts.

@rkavitha-hcl
Copy link
Author

Test results:

~/reboot_feature/sonic-buildimage/src/sonic-framework/tests$ ./tests
[==========] Running 41 tests from 4 test suites.
[----------] Global test environment set-up.
[----------] 2 tests from RebootBETest
[ RUN      ] RebootBETest.WarmbootInProgressBlocksNewWarmboot
NOTICE:- Start: --- Starting rebootbackend ---
.
.
.
[       OK ] TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest.TestWarmFailureFollowedByColdBoot/1 (5103 ms)
[----------] 26 tests from TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest (63060 ms total)

[----------] Global test environment tear-down
[==========] 41 tests from 4 test suites ran. (68474 ms total)
[  PASSED  ] 41 tests.
~/reboot_feature/sonic-buildimage/src/sonic-framework/tests$ ./tests_asan 
[==========] Running 41 tests from 4 test suites.
[----------] Global test environment set-up.
[----------] 2 tests from RebootBETest
[ RUN      ] RebootBETest.WarmbootInProgressBlocksNewWarmboot
.
.
.
[       OK ] TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest.TestWarmFailureFollowedByColdBoot/1 (5105 ms)
[----------] 26 tests from TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest (63103 ms total)

[----------] Global test environment tear-down
[==========] 41 tests from 4 test suites ran. (68527 ms total)
[  PASSED  ] 41 tests.
~/reboot_feature/sonic-buildimage/src/sonic-framework/tests$ ./tests_tsan 
[==========] Running 41 tests from 4 test suites.
[----------] Global test environment set-up.
[----------] 2 tests from RebootBETest
[ RUN      ] RebootBETest.WarmbootInProgressBlocksNewWarmboot
NOTICE:- Start: --- Starting rebootbackend ---
.
.
.
[       OK ] TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest.TestWarmFailureFollowedByColdBoot/1 (5105 ms)
[----------] 26 tests from TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest (63119 ms total)

[----------] Global test environment tear-down
[==========] 41 tests from 4 test suites ran. (68746 ms total)
[  PASSED  ] 41 tests.
~/reboot_feature/sonic-buildimage/src/sonic-framework/tests$ ./tests_usan 
[==========] Running 41 tests from 4 test suites.
[----------] Global test environment set-up.
[----------] 2 tests from RebootBETest
[ RUN      ] RebootBETest.WarmbootInProgressBlocksNewWarmboot
.
.
.
[       OK ] TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest.TestWarmFailureFollowedByColdBoot/1 (5103 ms)
[----------] 26 tests from TestWithStartupWarmbootEnabledState/RebootBEAutoStartTest (63062 ms total)

[----------] Global test environment tear-down
[==========] 41 tests from 4 test suites ran. (68473 ms total)
[  PASSED  ] 41 tests.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

#!/usr/bin/env bash


mkdir -p /var/sonic
Copy link
Collaborator

Choose a reason for hiding this comment

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

mkdir

The directory and file is static. Do you want to create them during build Dockerfile?

libperl5.32==5.32.1-4+deb11u3
libpgm-5.3-0==5.3.128~dfsg-2
libprocps8==2:3.3.17-5
libprotobuf-dev==3.21.12-3
Copy link
Collaborator

Choose a reason for hiding this comment

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

libprotobuf-dev

Do you really need a -dev package inside this container?

@qiluo-msft qiluo-msft requested a review from wen587 February 18, 2025 21:26
@@ -63,6 +63,7 @@
},
{%- set features = [("bgp", "{% if not DEVICE_RUNTIME_METADATA['ETHERNET_PORTS_PRESENT'] or ('CHASSIS_METADATA' in DEVICE_RUNTIME_METADATA and DEVICE_RUNTIME_METADATA['CHASSIS_METADATA']['module_type'] in ['supervisor']) %}disabled{% else %}enabled{% endif %}", false, "enabled"),
("database", "always_enabled", false, "always_enabled"),
("framework", "enabled", false, "enabled"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

false

We now rely on /etc/sonic/config_db.json to provide golden config, and minimize hard-coded config inside /etc/sonic/init_cfg.json. Is it possible not to change this file?

@wen587 to review.

@@ -180,6 +180,8 @@ supervisorctl start tunnelmgrd

supervisorctl start fabricmgrd

supervisorctl start rebootbackend
Copy link
Collaborator

Choose a reason for hiding this comment

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

rebootbackend

I could not find any code relating this program. And the name is weird.

@qiluo-msft qiluo-msft requested a review from hdwhdw February 18, 2025 23:32
[submodule "src/sonic-framework/gnoi"]
path = src/sonic-framework/gnoi
url = https://github.com/openconfig/gnoi
Copy link
Collaborator

Choose a reason for hiding this comment

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

url = https://github.com/openconfig/gnoi

@hdwhdw to review this new dependency

Copy link

Choose a reason for hiding this comment

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

I don't have an issue with this for now. But this creates a potential issue of the gnoi used by sonic-framework go out of sync with the gnoi used by sonic-gnmi. There isn't a good short-term solution, but we should have some issue tracking it and eventually use the same gnoi across sonic, especially given it is not so easy to update this dependency in sonic-gnmi.

[submodule "src/sonic-framework/gnoi"]
path = src/sonic-framework/gnoi
url = https://github.com/openconfig/gnoi
Copy link

Choose a reason for hiding this comment

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

I don't have an issue with this for now. But this creates a potential issue of the gnoi used by sonic-framework go out of sync with the gnoi used by sonic-gnmi. There isn't a good short-term solution, but we should have some issue tracking it and eventually use the same gnoi across sonic, especially given it is not so easy to update this dependency in sonic-gnmi.

/* Reboot is a request to the reboot sonic host service to request a reboot
from the platform. This takes as an argument a string based json formatted
Reboot request from
system.proto.’https://github.com/openconfig/gnoi/blob/73a1e7675c5f963e7810bd3828203f2758eb47e8/system/system.proto#L107
Copy link

Choose a reason for hiding this comment

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

The link can easily go stale, suggest simply using https://github.com/openconfig/gnoi/blob/main/system/system.proto

@@ -0,0 +1,19 @@
# framework package

FRAMEWORK = framework_1.0.0_$(CONFIGURED_ARCH).deb
Copy link
Collaborator

Choose a reason for hiding this comment

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

framework

framework is a confusing name, according to HLD, you want to name it as 'WarmbootManager' ?

test_main.cpp

tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_COVERAGE) $(CFLAGS_SAI)
tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_COVERAGE) $(CFLAGS_SAI)
Copy link
Collaborator

Choose a reason for hiding this comment

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

COVERAGE

Could you share the coverage data? even better, you can refine build pipeline to expose the coverage data.

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.

5 participants