[flow counters] High level design for trap flow counters support #858
Conversation
Junchao-Mellanox
force-pushed
the
flow-counter
branch
from
e78e648 to
990e1e6
Compare
Junchao-Mellanox
changed the title
Junchao-Mellanox
force-pushed
the
flow-counter
branch
from
990e1e6 to
c81ba64
Compare
Junchao-Mellanox
force-pushed
the
flow-counter
branch
from
c81ba64 to
fd50ba4
Compare
Junchao-Mellanox
force-pushed
the
flow-counter
branch
from
fd50ba4 to
434a5fc
Compare
| - Statistics shall be enabled or disabled for all configured traps (not per-trap type) | ||
| - Statistics for traps shall be provided as a number of messages delivered from ASIC to host over the host interface per trap type | ||
| - Statistics shall be available for clearing (for all traps together) using the CLI command | ||
| - Statistic shall support number of trap packets/bytes and number of trap PPS |
There was a problem hiding this comment.
in the community meeting, zhenghui raise a question that we should add drop counter for the trap since trap policer is mainly used for dos scenario. In this case, it is important to know what packets are dropped. in order to count the drop counters, we need to use the sai policer_state to get drop packets/bytes. https://github.com/opencomputeproject/SAI/blob/master/inc/saipolicer.h#L219
There was a problem hiding this comment.
In this case, flow counter is somewhat duplicated with policer stats, we probably want to use the existing policer objects even for received packets/bytes?.
There was a problem hiding this comment.
agree, instead of attaching counter to the policer, we can just attach policer_stat then we can get dropped and non-dropped counters.
There was a problem hiding this comment.
Hi @lguohan and @caizhenghui-juniper ,
Policer object is usually bound to a trap group, and a trap group usually contains multiple traps. For example, trap group "queue4_group1" could have trap like "bgp,bgpv6,lacp". Based on that, sai_policer_stats can only provide the stats for a group like "queue4_group1", and there is no detail stat for trap like "bgp,bgpv6,lacp".
I would say that flow counter is not a duplicated with policer stats because it provides different use case and more detailed information, and it can be used to debug the policer if it does not work as expect.
For example, for DDOS case, trap flow counter is still useful, as user can see that there are too many packet traps to CPU via command show flowcnt-trap stats, and user can get the exact trap type easily, and user might realize that he/she forgot to set the policer for the trap group.
So, my view is that "trap flow counter" is the counter that provides a per trap basis statistic; policer stat is the counter that provides a per policer basis statistic, and policer now can only be bound to a trap group. These are different use-cases and these counters (per trap or per policer object) provide different granularity. Policer stats should be supported in another feature.
There was a problem hiding this comment.
@caizhenghui-juniper @lguohan would you please have a look at Junchao's feedback?
There was a problem hiding this comment.
@Junchao-Mellanox In this case, I can see the use case fo the flow counter. But still, the drop counter is needed for insights about the specific flow types for detection. Please add it in the design. Furthermore, can you also add the change of CoPP testing using the counters too?
There was a problem hiding this comment.
@caizhenghui-juniper Thanks for your comment. For the CoPP test case change, you can check PR sonic-net/sonic-mgmt#4456. For drop counter for a trap type, I don't see a SAI attribute support it, I will discuss with Arch and get back to you.
There was a problem hiding this comment.
Hi @caizhenghui-juniper and @lguohan , I agree that we should have drop counter per trap to enable better debug capability. As you may know, the existing generic counter framework does not provide such an SAI attribute, neigher do other SAI objects. And it is probably not suitable to add such attribute to generic counter. We are working on a new SAI attribute to cover that part, and it will be a new feature (need SAI/SDK/SONiC design and development).
There was a problem hiding this comment.
we should have selected the policer stat to implement the feature. if we proceed with this design, what is the transition plan to move to police stat, i do not think other platform can support policer stat as well as counter stat at the same time.
**What I did** Add trap flow counter related tables/entries to schema.h. See HLD: sonic-net/SONiC#858 **Why I did it** Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Host interface trap counter can get number of received traps per Trap ID. **How I verified it** Manual test/VS test/sonic mgmt test
|
Hi @lguohan and @caizhenghui-juniper , could you please review my reply and provide your comments? |
This reverts commit c630ea1 Reverts #4456 This feature has not yet been reviewed sonic-net/SONiC#858 Affecting nighty run results because of this new testcase without feature support
|
@prsunny could you please help to review as well? |
* Add trap flow counter support. See HLD: sonic-net/SONiC#858 * Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Host interface trap counter can get number of received traps per Trap ID.
|
@caizhenghui-juniper , could you please sign-off? |
…sonic-net#4747) This reverts commit c630ea1 Reverts sonic-net#4456 This feature has not yet been reviewed sonic-net/SONiC#858 Affecting nighty run results because of this new testcase without feature support
* Add trap flow counter support. See HLD: sonic-net/SONiC#858 * Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Host interface trap counter can get number of received traps per Trap ID.
High level design for flow counters support.
Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Flow counters could cover cases like:
This document focus on host interface traps counter.
Related PRs: