Skip to content
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

[flow counters] High level design for trap flow counters support #858

Merged
merged 1 commit into from Dec 15, 2021

Conversation

Junchao-Mellanox
Copy link
Contributor

@Junchao-Mellanox Junchao-Mellanox commented Sep 7, 2021

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:

  • Host interface traps (number of received traps per Trap ID)
  • Routes matching the configured prefix pattern (number of hits and number of bytes)
  • FDB entries matching the configured VXLAN tunnel or using the VLAN ID as pattern
  • Next-Hop/Next-Hop Group/Next-Hop Group Member

This document focus on host interface traps counter.

Related PRs:

PR title state context
[sonic-buildimage] Add trap flow counter support GitHub issue/pull request detail GitHub pull request check contexts
[sonic-swss] [orchagent] Add trap flow counter support GitHub issue/pull request detail GitHub pull request check contexts
[sonic-mgmt] Add test case for trap flow counter feature GitHub issue/pull request detail GitHub pull request check contexts
[sonic-utilities] Add trap flow counter support GitHub issue/pull request detail GitHub pull request check contexts
[sonic-sairedis] Add trap flow counter support GitHub issue/pull request detail GitHub pull request check contexts
[sonic-swss-common] Add trap flow counter support GitHub issue/pull request detail GitHub pull request check contexts
[sonic-utilities] [Command Reference] Add command reference for trap flow counters GitHub issue/pull request detail GitHub pull request check contexts
[sonic-buildimage] [YANG] Add trap flow counter to yang model GitHub issue/pull request detail GitHub pull request check contexts

- 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, instead of attaching counter to the policer, we can just attach policer_stat then we can get dropped and non-dropped counters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@caizhenghui-juniper @lguohan would you please have a look at Junchao's feedback?

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@zhangyanzhao zhangyanzhao added this to In progress in 202111 Release HLD via automation Oct 12, 2021
@zhangyanzhao zhangyanzhao moved this from In progress to Reviewed in 202111 Release HLD Oct 12, 2021
qiluo-msft pushed a commit to sonic-net/sonic-swss-common that referenced this pull request Oct 18, 2021
**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
@Junchao-Mellanox
Copy link
Contributor Author

Hi @lguohan and @caizhenghui-juniper , could you please review my reply and provide your comments?

neethajohn added a commit to sonic-net/sonic-mgmt that referenced this pull request Nov 23, 2021
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
@liat-grozovik
Copy link
Collaborator

@prsunny could you please help to review as well?
Code changes are already under review and it is better to have the HLD approved a head of the review completion.

prsunny pushed a commit to sonic-net/sonic-swss that referenced this pull request Dec 1, 2021
* 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.
@prsunny
Copy link
Contributor

prsunny commented Dec 8, 2021

@caizhenghui-juniper , could you please sign-off?

Copy link

@caizhenghui-juniper caizhenghui-juniper left a comment

Choose a reason for hiding this comment

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

Please add drop counter capability.

@qiluo-msft qiluo-msft merged commit 74b334f into sonic-net:master Dec 15, 2021
202111 Release HLD automation moved this from Reviewed to Done Dec 15, 2021
AntonHryshchuk pushed a commit to AntonHryshchuk/sonic-mgmt that referenced this pull request Jan 4, 2022
…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
@Junchao-Mellanox Junchao-Mellanox deleted the flow-counter branch January 24, 2022 01:59
isabelmsft pushed a commit to isabelmsft/sonic-mgmt that referenced this pull request Feb 10, 2022
…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
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants