Skip to content

Conversation

@bhaveshdell
Copy link
Contributor

@bhaveshdell bhaveshdell commented May 14, 2025

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.

This PR was originally reviewed and merged.
#17949

However, due to build error post-merge the PR was reverted.
#20052

The possible fix for build error has been fixed as per comments in PR
#20064

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bhaveshdell
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

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

@bhaveshdell
Copy link
Contributor Author

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

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

@prsunny
Copy link
Contributor

prsunny commented May 20, 2025

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Contributor

prsunny commented May 20, 2025

/AzurePipelines run Azure.sonic-buildimage

@bhaveshdell , you can try with /azpw run Azure.sonic-buildimage

@venkatmahalingam
Copy link
Collaborator

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

Only PR owner can use /azpw run

@bhaveshdell
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zhangyanzhao
Copy link

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

Only PR owner can use /azpw run

@qiluo-msft qiluo-msft requested a review from Copilot June 5, 2025 17:42
@qiluo-msft
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the existing Event Framework by introducing alarm management via a new YANG model and by adding a standalone eventdb service alongside adjustments to eventd. Key changes include:

  • Adding sonic-alarm.yang to define alarm state and statistics.
  • Introducing eventdb daemon: source code, build/test integration, Debian install, and supervisor configuration.
  • Updating build scripts, test configs, and Docker templates to support both eventd and eventdb.

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/sonic-yang-models/yang-models/sonic-alarm.yang New YANG module for alarm operational data
src/sonic-eventd/var/evprofile/default.json Empty default event profile
src/sonic-eventd/tests/subdir.mk Added eventdb_ut dependency
src/sonic-eventd/tests/eventdb_database_config_*.json Test database config for eventdb
src/sonic-eventd/tests/eventd.json eventd JSON config for history limits
src/sonic-eventd/src/subdir.mk Added eventdb build/test object lists
src/sonic-eventd/src/loghandler.{h,cpp} Syslog helper API for events/alarm logging
src/sonic-eventd/src/eventutils.{h,cpp} Parsing utilities for JSON configs and event profiles
src/sonic-eventd/src/eventdb.cpp Entry point for eventdb, signal handling
src/sonic-eventd/src/eventconsume.{h,cpp} Core logic for consuming events and updating stats
src/sonic-eventd/etc/eventd.json Default eventd config
src/sonic-eventd/debian/sonic-eventd.install Install rules for eventdb and its configs
src/sonic-eventd/Makefile New eventdb targets and test targets
files/build_templates/docker_image_ctl.j2 Cold-boot Redis load logic
dockers/docker-eventd/supervisord.conf Added eventdb supervisord program
dockers/docker-database/database_config.json.j2 Added EVENT_DB mapping
Comments suppressed due to low confidence (2)

src/sonic-yang-models/yang-models/sonic-alarm.yang:96

  • The id leaf in ALARM_STATS_LIST uses an enumeration with only a single member state and no explicit value; this is likely incomplete. Define all intended enum values with associated numeric values or revisit the choice of type.
