Skip to content

Populate CHASSIS_APP_DB when flex counters are read #1378

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

Closed
viveksrao-arista wants to merge 6 commits into from
Closed

Populate CHASSIS_APP_DB when flex counters are read #1378

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a810271
Populate CHASSIS_APP_DB when flex counters are read
viveksrao-arista Apr 19, 2024
1e5e680
Merge branch 'sonic-net:master' into master-populate-chassis-app-db-o…
viveksrao-arista Jun 1, 2024
73368cc
Remove global variables. Use VendorSai object to query queue attribut…
viveksrao-arista Jun 1, 2024
aeb2bcd
Remove global variables. Use VendorSai object to query queue attribut…
viveksrao-arista Jun 1, 2024
62cc42c
Merge branch 'sonic-net:master' into master-populate-chassis-app-db-o…
vivekverma-arista Aug 11, 2024
56605d0
Merge branch 'master' into master-populate-chassis-app-db-on-flexc
vivekverma-arista Aug 17, 2024
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
137 changes: 132 additions & 5 deletions syncd/FlexCounter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ static const std::string ATTR_TYPE_PG = "Priority Group Attribute";
static const std::string ATTR_TYPE_MACSEC_SA = "MACSEC SA Attribute";
static const std::string ATTR_TYPE_ACL_COUNTER = "ACL Counter Attribute";

// --> Needed for agg voq stats
static sai_queue_api_t *sai_queue_api = NULL;
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

no api should be uses, VendorCalss shoud be used

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista May 17, 2024

Choose a reason for hiding this comment

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

Sorry, I do not understand. I need a handle sai_queue_api so that I can query queue stats like this status = sai_queue_api->get_queue_attribute(rid, 1, &attr);. I am not aware of an alternative to this. What is VendorClass? How to use it? Can you point me to an example?

Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 17, 2024

Choose a reason for hiding this comment

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

use VendorSai class

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done.

static swss::DBConnector configDb("CONFIG_DB", 0);
static swss::Table cfgDeviceMetaDataTable(&configDb, CFG_DEVICE_METADATA_TABLE_NAME);
static swss::DBConnector counterDb("COUNTERS_DB", 0);
// <-- agg voq stats end

Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

please remove all global variables, all connectsion for db should come from parametrs to the class not had coded

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Fixed.

BaseCounterContext::BaseCounterContext(const std::string &name):
m_name(name)
{
Expand Down Expand Up @@ -363,6 +370,7 @@ void deserializeAttr(
sai_deserialize_acl_counter_attr(name, attr);
}


template <typename StatType>
class CounterContext : public BaseCounterContext
Comment on lines 367 to 368
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

this should be in separate file 1 class per one file cpp

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista May 17, 2024

Choose a reason for hiding this comment

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

This code is not added by me.

{
Expand Down Expand Up @@ -511,8 +519,72 @@ class CounterContext : public BaseCounterContext
}
}

