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

[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

Conversation

wangshengjun
Copy link
Contributor

…e is not PORT.

Signed-off-by: wangshengjun wangshengjun@asterfusion.com

- What I did
Fix the 'crm show' error for acl table which bind point type is LAG.
- How I did it
check whether the key exists in the dict before dump the data.
- How to verify it
admin@sonic:$ cat acl_redirect_portchannel.json
{
"ACL_TABLE":{
"Test2":{
"policy_desc":"test2",
"type":"L3",
"ports":["PortChannel0001"]
}
},
"ACL_RULE":{
"Test2|RULE1":{
"PRIORITY":"100",
"SRC_IP":"2.1.1.1",
"PACKET_ACTION":"DROP"
}
}
}
admin@sonic:
$ sudo config load acl_redirect_portchannel.json
Load config from the file(s) acl_redirect_portchannel.json ? [y/N]: y
Running command: /usr/local/bin/sonic-cfggen -j acl_redirect_portchannel.json --write-to-db
admin@sonic:~$ show acl table
Name Type Binding Description Stage


Test2 L3 PortChannel0001 test2 ingress
- Previous command output (if the output of a command-line utility has changed)
admin@sonic:~$ crm show resources acl group
Traceback (most recent call last):
File "/usr/bin/crm", line 12, in
sys.exit(cli())
File "/usr/lib/python2.7/dist-packages/click/core.py", line 722, in call
return self.main(*args, **kwargs)
File "/usr/lib/python2.7/dist-packages/click/core.py", line 697, in main
rv = self.invoke(ctx)
File "/usr/lib/python2.7/dist-packages/click/core.py", line 1066, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/lib/python2.7/dist-packages/click/core.py", line 1066, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/lib/python2.7/dist-packages/click/core.py", line 1066, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/lib/python2.7/dist-packages/click/core.py", line 1066, in invoke
return process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/lib/python2.7/dist-packages/click/core.py", line 895, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/usr/lib/python2.7/dist-packages/click/core.py", line 535, in invoke
return callback(*args, **kwargs)
File "/usr/lib/python2.7/dist-packages/click/decorators.py", line 17, in new_func
return f(get_current_context(), *args, **kwargs)
File "/usr/lib/python2.7/dist-packages/crm/main.py", line 480, in group
ctx.obj["crm"].show_acl_resources()
File "/usr/lib/python2.7/dist-packages/crm/main.py", line 109, in show_acl_resources
crm_stats['crm_stats
' + res + "_used"],
KeyError: 'crm_stats_acl_table_used'

127.0.0.1:6379[2]> HGETALL "CRM:ACL_STATS:INGRESS:LAG"

  1. "crm_stats_acl_group_used"
  2. "1"
  3. "crm_stats_acl_group_available"
  4. "0"
    127.0.0.1:6379[2]>

- New command output (if the output of a command-line utility has changed)
admin@sonic:~$ crm show resources acl group
Stage Bind Point Resource Name Used Count Available Count


INGRESS PORT acl_group 0 0
INGRESS PORT acl_table 2 0
INGRESS LAG acl_group 1 0

…e is not PORT.

Signed-off-by: wangshengjun <wangshengjun@asterfusion.com>
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? :)

@jleveque
Copy link
Contributor

Retest this please

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.

4 participants