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

[acl]: Insert new acl rule with priority in between other rules #482

Merged
merged 5 commits into from
May 17, 2018

Conversation

simone-dell
Copy link
Contributor

What I did
I added a new acl test case: Four ACL rules are configured with priorities 10, 20, 30, 40. Then I added another rule with priority 21. I made sure all five rules were pushed to the ASICDB and with the proper priorities.

Why I did it
Part of the ACL test cases suggested by Dell and approved by Microsoft

How I verified it
Ran the python script in the repo

Details if related
Logs are attached
insert_acl_rule_logs.txt

New test case - I created 4 rules with priorities 10, 20, 30, 40, then I added a fifth rule with priority 21.  Made sure they were all programmed correctly in the DB
I configured five rules with priorities 10, 20, 30, 40, then configured another rule with priority 21.  Verified that all five rules were pushed to the ASICDB
Copy link
Collaborator

@zhenggen-xu zhenggen-xu left a comment

Choose a reason for hiding this comment

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

Why do you add a new test_acl.py file in the orchagent directory? If by accident, please remove it. Modification should be in the tests/test_acl.py only.

mistakenly added this file
@simone-dell
Copy link
Contributor Author

Thanks for pointing it out. I have removed the extra file

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

can you simplify the code logic by using for loop? for example, for the first four rules, can you use for loop to insert, the code are very similar. The validation code has similar issues.

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

please use the same check_rule_existence function you are going to write in your other PR.


tbl = swsscommon.Table(db, "ACL_RULE")
fvs5 = swsscommon.FieldValuePairs([("PRIORITY", "21"), ("PACKET_ACTION", "DROP"), ("ETHER_TYPE", "4660")])
tbl.set("test_insert|acl_test_rule5", fvs5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use num_rules as above for indexing instead of hard-coding to 5

(status, fvs) = atbl.get(entry)
assert status == True
assert len(fvs) == 6
if ('SAI_ACL_ENTRY_ATTR_PRIORITY', '10') in fvs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kindly optimize this part as suggested by Guohan. As a general comment, suggest to use elif if you are checking the same condition against different values

@simone-dell
Copy link
Contributor Author

@lguohan I used the same logic in pull #481

@prsunny prsunny merged commit 8314a94 into sonic-net:master May 17, 2018
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…sonic-net#482)

* Tool to verify ASIC route entries against APPL-DB ROUTE & INTF tables

* code updated per review comments
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
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