void addVoqEntryToChassisAppDb(_In_ const sai_object_id_t rid,
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

first parameter should be also in separate line

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done.

_In_ const sai_object_id_t vid,
_In_ std::vector<swss::FieldValueTuple> &counterValues,
_Out_ swss::Table *appDbCountersTable ) {
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

{ to new line, plese forllow code quality

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done.

// We need to update CHASSIS_APP_DB only if it is a VOQ counter. So first check
// if the counter is a QUEUE counter and if yes, query the queue attribute to check
// if it is a VOQ queue or not.
sai_status_t status;

if (m_objectType == SAI_OBJECT_TYPE_QUEUE) {
sai_attribute_t attr;
attr.id = SAI_QUEUE_ATTR_TYPE;
status = sai_queue_api->get_queue_attribute(rid, 1, &attr);
if (status != SAI_STATUS_SUCCESS ) {
SWSS_LOG_ERROR( "sai get_queue_attribute failed for SAI_QUEUE_ATTR_TYPE. "
"status %d rid 0x%" PRIx64 "vid 0x%" PRIx64, status, rid, vid);
return;
}
if (attr.value.s32 == SAI_QUEUE_TYPE_UNICAST_VOQ) {
attr.id = SAI_QUEUE_ATTR_INDEX;
status = sai_queue_api->get_queue_attribute(rid, 1, &attr);
if (status != SAI_STATUS_SUCCESS ) {
SWSS_LOG_ERROR( "sai get_queue_attribute failed for SAI_QUEUE_ATTR_INDEX."
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

why there is space after ( ? remove

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista May 17, 2024

Choose a reason for hiding this comment

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

Well that's string concatenation and is used to keep the line within a reasonable length. Helps when editing code in terminals and editors that do not support line length of say for eg more than 85 characters or so. Good practice to make your code readable across various editors and terminals. Linux mandates a line length of 85 chars. Not sure what Sonic enforces. Good to follow none the less.

Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 17, 2024

Choose a reason for hiding this comment

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

please keep same style as rest of the file

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done

" status %d rid 0x%" PRIx64 "vid 0x%" PRIx64, status, rid, vid) ;
return;
}
auto iface = counterDb.hget(COUNTERS_PORT_NAME_VOQ_MAP, sai_serialize_object_id(vid));
if (!iface)
{
SWSS_LOG_ERROR("Cannot query port name for queue. rid 0x%" PRIx64 "vid 0x%" PRIx64, rid, vid);
return;
}
Comment on lines +548 to +552
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

add empty line before each if and after each }

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done

string hostname;
if (!cfgDeviceMetaDataTable.hget("localhost", "hostname", hostname))
{
// hostname is not configured.
SWSS_LOG_ERROR("Host name is not configured");
return;
}
string asic;
if (!cfgDeviceMetaDataTable.hget("localhost", "asic_name", asic))
{
// Asic name not configured
SWSS_LOG_ERROR("Asic name is not configured");
return;
}
// Form the key to be inserted in "COUNTERS" table in CHASSIS_APP_DB.
// The key is of the form <INTF>@<VOQ>.
// Like this <table>|fsi_id|asic_id|intf@fsi_id|asic_id:VOQ_index
// Interface part:
// iface: "fsi_id|asic_id|intf" - desribes the chip and core where the intf
// is present
//
// VOQ part:
// fsi_id: FSI id of the chip where the voq is present
// asic_id: asic id of the core where the voq is present
// VOQ_index: index of the voq among all the voqs for this interface on this chip and asic
string key = *iface + "@" + hostname + "|" + asic + ":" + std::to_string( attr.value.u8 );
appDbCountersTable->set(key, counterValues, "");
}
}
}

virtual void collectData(
_In_ swss::Table &countersTable) override
_In_ swss::Table &countersTable,
_In_ swss::Table *appDbCountersTable) override
{
SWSS_LOG_ENTER();
sai_stats_mode_t effective_stats_mode = m_groupStatsMode;
Expand Down Expand Up @@ -541,6 +613,11 @@ class CounterContext : public BaseCounterContext
values.emplace_back(serializeStat(statIds[i]), std::to_string(stats[i]));
}
countersTable.set(sai_serialize_object_id(vid), values, "");
if (appDbCountersTable)
{
// Chassis based system
addVoqEntryToChassisAppDb(rid, vid, values, appDbCountersTable);
}
}

for (const auto &kv : m_bulkContexts)
Expand Down Expand Up @@ -954,7 +1031,8 @@ class AttrContext : public CounterContext<AttrType>
}

void collectData(
_In_ swss::Table &countersTable) override
_In_ swss::Table &countersTable,
_In_ swss::Table *appDbCountersTable) override
{
SWSS_LOG_ENTER();

Expand Down Expand Up @@ -995,6 +1073,11 @@ class AttrContext : public CounterContext<AttrType>
values.emplace_back(meta->attridname, sai_serialize_attr_value(*meta, attrs[i]));
}
countersTable.set(sai_serialize_object_id(vid), values, "");
if (appDbCountersTable)
{
// Chassis based system
appDbCountersTable->set(sai_serialize_object_id(vid), values, "");
}
}
}
};
Expand All @@ -1014,6 +1097,14 @@ FlexCounter::FlexCounter(
m_enable = false;
m_isDiscarded = false;

if (!sai_queue_api) {
sai_status_t status;
status = sai_api_query(SAI_API_QUEUE, (void **)&sai_queue_api);
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

do not use gloal apis ! Vendor class should be used!

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done

if (status != SAI_STATUS_SUCCESS) {
SWSS_LOG_ERROR("SAI queue API query failed. Status %d", status);
}
}

Comment on lines +1134 to +1145
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik Jun 2, 2024

Choose a reason for hiding this comment

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

all databases names are hardcoded, they should be obrained from defines or config header

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 4, 2024

Choose a reason for hiding this comment

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

Same as above

startFlexCounterThread();
}

