-
Notifications
You must be signed in to change notification settings - Fork 529
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
[orchagent] Support for aggregrate VOQ Counters. #3047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add tests
orchagent/portsorch.cpp
Outdated
for (size_t queueIndex = 0; queueIndex < queue_ids.size(); ++queueIndex) | ||
{ | ||
std::ostringstream key; | ||
key << port.m_system_port_info.alias << "@" << gMyHostName << "|" << gMyAsicName << ":" << queueIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please share key example here.
orchagent/portsorch.cpp
Outdated
{ | ||
SWSS_LOG_DEBUG("%s %s %s",key.str().c_str(), sai_serialize_queue_stat(it).c_str(), | ||
to_string(queueStats[index]).c_str()); | ||
m_tableVoqQueueCounter->hset(key.str(), sai_serialize_queue_stat(it), to_string(queueStats[index++])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is this feature supported on SUP ?
- Please share CLI command as well as o/p.
- Please explain how aggregation of VoQ Stats from multiple LCs happening here. As VoQ id may be same across multiple LCs, it may end up over-writing existing data.
- please try reading the queue counters from LC while this polling is happening to make sure LC queue counter show is not impacting SUP o/p.
@vivekverma-arista Can you please do test to send traffic from multiple sources to single destination and see if the aggregation is happening properly in the CLI o/p ? |
Also, Can we have command format like - show queue counter --voq as it will be in-line with following - show queue counters |
please rebase and bring to latest. |
@saksarav-nokia please review as well |
orchagent/portsorch.cpp
Outdated
std::ostringstream key; | ||
key << port.m_system_port_info.alias << "@" << gMyHostName << "|" << gMyAsicName << ":" << queueIndex; | ||
|
||
sai_status_t status = sai_queue_api->get_queue_stats( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support getting the voq stats locally at the linecard level, can we update the chassis_db when the voq stats are updated locally on the linecard? why we another timer and get the stats again to update to the chassis?
This reverts commit cb6b968.
This reverts commit fe29d7c.
Create voq to port name map during port initializa Why I did it? When ports are getting created and voq's are being assigned to them, create a voq to port name map table in counter DB so that it is easy to find the port name for a specific voq when creating an entry in chassis_app_db to calculate agg voq counters.
Please update the code as it is out of date. @viveksrao-arista, Is this change to maintain mapping of VoQ to system port ? To be needed while updating the data to chassis_app_db ? |
Yes, this table maps a VoQ to system port and is used to obtain the interface name to which the VoQ belongs to when updating chassis_app_db. |
@viveksrao-arista please rebase it. |
Rebased the code. |
@vivekverma-arista please re-base |
@vivekverma-arista : Please update PR description on which HLD Component this PR is targetting. |
We have changed the approach to be taken, hence closing this pull request. HLD has been updated with the new approach and new PR will be opened. |
What I did
Implemented aggregate VOQ counters support according to aggregate VOQ counters HLD #1578
Why I did it
Part of aggregate VOQ counter feature support #1543
How I verified it
Sent traffic to the switch and checked that VOQ stats are getting populated in CHASSIS_APP_DB