Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel #3076

stephenxs · 2024-03-13T14:49:31Z

What I did

Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel
Depends on sonic-net/sonic-sairedis#1362

Why I did it

Currently, the operations of SAI objects and their counters (if any) are triggered by different channels, which introduces racing conditions:

the creation and destruction of the objects are notified using the SelectableChannel,
the operations of counters, including starting and stopping polling the counters, are notified by listening to the FLEX_COUNTER and FLEX_COUNTER_GROUP tables in the FLEX_COUNTER_DB
The orchagent always respects the order when starting/stopping counter-polling (which means to start counter-polling after creating the object and to stop counter-polling before destroying the object) but syncd can receive events in a wrong order, eg. it receives destroying an object first and then stopping counter polling on the object, it can poll counter for a non-exist object, which causes errors in vendor SAI.

The new solution is to extend SAI redis attributes on the SAI_SWITCH_OBJECT to notify counter polling. As a result, all the objects and their counters are notified using a unified channel, the SelectableChannel.

How I verified it

Manual test
Regression test
Mock test

Details if related

Extend SAI redis attribute on the switch object to identify counter-polling operations.
Introduce a CLI option of orchagent to identify whether the new or the old approach is used for notifying counter-polling. It can not be changed on the fly.
Move the logic to notify counter polling from each orchagent class and flex counter manager class to a commonplace. The logic is:
- For the new approach, notify counter polling using SAI extended redis attribute
- For the old approach, notify counter-polling using flex database tables. The corresponding flex database table objects (ProducerTable) are initialized during orchagent initialization, before any flex counter operations.
Remove the definition of and the references to flex counter and flex counter group table in each orchagent class (usually m_flexCounterTable and m_flexCounterGroupTable)
Improve the logic to load the counters' Lua plugin for mock test. The Lua scripts are not available in the mock test scenarios, which causes exceptions to load them in mock test. In case it fails to load the Lua plugin,
- Originally, it skipped any flex counter group operations.
- Now, it will leave the SHA code of the Lua plugin empty, and continue to notify the flex counter operations to SAI redis.
  - In the production scenario, where it can hardly happen, the plugin field will not be notified to SAI.
  - In the mock test scenario, it will notify SAI as it is. This is for mock test to verify whether the plugin field is provided as expected.
Gearbox counter handling.
- Maintain a mapping from each OID created based on the gearbox, from the object ID itself to the gearbox's OID.
- To notify counter operations for such OIDs in the new approach, we need to first fetch the corresponding gearbox OID from the mapping and the notify the gearbox OID as switch OID.
- The old approach looks buggy.
  - There can be more than one gearbox syncd docker in the system, which is a P2MP scenario, the OA needs to communicate to multiple gearbox syncd dockers.
  - The old approach leverages ConsumerTable, ProducerTable mechanism to communicate between OA and sairedis. However, it works for P2P scenarios only. There is a logic for ConsumerTable to consume the update once a gearbox syncd sees it, leaving all rest gearbox syncd daemons to see nothing. As a result, for each update there is only one gearbox syncd that sees and handles it.
There is a logic in intfsorch, copporch and vxlanorch which is to add a counter for an object only after it occurs in the VIRTORID table, which is a WA of the counter out-of-order issue. Now that the issue has been fixed in the new approach, it does not need to check anymore.

Performance analysis
The counter operations are handled in the same thread in both the new and old solutions.
In swss, the counter operation was asynchronous in the old solution and is synchronous now, which can introduce a bit more latency. However, as the number of counter operations is small, no performance degradation is observed.

kcudnik · 2024-03-22T16:07:15Z

Please fix build issues

stephenxs · 2024-03-23T01:15:41Z

Please fix build issues

