Populate CHASSIS_APP_DB when flex counters are read by viveksrao-arista · Pull Request #1378 · sonic-net/sonic-sairedis

viveksrao-arista · 2024-05-16T16:49:01Z

When flex counters are read, if the counter is for a VOQ, populate chassis app db for this VOQ so that on the supervisor it can be used to calculate aggregate stats across all VOQs for a port.

When flex counters are read, if the counter is for a VOQ, populate chassis app db.

kcudnik

fix coments

kcudnik · 2024-05-16T17:45:46Z

syncd/FlexCounter.cpp

+// --> Needed for agg voq stats
+static sai_queue_api_t *sai_queue_api = NULL;
+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
+


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

kcudnik · 2024-05-16T17:46:00Z

syncd/FlexCounter.cpp

 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;


no api should be uses, VendorCalss shoud be used

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?

use VendorSai class

kcudnik · 2024-05-16T17:46:18Z

syncd/FlexCounter.cpp

        }
    }

+    void addVoqEntryToChassisAppDb(_In_ const sai_object_id_t rid,


first parameter should be also in separate line

kcudnik · 2024-05-16T17:46:33Z

syncd/FlexCounter.cpp

+    void addVoqEntryToChassisAppDb(_In_ const sai_object_id_t rid,
+                                   _In_ const sai_object_id_t vid,
+                                   _In_ std::vector<swss::FieldValueTuple> &counterValues,
+                                   _Out_ swss::Table *appDbCountersTable ) {


{ to new line, plese forllow code quality

kcudnik · 2024-05-16T17:46:55Z

syncd/FlexCounter.cpp

+             if (!iface)
+             {
+                SWSS_LOG_ERROR("Cannot query port name for queue. rid 0x%" PRIx64 "vid 0x%" PRIx64, rid, vid);
+                return;
+             }


add empty line before each if and after each }

kcudnik · 2024-05-16T17:47:27Z

syncd/FlexCounter.cpp

 template <typename StatType>
 class CounterContext : public BaseCounterContext


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

This code is not added by me.

kcudnik · 2024-05-16T17:47:41Z

syncd/FlexCounter.cpp

+             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."


why there is space after ( ? remove

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.

please keep same style as rest of the file

kcudnik · 2024-05-16T17:48:24Z

syncd/FlexCounter.cpp


+    if (!sai_queue_api) {
+       sai_status_t status;
+       status = sai_api_query(SAI_API_QUEUE, (void **)&sai_queue_api);


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

kcudnik · 2024-05-16T17:52:17Z

syncd/FlexCounter.cpp

+    {
+       swss::DBConnector appDb("CHASSIS_APP_DB", 0, true);
+       swss::RedisPipeline appDbPipeline(&appDb);
+       appDbCountersTable = new swss::Table(&appDbPipeline, "COUNTERS", true);


don't use new operator !

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

use shared_ptr

…n-flexc

…es. Use shared_ptr instead of using new operator. Address code formatting comments

syncd/FlexCounter.cpp

kcudnik · 2024-06-02T15:54:54Z

syncd/FlexCounter.cpp

 };

+void FlexCounter::getCfgSwitchType(
+         _In_ std::string &switch_type)


you are changing output variable, yo ushould use _ Out _, or better make function as return std::string instead of modyfying input parameter

kcudnik · 2024-06-02T15:55:34Z

syncd/FlexCounter.cpp

+   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());


this seems really fishy here, why you need to check all those variables? this should be more unified?

Copying this function from https://github.com/sonic-net/sonic-swss/blob/master/orchagent/main.cpp#L160

kcudnik · 2024-06-02T15:56:05Z

syncd/FlexCounter.cpp

+{
+   try
+   {
+      if (!m_cfgDeviceMetaDataTable->hget("localhost", "switch_type", switch_type))


is this connecting for localhost database to get this ? this seems a hacky way to do that, also all variables are hard coded, those should be defined somewhere with comments

Pretty much every access to database is hardcoded in FlexCounter.cpp. We probably should change all of them in a separate pull request to not mix things up.

kcudnik · 2024-06-02T15:57:03Z

syncd/FlexCounter.cpp

+    m_configDb = std::make_shared<swss::DBConnector>("CONFIG_DB", 0);
+    m_cfgDeviceMetaDataTable = std::make_shared<swss::Table>(m_configDb.get(), CFG_DEVICE_METADATA_TABLE_NAME);
+
+    getCfgSwitchType(m_switchType);
+
+    if (m_switchType == "voq")
+    {
+       m_appDb = std::make_shared<swss::DBConnector>("CHASSIS_APP_DB", 0, true);
+       m_appDbPipeline = std::make_shared<swss::RedisPipeline>(m_appDb.get());
+       m_appDbCountersTable = std::make_shared<swss::Table>(m_appDbPipeline.get(), "COUNTERS", true);
+    }
+


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

Same as above

…es. Use shared_ptr instead of using new operator. Address code formatting comments

…n-flexc

vivekverma-arista · 2024-08-29T08:18:45Z

We will not be changing sonic-sairedis as per the new design, therefore closing this pull request.

Updated HLD: #1587

Populate CHASSIS_APP_DB when flex counters are read

a810271

When flex counters are read, if the counter is for a VOQ, populate chassis app db.

kcudnik requested changes May 16, 2024

View reviewed changes

viveksrao-arista and others added 2 commits June 1, 2024 16:21

Merge branch 'sonic-net:master' into master-populate-chassis-app-db-o…

1e5e680

…n-flexc

Remove global variables. Use VendorSai object to query queue attribut…

73368cc

…es. Use shared_ptr instead of using new operator. Address code formatting comments

kcudnik reviewed Jun 2, 2024

View reviewed changes

syncd/FlexCounter.cpp Show resolved Hide resolved

kcudnik reviewed Jun 2, 2024

View reviewed changes

Remove global variables. Use VendorSai object to query queue attribut…

aeb2bcd

…es. Use shared_ptr instead of using new operator. Address code formatting comments

deepak-singhal0408 assigned kenneth-arista Jul 24, 2024

vivekverma-arista added 2 commits August 11, 2024 11:09

Merge branch 'sonic-net:master' into master-populate-chassis-app-db-o…

62cc42c

…n-flexc

Merge branch 'master' into master-populate-chassis-app-db-on-flexc

56605d0

viveksrao-arista closed this Aug 29, 2024

vivekverma-arista deleted the master-populate-chassis-app-db-on-flexc branch August 29, 2024 08:17

		template <typename StatType>
		class CounterContext : public BaseCounterContext

Conversation

viveksrao-arista commented May 16, 2024

Uh oh!

kcudnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivekverma-arista commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!