type enumeration {

files/build_templates/docker_image_ctl.j2:103

  • Typo in comment: change emtpy to empty.
# Create an emtpy file and overwrite any RDB if already there

@@ -0,0 +1,5 @@
#include <iostream>
extern "C" void openSyslog();
extern "C" void writeToSyslog(std::string ev_id, int ev_sev, std::string ev_type, std::string ev_act, std::string ev_msg, std::string ev_static_msg);
Copy link

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

Using std::string in extern "C" declarations breaks the C ABI and can lead to undefined behavior. Change signatures to use C-compatible types (e.g., const char*).

Suggested change
extern "C" void writeToSyslog(std::string ev_id, int ev_sev, std::string ev_type, std::string ev_act, std::string ev_msg, std::string ev_static_msg);
extern "C" void writeToSyslog(const char* ev_id, int ev_sev, const char* ev_type, const char* ev_act, const char* ev_msg, const char* ev_static_msg);
``` #Closed

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is valid comment.

extern "C" void writeToSyslog(std::string ev_id, int ev_sev, std::string ev_type, std::string ev_act, std::string ev_msg, std::string ev_static_msg) {
int SYSLOG_FACILITY = LOG_LOCAL4;
if (ev_act.empty()) {
const char LOG_FORMAT[] = "[%s], %%%s: %s %s";
Copy link

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

The format string uses %%%s, which will emit a literal % before the %s. It should likely be %s instead of %%%s to correctly interpolate the event name.

Suggested change
const char LOG_FORMAT[] = "[%s], %%%s: %s %s";
const char LOG_FORMAT[] = "[%s], %s: %s %s";

Copilot uses AI. Check for mistakes.
Comment on lines 7 to 22
using namespace std;

const string EVENT_SEVERITY_CRITICAL_STR = "CRITICAL";
const string EVENT_SEVERITY_MAJOR_STR = "MAJOR";
const string EVENT_SEVERITY_MINOR_STR = "MINOR";
const string EVENT_SEVERITY_WARNING_STR = "WARNING";
const string EVENT_SEVERITY_INFORMATIONAL_STR = "INFORMATIONAL";

const string EVENT_ENABLE_TRUE_STR = "true";
const string EVENT_ENABLE_FALSE_STR = "false";

const string EVENT_ACTION_RAISE_STR = "RAISE";
const string EVENT_ACTION_CLEAR_STR = "CLEAR";
const string EVENT_ACTION_ACK_STR = "ACKNOWLEDGE";
const string EVENT_ACTION_UNACK_STR = "UNACKNOWLEDGE";

Copy link

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Avoid using namespace std; in a header file to prevent namespace pollution; qualify standard types explicitly.

Suggested change
using namespace std;
const string EVENT_SEVERITY_CRITICAL_STR = "CRITICAL";
const string EVENT_SEVERITY_MAJOR_STR = "MAJOR";
const string EVENT_SEVERITY_MINOR_STR = "MINOR";
const string EVENT_SEVERITY_WARNING_STR = "WARNING";
const string EVENT_SEVERITY_INFORMATIONAL_STR = "INFORMATIONAL";
const string EVENT_ENABLE_TRUE_STR = "true";
const string EVENT_ENABLE_FALSE_STR = "false";
const string EVENT_ACTION_RAISE_STR = "RAISE";
const string EVENT_ACTION_CLEAR_STR = "CLEAR";
const string EVENT_ACTION_ACK_STR = "ACKNOWLEDGE";
const string EVENT_ACTION_UNACK_STR = "UNACKNOWLEDGE";
const std::string EVENT_SEVERITY_CRITICAL_STR = "CRITICAL";
const std::string EVENT_SEVERITY_MAJOR_STR = "MAJOR";
const std::string EVENT_SEVERITY_MINOR_STR = "MINOR";
const std::string EVENT_SEVERITY_WARNING_STR = "WARNING";
const std::string EVENT_SEVERITY_INFORMATIONAL_STR = "INFORMATIONAL";
const std::string EVENT_ENABLE_TRUE_STR = "true";
const std::string EVENT_ENABLE_FALSE_STR = "false";
const std::string EVENT_ACTION_RAISE_STR = "RAISE";
const std::string EVENT_ACTION_CLEAR_STR = "CLEAR";
const std::string EVENT_ACTION_ACK_STR = "ACKNOWLEDGE";
const std::string EVENT_ACTION_UNACK_STR = "UNACKNOWLEDGE";
``` #Closed

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a valid comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will address the comments

Comment on lines 4 to 10
static EventConsume *evtd_instance = NULL;

void signalHandler(const int signal) {
SWSS_LOG_NOTICE("in signalHandler");

if (signal == SIGINT) {
evtd_instance->read_eventd_config();
Copy link

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

Calling non-async-signal-safe functions inside a signal handler is unsafe and can cause undefined behavior. Consider deferring configuration reload to the main loop or using a flag checked outside the handler.

Suggested change
static EventConsume *evtd_instance = NULL;
void signalHandler(const int signal) {
SWSS_LOG_NOTICE("in signalHandler");
if (signal == SIGINT) {
evtd_instance->read_eventd_config();
#include <atomic>
static EventConsume *evtd_instance = NULL;
static std::atomic<bool> reload_config_flag(false);
void signalHandler(const int signal) {
SWSS_LOG_NOTICE("in signalHandler");
if (signal == SIGINT) {
reload_config_flag.store(true);

Copilot uses AI. Check for mistakes.
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bhaveshdell
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@venkatmahalingam
Copy link
Collaborator

@qiluo-msft Are we good with the new changes?

@venkatmahalingam
Copy link
Collaborator

Please help to re-review the changes and approve.

@adyeung @Praveen-Brcm @leeprecy

Note: This PR was originally reviewed by BRCM.
#17949


json j;
try {
j = json::parse(ifs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  [](http://example.com/codeflow?start=0&length=6)

Could you use 4x space indentation?

try {
j = json::parse(ifs);
}
catch (const std::exception &e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exception

Could you use more specific exception class?

try {
file >> j;
}
catch (const std::exception &e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exception

Could you use more specific exception class?

syslog(LOG_MAKEPRI(ev_sev, SYSLOG_FACILITY), LOG_FORMAT,
safe(ev_type), safe(ev_id), safe(ev_src), safe(ev_static_msg), safe(ev_msg));
} else {
const char LOG_FORMAT[] = "[%s] (%s), %%%s: %%%%%s: %s %s";
Copy link
Collaborator

Choose a reason for hiding this comment

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

%%%%%

why so many % ?

Copy link
Contributor Author

@bhaveshdell bhaveshdell Nov 6, 2025

Choose a reason for hiding this comment

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

Here's the sample log, the % is to escape.
The %% is applied for help with parsing.

syslog:Nov 06 21:04:38.345373+00:00 2025 sonic WARNING eventd#eventd[25]: [ALARM] (RAISE), %PSU_POWER_STATUS: %%PSU 1: Psu Power Status PSU 1 is out of power.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

remove ack, noack from sonic-common-event yang.
Set eventdb testcases as separate test binary.
Skip testcase execution if DB connection failure.
Set eventdb process startretries= 0 and expected exit code=0.
This is since if no eventi types are defined in the event_profile, the process can exit.
Avoid using namespace std; in a header file to prevent namespace pollution; qualify standard types explicitly.
Avoid non-async-signal-safe function inside signal handler.
Script starts eventdb only if atleast one event is defined in the profile.
Add validation around json operations.
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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