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

Event and alarm management #17949

Merged
merged 9 commits into from
Aug 23, 2024
Merged

Conversation

bhaveshdell
Copy link
Contributor

@bhaveshdell bhaveshdell commented Jan 30, 2024

Why I did it

This PR contains code changes for providing extension to the Event Framework as specified in the sonic-net/SONiC#1409

How I did it

Followed design specified in sonic-net/SONiC#1409.

How to verify it

Unit tests added.

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

remove ack, noack from sonic-common-event yang.
@@ -0,0 +1,134 @@
module sonic-event {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you add a code to call this event from one of the App to showcase the usage?

@@ -0,0 +1,54 @@
#ifndef __EVENTCONSUME_H__
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see the existing event infra and this new changes work together in the unit test.

For example, if there is a BGP event, how does it get saved in the EVENT_DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UT code added with this PR.

"id" : 19,
"separator": "|",
"instance" : "redis"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This a new redis DB instance addition. Have few queries:

  1. is there a max limit of redis DB instances that can be spawned and maintained on a given SONiC instance?
  2. If so, was it ensured that its within limits and no impact to the overall system bring-up and working in steady state?
  3. Is this a global database i.e. only a single instance of it would be spawned on the system in global namespace? or would it be one per ASIC/NPU namespace? Where is that determined/specified in this changeset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Currently the max limit is set at 100 logical instances. /etc/redis/redis.conf
  2. This is within the global database. (redis instance.)

@bhaveshdell
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bhaveshdell
Copy link
Contributor Author

/AzurePipelines run Azure.sonic-buildimage

Copy link

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

@bhaveshdell
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bhaveshdell
Copy link
Contributor Author

@bmridul @shyam77git @qiluo-msft Please review this PR.

@zhangyanzhao
Copy link
Collaborator

need more reviews

@jeff-yin jeff-yin requested a review from adyeung May 29, 2024 23:18
@zhangyanzhao
Copy link
Collaborator

@reviewers, can you please help to review this PR? Thanks.

@adyeung
Copy link
Collaborator

adyeung commented May 30, 2024

@leeprecy please help review

@venkatmahalingam
Copy link
Collaborator

@zhangyanzhao Can we merge this PR? or we need more approval?

@venkatmahalingam
Copy link
Collaborator

@lguohan @qiluo-msft @adyeung Can you review and merge the pull request?

@adyeung
Copy link
Collaborator

adyeung commented Aug 23, 2024

@Praveen-Brcm pls help review and merge

@Praveen-Brcm Praveen-Brcm merged commit 10f0fe8 into sonic-net:master Aug 23, 2024
20 checks passed
@liushilongbuaa
Copy link
Contributor

liushilongbuaa commented Aug 27, 2024

@bhaveshdell , PR validation is too old. It run on May. Now it breaks vs build. LINK

Building target: eventd-tests
Invoking: G++ Linker
g++ -Wl,-z,relro -Wl,-z,now -o tests/tests ./src/eventd.o ./src/eventconsume.o ./src/eventutils.o ./src/loghandler.o ./tests/eventd_ut.o  ./tests/main.o  -levent -lhiredis -lswsscommon -lpthread -lboost_thread -lboost_system -lzmq -lboost_serialization -luuid -llua5.1 -L/usr/src/gtest -lgtest -lgtest_main -lgmock -lgmock_main
g++ -Wl,-z,relro -Wl,-z,now -o tests/eventdb ./src/eventd.o ./src/eventconsume.o ./src/eventutils.o ./src/loghandler.o ./tests/eventdb_ut.o -levent -lhiredis -lswsscommon -lpthread -lboost_thread -lboost_system -lzmq -lboost_serialization -luuid -llua5.1 -L/usr/src/gtest -lgtest -lgtest_main -lgmock -lgmock_main
/usr/bin/ld: ./tests/eventdb_ut.o: in function `std::__new_allocator<std::__detail::_Hash_node_base*>::allocate(unsigned long, void const*)':
/usr/include/c++/12/bits/new_allocator.h:125: undefined reference to `std::__throw_bad_array_new_length()'
/usr/bin/ld: /usr/include/c++/12/bits/new_allocator.h:125: undefined reference to `std::__throw_bad_array_new_length()'
/usr/bin/ld: /usr/include/c++/12/bits/new_allocator.h:125: undefined reference to `std::__throw_bad_array_new_length()'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:64: eventd-tests] Error 1
make[2]: *** Waiting for unfinished jobs....
Finished building: rsyslog_plugin/main.cpp
 
Finished building: rsyslog_plugin/rsyslog_plugin.cpp
 
Finished building: rsyslog_plugin_tests/rsyslog_plugin_ut.cpp
 
make[2]: Leaving directory '/sonic/src/sonic-eventd'
dh_auto_build: error: make -j4 "INSTALL=install --strip-program=true" returned exit code 2
make[1]: *** [debian/rules:6: build] Error 25
make[1]: Leaving directory '/sonic/src/sonic-eventd'
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2
[  FAIL LOG END  ] [ target/debs/bullseye/sonic-eventd_1.0.0-0_amd64.deb ]

liushilongbuaa added a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Aug 28, 2024
@bhaveshdell
Copy link
Contributor Author

@liushilongbuaa How can I commit the fix for the failure? Is the PR reverted?

yxieca pushed a commit that referenced this pull request Aug 28, 2024
yxieca added a commit that referenced this pull request Aug 28, 2024
@yxieca
Copy link
Contributor

yxieca commented Aug 28, 2024

@bhaveshdell your PR has been reverted as @liushilongbuaa pointed out.

I created a reversion of reversion PR #20064 , I suggest let's watch this PR build and test. Once the issue surfaced again, you can add more changed to 20064 to address the issue. Or you can create another PR, with your original change, plus change addressing the issue.

liushilongbuaa added a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Aug 29, 2024
liushilongbuaa added a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Aug 30, 2024
liushilongbuaa added a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Aug 30, 2024
vvolam pushed a commit to vvolam/sonic-buildimage that referenced this pull request Sep 12, 2024
* Support for event persistence in redis-db.

* Updates

* Updates including fix to  eventdb in test enviroment.

* Add sonic yang to model event and alarm table.
remove ack, noack from sonic-common-event yang.

* Add event/alarm  persistence related testscases

* Remove file eventdb_ut.cpp.

* Updates to eventdb testsuite.

* Revert changes to existing eventd UT.
Set eventdb testcases as separate test binary.
Skip testcase execution if DB connection failure.

* Commit test related config files.
vvolam pushed a commit to vvolam/sonic-buildimage that referenced this pull request Sep 12, 2024
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.

10 participants