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.

Sign up for GitHub

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

Publish events for select operation and getresponse failure #1142

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Publish events for select operation and getresponse failure #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4ac150f
Add events
zbud-msft Oct 28, 2022
3f9993d
Fix tag
zbud-msft Oct 31, 2022
bf3b16e
Fix compile errors
zbud-msft Oct 31, 2022
6930f14
Fix compile errors
zbud-msft Oct 31, 2022
e710bf0
Remove white space
zbud-msft Oct 31, 2022
f594359
Remove all whitespaces from eol
zbud-msft Oct 31, 2022
6770dfa
Remove redundant event publish
zbud-msft Nov 1, 2022
4d80abf
Add UT for code coverage
zbud-msft Nov 3, 2022
e51175f
Fix compile error
zbud-msft Nov 3, 2022
f1c9400
Fix UT
zbud-msft Nov 3, 2022
ecd6c2d
Add fix for UT
zbud-msft Nov 3, 2022
0d9e2c3
Fix build issues
zbud-msft Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/RedisChannel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@

#include "swss/logger.h"
#include "swss/select.h"
#include "swss/events.h"

using namespace sairedis;

event_handle_t g_events_handle;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide use a global event publisher, then this should not be init/de-init in class level.
Because this is a global variable, but in dtor of RedisChannel the global variable will be deleted. Then when one RedisChannel instance deleted, all other instance will break when send event.

Please check the source code of deinit:
https://github.com/sonic-net/sonic-swss-common/blob/bcf48b26361f94e10a0eafc2c49c0bf0f440b2d5/common/events.cpp

Also if we decide use a global publisher, we need improve the event message, so receiver can identify which instance send the message.

And if every RedisChannel instance need have it's own event publisher, this should move to class member.

Because the thread safe issue I suggest we not use a global publisher, however it's depends on the requirement.

Copy link
Contributor

@liuh-80 liuh-80 Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

According to offline sync, I suggest not necessary use global or class level event publisher, we can just use a publisher on stack to save resource, because we only need send failed event.

However even we using a publisher on stack, the thread safe issue still need fix, best solution is to add lock in events API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

please dont use global objects


RedisChannel::RedisChannel(
_In_ const std::string& dbAsic,
_In_ Channel::Callback callback):
Expand All @@ -32,6 +35,9 @@ RedisChannel::RedisChannel(
SWSS_LOG_NOTICE("creating notification thread");

m_notificationThread = std::make_shared<std::thread>(&RedisChannel::notificationThreadFunction, this);

g_events_handle = events_init_publisher("sonic-events-swss");
Copy link
Contributor

Choose a reason for hiding this comment

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

Channel will be use by multi-thread, however the implementation of events_init_publisher()/events_deinit_publisher() are no thread safe, because it access a std::map:

https://github.com/sonic-net/sonic-swss-common/blob/bcf48b26361f94e10a0eafc2c49c0bf0f440b2d5/common/events.cpp#L28

so suggest improve the events.cpp code first before this PR merge.
Or you can add lock here.


}

RedisChannel::~RedisChannel()
Expand All @@ -48,6 +54,8 @@ RedisChannel::~RedisChannel()
m_notificationThread->join();

SWSS_LOG_NOTICE("join ntf thread end");

events_deinit_publisher(g_events_handle);
}

std::shared_ptr<swss::DBConnector> RedisChannel::getDbConnector() const
Expand Down Expand Up @@ -177,6 +185,13 @@ sai_status_t RedisChannel::wait(
}

SWSS_LOG_ERROR("SELECT operation result: %s on %s", swss::Select::resultToString(result).c_str(), command.c_str());

m_event_params = {
{ "operation_result", swss::Select::resultToString(result) },
{ "command", command }};

event_publish(g_events_handle, "select-operation-failure", &m_event_params);
Copy link
Contributor

Choose a reason for hiding this comment

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

As my understand the requirement want to send this event to subscriptor, so e_event_params not necessary to be a data member.

Also UT should receive event send here and check, not necessary to check parameters.

And if we decide use a global publisher, zmq send is not thread safe, we need add lock here or improve the event code first:
https://github.com/sonic-net/sonic-swss-common/blob/bcf48b26361f94e10a0eafc2c49c0bf0f440b2d5/common/events.cpp#L150


break;
}

Expand Down
3 changes: 3 additions & 0 deletions lib/RedisChannel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "swss/consumertable.h"
#include "swss/notificationconsumer.h"
#include "swss/selectableevent.h"
#include "swss/events.h"

#include <memory>
#include <functional>
Expand Down Expand Up @@ -52,6 +53,8 @@ namespace sairedis
_In_ const std::string& command,
_Out_ swss::KeyOpFieldsValuesTuple& kco) override;

event_params_t m_event_params;

protected:

virtual void notificationThreadFunction() override;
Expand Down
15 changes: 15 additions & 0 deletions unittest/lib/TestRedisChannel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,18 @@ TEST(RedisChannel, flush)

rc.flush();
}

TEST(RedisChannel, wait)
{
RedisChannel rc("ASIC_DB", callback);

rc.setResponseTimeout(60);

swss::KeyOpFieldsValuesTuple kco;

EXPECT_EQ(rc.wait("notify", kco), SAI_STATUS_FAILURE);

EXPECT_EQ(rc.m_event_params.size(), 2);
EXPECT_NE(rc.m_event_params["operation_result"], "");
EXPECT_NE(rc.m_event_params["command"], "");
}