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.

Sign up for GitHub

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

[crm/show]fix the 'crm show' error for acl table which bind point typ… #1178

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

[crm/show]fix the 'crm show' error for acl table which bind point typ… #1178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions crm/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,12 @@ def show_acl_resources(self):

if crm_stats:
for res in ["acl_group", "acl_table"]:
data.append([
stage, bind_point, res,
crm_stats['crm_stats_' + res + "_used"],
crm_stats['crm_stats_' + res + "_available"]
])
if "crm_stats_" + res + "_used" in crm_stats:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a bug that we only have group but not table? can you please provide the show acl table output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

admin@sonic:~$ show acl table
Name Type Binding Description Stage
Test2 L3 PortChannel0001 test2 ingress

admin@sonic:~$ crm show resources acl table

Table ID Resource Name Used Count Available Count

0x700000000036a acl_entry 2 254

Table ID Resource Name Used Count Available Count

0x70000000003c9 acl_entry 1 255
0x70000000003c9 acl_counter 1 255

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we create acl table, aclorch uses 'SAI_ACL_BIND_POINT_TYPE_PORT' as the fixed parameters for 'incCrmAclUsedCounter'.

Copy link
Contributor

Choose a reason for hiding this comment

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

so its a bug in aclorch code. @daall , what do you think?, shall we raise a github issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is probably a bug, but I still don't understand how we trigger this bug in the CLI. I think all tables that aclorch creates have port + LAG bindings regardless of whether the members of that tables are ports or LAGs. So why does this only appear when the table members are LAGs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@daall, thats because in this test example, there is only one entry for LAG and nothing for port. Are you saying that is not expected?

admin@sonic:~$ show acl table
Name Type Binding Description Stage
Test2 L3 PortChannel0001 test2 ingress

Copy link
Contributor

Choose a reason for hiding this comment

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

@prsunny let me try applying the orchagent fix and seeing what happens. I agree with you and @wangshengjun that orchagent should be updating the CRM for BIND_POINT_LAG as well, but I am unsure if that will totally fix this problem or if this (or some other CLI update) is still needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daall if you try to fix the aclorch, please think about the scenario that acl table bind on both port and LAG.In this case, the CRM shoud add for both port and LAG for one acl table, is that reasonable for CRM?And if we add twice for one acl table, what about the del case for that acl table, how to deal with the CRM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daall Is there any progress can share with us?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wangshengjun I believe the orchagent changes have been fully merged now, could you let us know if you still see this bug in the latest master image? :)

data.append([
stage, bind_point, res,
crm_stats['crm_stats_' + res + "_used"],
crm_stats['crm_stats_' + res + "_available"]
])

click.echo()
click.echo(tabulate(data, headers=header, tablefmt="simple", missingval=""))
Expand Down