Hi @kcudnik
It depends on the sairedis PR (sonic-net/sonic-sairedis#1362) to define extended attributes.

kcudnik · 2024-03-23T05:42:38Z

Will review

stephenxs · 2024-03-28T23:34:19Z

/azpw run

mssonicbld · 2024-03-28T23:34:21Z

/AzurePipelines run

azure-pipelines · 2024-03-28T23:34:31Z

Azure Pipelines successfully started running 1 pipeline(s).

stephenxs · 2024-03-29T09:12:31Z

/azpw run

stephenxs · 2024-03-31T00:36:17Z

/azpw run

stephenxs · 2024-03-31T02:58:24Z

As mentioned in the description, the PR depends on sairedis PR (sonic-net/sonic-sairedis#1362) merged a week ago.
However, the sairedis Debian packages fetched in the build were TOO OLD to satisfy the dependency.

##[debug]ProjectId: be1b070f-be15-4154-aade-b1d3bfb17054
##[debug]PipelineVersionToDownload: latestFromBranch
##[debug]Agent environment resources - Disk: / Available 25389.00 MB out of 29593.00 MB, Memory: Used 701.00 MB out of 15968.00 MB, CPU: Usage 2.53%
##[debug]PipelineId from non-trigerringBuild: 495079
Download from the specified build: #495079
##[debug]Processed: ##vso[task.setvariable variable=BuildNumber;issecret=False;]495079
Download artifact to: /data/work/1/a/download/sairedis
Using default max parallelism.

It fetched 495079 in the build. However, it was built on 9th March, more than 2 weeks before my sairedis PR. I think this is because there has been no successful build since 495079. Can anyone help to look into it?

511120	20240330.8	master	Azure.sonic-sairedis	failed	2024-03-30T09:00:59	2024-03-30T10:55:25	594944dbe4	[Build Link](https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=511120)	[Artifacts](https://sonic-build.azurewebsites.net/ui/sonic/pipelines/12/builds/511120/artifacts?branchName=master)
508223	20240326.3	master	Azure.sonic-sairedis	failed	2024-03-26T14:26:18	2024-03-26T15:20:54	594944dbe4	[Build Link](https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=508223)	[Artifacts](https://sonic-build.azurewebsites.net/ui/sonic/pipelines/12/builds/508223/artifacts?branchName=master)
505863	20240323.8	master	Azure.sonic-sairedis	failed	2024-03-23T09:01:19	2024-03-23T10:32:08	bb948f632b	[Build Link](https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=505863)	[Artifacts](https://sonic-build.azurewebsites.net/ui/sonic/pipelines/12/builds/505863/artifacts?branchName=master)
500443	20240316.8	master	Azure.sonic-sairedis	failed	2024-03-16T09:01:01	2024-03-16T10:48:54	bb948f632b	[Build Link](https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=500443)	[Artifacts](https://sonic-build.azurewebsites.net/ui/sonic/pipelines/12/builds/500443/artifacts?branchName=master)

orchagent/p4orch/tests/test_main.cpp

orchagent/saihelper.cpp

orchagent/vxlanorch.cpp

orchagent/flex_counter/flex_counter_manager.cpp

orchagent/flex_counter/flex_counter_manager.h

orchagent/intfsorch.cpp

stephenxs · 2024-04-21T23:30:13Z

/azpw run

mssonicbld · 2024-04-21T23:30:15Z

/AzurePipelines run

azure-pipelines · 2024-04-21T23:30:24Z

Azure Pipelines successfully started running 1 pipeline(s).

stephenxs · 2024-04-30T03:41:32Z

Rebased to resolve the conflict.

kcudnik · 2024-05-04T08:03:48Z

orchagent/saihelper.cpp

+extern bool gTraditionalFlexCounter;
+
+vector<sai_object_id_t> gGearboxOids;
+
+unique_ptr<DBConnector> gFlexCounterDb;
+unique_ptr<ProducerTable> gFlexCounterGroupTable;
+unique_ptr<ProducerTable> gFlexCounterTable;
+unique_ptr<DBConnector> gGearBoxFlexCounterDb;
+unique_ptr<ProducerTable> gGearBoxFlexCounterGroupTable;
+unique_ptr<ProducerTable> gGearBoxFlexCounterTable;


seems like all this is related, so it could be moved to struct/class, but it seems for now that this could be like that, since OA needs ad ground up refactoring to move all global stuff to proper classes/namespaces

kcudnik

i believe this will work, but some of changes i proposed would be desired

kcudnik · 2024-05-04T08:04:10Z

orchagent/saihelper.cpp

@@ -261,6 +272,20 @@ void initSaiApi()
 sai_log_set(SAI_API_TWAMP, SAI_LOG_LEVEL_NOTICE);
 }

+void initFlexCounterTables()


yea, looks like a constructor task for proposed class :D

kcudnik · 2024-05-04T08:04:39Z

orchagent/saihelper.cpp

@@ -261,6 +272,20 @@ void initSaiApi()
 sai_log_set(SAI_API_TWAMP, SAI_LOG_LEVEL_NOTICE);
 }

+void initFlexCounterTables()


should this be static inline ?

No need. It is called outside this compiling unit.

kcudnik · 2024-05-04T08:05:07Z

orchagent/saihelper.cpp

+ }
+ else
+ {
+ sai_s8_list.list = nullptr;


ehter use initSaiRedisCounterEmptyParameter or remove that function

kcudnik · 2024-05-04T08:07:50Z

orchagent/saihelper.cpp

+{
+ if (sai_switch_api == nullptr)
+ {
+ // This can happen during destruction of the orchagent daemon.


why syncd operation counter would be notifying syncd when OA is destroyed ? sai_switch_api should be destroyed at the end, only scenario i can imaged, is that SAI API init failed and sai_switch_api was NULL yet, but in that case no other operations should be performed and OA should quit, if switch API is NULL you can do nothing, that means sairedis lib initialization failed, so you will not send any notification anywaahre, and such initialization failre should be logged with critical prioirty not just error (not this log)

This is for mock test scenario only.
In production scenario, the test if (sai_switch_api == nullptr) won't be true.

kcudnik · 2024-05-04T08:08:55Z

orchagent/saihelper.cpp

+static inline void operateFlexCounterDbSingleField(std::vector<FieldValueTuple> &fvTuples,
+ const string &field, const string &value)
+{
+ if (!field.empty() && !value.empty())


is this prevention here for a reason? ot at some point OA can actually populate empty counter? if any of those is empty this should be logged as ERROR

This is also for mock test.
The common logic to install plugin is like this:

load lua plugin and generate SHA code

install the lua plugin into {plugin name: plugin SHA code} the flex counter group

In mock test scenario, it fails to load the Lua plugin, which leaves the SHA code empty string. But we still want the lua plugin to be installed into the flex counter group. By doing so, we can verify whether the plugin name is correct and meet the coverage requirement.

In production scenario, we will check whether either is empty, if so, we will not install the plugin. This is just a production and can not happen.

kcudnik · 2024-05-04T08:12:45Z

orchagent/saihelper.cpp

+ if (gTraditionalFlexCounter)
+ {
+ operateFlexCounterGroupDatabase(group, poll_interval, stats_mode, plugin_name, plugins, operation, is_gearbox);
+ return;
+ }


so, normally in the OOP, such cases is resolved in proper way, by having base abstract class, and pointer in that class in OA, then base on command line in OA whether we want to use traditional counters or new one, just one of 2 objects is createed and populate the pointer, then all operations are the same, add field to counder, add counter remove counter etc, and in all entire code there is no need for gTraditionalFlexCounter) check, and i saw is checked in multiple places, and old and new counter realise the same functionality at the end, but im aware that at this stage this conversion without refactoring OA maybe not possible, it should be designed from the start

yea, look all the below functions, they all contain the same check, and adding new code could confuse unaware programmer, so moving this to 2 classes with one new and second ol would be great solution to that

kcudnik · 2024-05-04T08:17:33Z

@stephenxs can you please handle the conflict? @kcudnik thanks for the great feedback. assuming conflicts are handled, any further feedback on the PR or this can be approved from your end?

i added some potential refactoring changes that could be desired, but i approved current solution, but i don't have permission to approve that, you need @qiluo-msft approve or @prsunny approve

dprital · 2024-05-06T16:54:22Z

@qiluo-msft @prsunny , Can you please approve and merge ?

…nter polling and create counter group SAI switch API is invoked to enable counter polling and create counter group CLI option to choose whether the new infra should be used Signed-off-by: Stephen Sun <stephens@nvidia.com>

Signed-off-by: Stephen Sun <stephens@nvidia.com>

stephenxs force-pushed the poc-flexcounter-new-infra branch 2 times, most recently from c2fdae6 to 644092b Compare March 18, 2024 12:58

stephenxs mentioned this pull request Mar 19, 2024

[Flex Counter] observe race condition while removing a RIF object sonic-net/sonic-buildimage#14628

Closed

stephenxs force-pushed the poc-flexcounter-new-infra branch from 644092b to 707f2de Compare March 20, 2024 13:19

stephenxs marked this pull request as ready for review March 21, 2024 10:39

stephenxs requested review from abdosi, judyjoseph and prsunny as code owners March 21, 2024 10:39

stephenxs requested a review from kcudnik March 21, 2024 10:40

stephenxs force-pushed the poc-flexcounter-new-infra branch 2 times, most recently from 15df98e to 8a349fc Compare March 28, 2024 10:16

stephenxs force-pushed the poc-flexcounter-new-infra branch 2 times, most recently from b1303d5 to 124600f Compare March 30, 2024 02:21

kcudnik reviewed Apr 2, 2024

View reviewed changes

orchagent/p4orch/tests/test_main.cpp Show resolved Hide resolved

kcudnik reviewed Apr 2, 2024

View reviewed changes

orchagent/saihelper.cpp Show resolved Hide resolved

kcudnik reviewed Apr 2, 2024

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

stephenxs force-pushed the poc-flexcounter-new-infra branch 2 times, most recently from 9a7400d to 7301ad0 Compare April 6, 2024 08:59

kcudnik reviewed Apr 7, 2024

View reviewed changes

orchagent/flex_counter/flex_counter_manager.cpp Show resolved Hide resolved

kcudnik reviewed Apr 7, 2024

View reviewed changes

orchagent/flex_counter/flex_counter_manager.h Outdated Show resolved Hide resolved

kcudnik reviewed Apr 7, 2024

View reviewed changes

orchagent/intfsorch.cpp Show resolved Hide resolved

stephenxs force-pushed the poc-flexcounter-new-infra branch from 1bbeb21 to 1730f9b Compare April 21, 2024 14:22

stephenxs force-pushed the poc-flexcounter-new-infra branch from 1730f9b to 1270128 Compare April 22, 2024 23:47

stephenxs force-pushed the poc-flexcounter-new-infra branch from 1270128 to c8487a1 Compare April 30, 2024 03:40

stephenxs requested a review from kcudnik April 30, 2024 03:54

prsunny requested a review from neethajohn April 30, 2024 16:24

kcudnik reviewed May 4, 2024

View reviewed changes

kcudnik approved these changes May 4, 2024

View reviewed changes

stephenxs force-pushed the poc-flexcounter-new-infra branch from c8487a1 to 9510d41 Compare May 4, 2024 14:15

stephenxs changed the title ~~Fix flow counter out-of-order issue by notifying counter operations using SelectableChannel~~ Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel May 6, 2024

stephenxs force-pushed the poc-flexcounter-new-infra branch from 9510d41 to edb90d1 Compare May 13, 2024 23:13

stephenxs added 7 commits May 15, 2024 15:05

Make init parameter functions static

56ed879

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Fix review comments

1ea7404

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Fix error

9cfaf80

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Improve coverage

a521a1b

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Fix review comments

288ea07

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Fix a minor review comment

7cca7d9

Signed-off-by: Stephen Sun <stephens@nvidia.com>

stephenxs force-pushed the poc-flexcounter-new-infra branch from 2dd1a2f to 7cca7d9 Compare May 15, 2024 07:05

kcudnik approved these changes May 15, 2024

View reviewed changes

prsunny approved these changes May 15, 2024

View reviewed changes

prsunny merged commit 26a5a1c into sonic-net:master May 15, 2024
17 checks passed

stephenxs deleted the poc-flexcounter-new-infra branch May 15, 2024 23:17

stephenxs mentioned this pull request May 17, 2024

[Flex counter] A buffer pool object can be removed before its counter is removed even if orchagent removes the counter first #3004

Closed

arlakshm mentioned this pull request Jun 5, 2024

[DPB][FLEX Counters] Error seen in SDK reading counters for removed ports sonic-net/sonic-buildimage#19105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel #3076

Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel #3076

stephenxs commented Mar 13, 2024 •

edited

Loading

kcudnik commented Mar 22, 2024

stephenxs commented Mar 23, 2024 •

edited

Loading

kcudnik commented Mar 23, 2024

stephenxs commented Mar 28, 2024

mssonicbld commented Mar 28, 2024

azure-pipelines bot commented Mar 28, 2024

stephenxs commented Mar 29, 2024

stephenxs commented Mar 31, 2024

stephenxs commented Mar 31, 2024 •

edited

Loading

stephenxs commented Apr 21, 2024

mssonicbld commented Apr 21, 2024

azure-pipelines bot commented Apr 21, 2024

stephenxs commented Apr 30, 2024

kcudnik May 4, 2024

kcudnik left a comment

kcudnik May 4, 2024

kcudnik May 4, 2024

stephenxs May 7, 2024

kcudnik May 4, 2024

stephenxs May 14, 2024

kcudnik May 4, 2024

stephenxs May 7, 2024

kcudnik May 4, 2024

stephenxs May 7, 2024

kcudnik May 4, 2024

kcudnik May 4, 2024

kcudnik commented May 4, 2024

dprital commented May 6, 2024

Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel #3076

Fix flex counter out-of-order issue by notifying counter operations using SelectableChannel #3076

Conversation

stephenxs commented Mar 13, 2024 • edited Loading

kcudnik commented Mar 22, 2024

stephenxs commented Mar 23, 2024 • edited Loading

kcudnik commented Mar 23, 2024

stephenxs commented Mar 28, 2024

mssonicbld commented Mar 28, 2024

azure-pipelines bot commented Mar 28, 2024

stephenxs commented Mar 29, 2024

stephenxs commented Mar 31, 2024

stephenxs commented Mar 31, 2024 • edited Loading

stephenxs commented Apr 21, 2024

mssonicbld commented Apr 21, 2024

azure-pipelines bot commented Apr 21, 2024

stephenxs commented Apr 30, 2024

Choose a reason for hiding this comment

kcudnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcudnik commented May 4, 2024

dprital commented May 6, 2024

stephenxs commented Mar 13, 2024 •

edited

Loading

stephenxs commented Mar 23, 2024 •

edited

Loading

stephenxs commented Mar 31, 2024 •

edited

Loading