Expand Down Expand Up @@ -1358,16 +1449,18 @@ bool FlexCounter::hasCounterContext(
}

void FlexCounter::collectCounters(
_In_ swss::Table &countersTable)
_In_ swss::Table &countersTable,
_In_ swss::Table *appDbCountersTable)
{
SWSS_LOG_ENTER();

for (const auto &it : m_counterContext)
{
it.second->collectData(countersTable);
it.second->collectData(countersTable, appDbCountersTable);
}

countersTable.flush();
appDbCountersTable->flush();
}

void FlexCounter::runPlugins(
Expand All @@ -1388,6 +1481,30 @@ void FlexCounter::runPlugins(
}
}

void getCfgSwitchType(string &switch_type)
{
try
{
if (!cfgDeviceMetaDataTable.hget("localhost", "switch_type", switch_type))
{
//Switch type is not configured. Consider it default = "switch" (regular switch)
switch_type = "switch";
}
}
catch(const std::system_error& e)
{
SWSS_LOG_ERROR("System error: %s", e.what());
switch_type = "switch";
}

if (switch_type != "voq" && switch_type != "fabric" && switch_type != "chassis-packet" && switch_type != "switch" && switch_type != "dpu")
{
SWSS_LOG_ERROR("Invalid switch type %s configured", switch_type.c_str());
//If configured switch type is none of the supported, assume regular switch
switch_type = "switch";
}
}

void FlexCounter::flexCounterThreadRunFunction()
{
SWSS_LOG_ENTER();
Expand All @@ -1396,6 +1513,16 @@ void FlexCounter::flexCounterThreadRunFunction()
swss::RedisPipeline pipeline(&db);
swss::Table countersTable(&pipeline, COUNTERS_TABLE, true);

string switchType;
swss::Table *appDbCountersTable = NULL;
getCfgSwitchType(switchType);
if (switchType == "voq")
{
swss::DBConnector appDb("CHASSIS_APP_DB", 0, true);
swss::RedisPipeline appDbPipeline(&appDb);
appDbCountersTable = new swss::Table(&appDbPipeline, "COUNTERS", true);
Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 16, 2024

Choose a reason for hiding this comment

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

don't use new operator !

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista May 17, 2024

Choose a reason for hiding this comment

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

Why? I need a pointer and not a reference to an object here.

Copy link
Copy Markdown
Collaborator

@kcudnik kcudnik May 17, 2024

Choose a reason for hiding this comment

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

use shared_ptr

Copy link
Copy Markdown
Author

@viveksrao-arista viveksrao-arista Jun 1, 2024

Choose a reason for hiding this comment

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

Done

}

while (m_runFlexCounterThread)
{
MUTEX;
Expand All @@ -1404,7 +1531,7 @@ void FlexCounter::flexCounterThreadRunFunction()
{
auto start = std::chrono::steady_clock::now();

collectCounters(countersTable);
collectCounters(countersTable, appDbCountersTable);

runPlugins(db);

Expand Down
8 changes: 5 additions & 3 deletions syncd/FlexCounter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ extern "C" {

namespace syncd
{
class BaseCounterContext
class BaseCounterContext
{
public:
BaseCounterContext(const std::string &name);
Expand All @@ -38,7 +38,8 @@ namespace syncd
_In_ sai_object_id_t vid) = 0;

virtual void collectData(
_In_ swss::Table &countersTable) = 0;
_In_ swss::Table &countersTable,
_In_ swss::Table *appDbCountersTable) = 0;

virtual void runPlugin(
_In_ swss::DBConnector& counters_db,
Expand Down Expand Up @@ -122,7 +123,8 @@ namespace syncd
_In_ const std::string &name) const;

void collectCounters(
_In_ swss::Table &countersTable);
_In_ swss::Table &countersTable,
_In_ swss::Table *appDbCountersTable);

void runPlugins(
_In_ swss::DBConnector& db);
Expand Down