-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
SONiC ACL based Metering HLD #1580
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: rajkumar38 <rpennadamram@marvell.com>
community review recording https://zoom.us/rec/share/eeYyppT83wfFB8AZbUF1qqCeHxnoDfRVtrEeXrsnuX71DjrJAn5QKRaBFB9l2nfR.PDKI31uK3y0qXsUI |
acl-loader script will be updated to display policer action in show output. | ||
|
||
```c | ||
admin@sonic:~$ show acl rule |
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.
How about policer statistics? Any plan to have CLI for ACL policer statistics? Please update the HLD.
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.
Since policer can be attached to different entities (port, mirror, acl), we will take it up as separate task. We will come up with separate HLD for the same. I will capture in Overview section that "policer statistics is not supported as part of this HLD" .
Ethernet3 | ||
Ethernet4 | ||
``` |
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.
Could you include CRM for the policer resources.
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.
Based on vendor SAI, policer resources can be per stage. And there is NO existing attribute or API to query the max and current object in SAI per STAGE. Will check with SAI community and update the HLD accordingly.
- MIRRORV6 | ||
- MIRRORDSCP | ||
|
||
- Support for stage: INGRESS and EGRESS. |
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.
why STAGE_PRE_INGRESS is not supported?
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.
Support for stage PRE/POST INGRESS is not yet added in SONiC acl. I will explicitly capture the same in HLD.
|
||
In acl-loader script, an optional argument "--policer_name" will be taken as input and all rules configured as part of the json will be enabled with policer action. | ||
``` | ||
acl-loader update full /tmp/test.json --policer_name policer_1 |
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.
It seems more better to assign policy action per acl rule in json file more than per json.
Is there command config policy mode/cir/cbs?
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.
Note that this is optional argument. Will explore the option of adding the policer action per rule and will update the HLD.
+ leaf-list actions { | ||
+ type enumeration { | ||
+ enum PACKET_ACTION; | ||
+ enum POLICER; |
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.
MIRROR action missed?
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.
Yes. Will update.
Say for flow "x", P1 is applied first, and the remaining traffic is policed by P2. Other traffic flows are applied with policer P1. | ||
### Testing Design | ||
|
||
#### VS Test cases |
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 plan to have unit tests in swss instead of python VS tests.
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.
Sure. Going forward, is python VS test is optional ?
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.
Not exactly. There would be cases which cannot be covered by unit-test in which case we can use VS test.
Support ACL action Policer for stage ingress/egress