Skip to content

[aclshow] output only counters per table/rule#442

Merged
liat-grozovik merged 3 commits intosonic-net:masterfrom
romankachur-mlnx:aclshow
Feb 11, 2019
Merged

[aclshow] output only counters per table/rule#442
liat-grozovik merged 3 commits intosonic-net:masterfrom
romankachur-mlnx:aclshow

Conversation

@romankachur-mlnx
Copy link
Copy Markdown
Contributor

@romankachur-mlnx romankachur-mlnx commented Jan 23, 2019 •
edited
Loading

- What I did
Modified aclshow utility.

  1. Removed three unneeded columns (Type / Priority / Action) from the output
    (as this information is duplicated by acl-loader utility).
  2. Removed unneeded arguments (details and ports) passed to the utility.
  3. Added handling of arguments (rules and tables), which were accepted by the utility but not handled
    (now the output is filtered based on the passed arguments).

- How I did it
Modified scripts/aclshow
- How to verify it
Run aclshow utility with all possible options:
aclshow
aclshow -h
aclshow -a -vv
aclshow -t <existing_table,unexisting_table> -a
aclshow -r <existing_rule,unexisting_rule> -a
- Previous command output (if the output of a command-line utility has changed)

aclshow   -a -t DATAACL -r RULE_2,RULE_3,DEFAULT_RULE -vv
Reading ACL info...
ACL Tables to show: 4
ACL Rules to show: 14
ACL Counters found: 14

RULE NAME     TABLE NAME    TYPE      PRIO  ACTION      PACKETS COUNT    BYTES COUNT
------------  ------------  ------  ------  --------  ---------------  -------------
RULE_1        DATAACL       L3        9999  FORWARD                 0              0
RULE_2        DATAACL       L3        9998  FORWARD                 0              0
RULE_3        DATAACL       L3        9997  FORWARD                 0              0
RULE_4        DATAACL       L3        9996  FORWARD                 0              0
RULE_5        DATAACL       L3        9995  FORWARD                 0              0
RULE_6        DATAACL       L3        9994  FORWARD                 0              0
RULE_7        DATAACL       L3        9993  DROP                    0              0
RULE_8        DATAACL       L3        9992  FORWARD                 0              0
RULE_9        DATAACL       L3        9991  FORWARD                 0              0
RULE_10       DATAACL       L3        9990  FORWARD                 0              0
RULE_11       DATAACL       L3        9989  FORWARD                 0              0
RULE_12       DATAACL       L3        9988  FORWARD                 0              0
RULE_13       DATAACL       L3        9987  FORWARD                 0              0
DEFAULT_RULE  DATAACL       L3           1  DROP                 5157         402246

- New command output (if the output of a command-line utility has changed)

aclshow   -a -t DATAACL -r RULE_2,RULE_3,DEFAULT_RULE -vv
Reading ACL info...
Total number of ACL Tables: 4
Total number of ACL Rules: 14

RULE NAME     TABLE NAME      PACKETS COUNT    BYTES COUNT
------------  ------------  ---------------  -------------
DEFAULT_RULE  DATAACL                  5177         403806
RULE_2        DATAACL                     0              0
RULE_3        DATAACL                     0              0

-->

@romankachur-mlnx
Copy link
Copy Markdown
Contributor Author

romankachur-mlnx commented Jan 24, 2019

retest this please

@romankachur-mlnx
Update aclshow
136b589 
Fixed output  "..ACL Tables: %d"
@romankachur-mlnx romankachur-mlnx changed the title [aclshow] improvement [aclshow] shorten command output. remove unneeded arguments. add handler for tables and rules Jan 24, 2019
@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Jan 24, 2019

@romankachur-mlnx please fix commit typo 'acs- to acl-'
The output of the new aclshow only provides counters' information per table/per rule. The info about the rule/table should be retrieved from 'acl-loader show'.
@lguohan now that the aclshow only handles ACL counters should we change the name?

@romankachur-mlnx it might be a good idea to send in the mailing list that the UI is going to change. we dont know if SONiC users has any automation based on it which is not exposed and the community need some time to get ready for it.

@romankachur-mlnx romankachur-mlnx changed the title [aclshow] shorten command output. remove unneeded arguments. add handler for tables and rules [aclshow] output only counters per table/rule Jan 24, 2019
@jleveque
Copy link
Copy Markdown
Contributor

jleveque commented Jan 24, 2019

now that the aclshow only handles ACL counters should we change the name?

Maybe we should consider combining acl-loader and aclshow logic into one utility, "aclutil"?

@romankachur-mlnx
Copy link
Copy Markdown
Contributor Author

romankachur-mlnx commented Jan 25, 2019

retest this please

@romankachur-mlnx
Update aclshow
89a2d30 
Fixed a case when counters, once reset to 0, are not showed untill they are increased ('>0').
Now counters, if reset to 0, are shown as 0 ('>=0'.)
@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Feb 7, 2019

now that the aclshow only handles ACL counters should we change the name?

Maybe we should consider combining acl-loader and aclshow logic into one utility, "aclutil"?

Following offline discussion it was agreed to have this fix regardless on the suggestion raised.
The idea is to have an existing tool working (fixes few detected issues) and then work on new UI for ACL show and config. From my understanding such UI will be defined as part of the everflow improvements.

@liat-grozovik liat-grozovik merged commit e856b8b into sonic-net:master Feb 11, 2019
@romankachur-mlnx romankachur-mlnx mentioned this pull request Mar 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liat-grozovik liat-grozovik liat-grozovik approved these changes

+1 more reviewer

@jleveque jleveque jleveque approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

acl-loader/aclshow Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@romankachur-mlnx @liat-grozovik @jleveque