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

SONiC ACL based Metering HLD #1580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rajkumar38
Copy link
Contributor

Support ACL action Policer for stage ingress/egress

Signed-off-by: rajkumar38 <rpennadamram@marvell.com>
@zhangyanzhao
Copy link
Collaborator

acl-loader script will be updated to display policer action in show output.

```c
admin@sonic:~$ show acl rule
Copy link
Contributor

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.

Copy link
Contributor Author

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

```
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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?

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

MIRROR action missed?

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants