[DPB-ACL] Handle ACL dependency

[vasant17]

NOTE: Please review only commit 77db91c. The other commit will be pushed as a different PR #1112.

What I did

Add update capability for ACL tables when stage and type attributes are not modified. That is provide an way to update portlist of ACL tables

Existing code never deleted a ACL group associated with a port once it is created(it is created when port is bound to an ACL table), that is because we never deleted the port. As part of DPB, as we need to delete the port, hence we also need to delete the ACL group associated with port. But note that, a port can be part of multiple ACL tables. We should delete the ACL group once port is unbound with the last ACL table, and we should create the ACL group once the port is bound to the first ACL table.

Introduced an ENUM and dependency bitmap in port class, to keep track of port dependencies. For example, if port is part of ACL table, Port::ACL_DEP bit is set, and if port is part of VLAN as a member, Port::VLAN_DEP bit is set. Bits are also created on removal of port from ACL table or VLAN. We can extend this to other features as well. Hence a port can be removed only when no bit is set. If any one bit is set, port removal logic will keep trying in portsOrch.

Made the logic to bind and unbind a port to ACL Table symmetric.

Write DPB test infra class and test cases. Note the scale test case is SKIPPED for now as we are hitting VS library issue and the orchagent gets stuck. Fix for that is in-review as of this writing, once we pull in that fix, I will enable that test case.

Code-cleanup

Why I did it
To support DPB feature with ACL dependency

How I verified it
Wrote VS pytests. Also ensured that current test cases are NOT broken

Details if related

[daall]

Thank you for adding this - being able to update the binding list in-place is going to make a lot of things simpler going forward. :) See comments for review - it's mostly just nit-picky style comments to make the code a little more clear. Good stuff!

[daall]

Gave it another pass, let me know if you have any questions. :)

[vasant17]

retest this please

[vasant17]

retest this please

[vasant17]

retest this please

[zhenggen-xu]

@yxieca @daall Do you have more comments on this PR? If not, can you sign off and merge? Thanks!

[daall]

@yxieca @daall Do you have more comments on this PR? If not, can you sign off and merge? Thanks!

@zhenggen-xu @vasant17 sorry for the delay! We're going to do some checking in the lab to make sure this is working OK across platforms. ACLs are one of the more finnicky, ASIC-specific parts of SONiC and we want to avoid any surprises down the road for you/us as a result of enabling this.

[daall]

@zhenggen-xu @vasant17 ok I tried this out with a Mellanox Spectrum chip and a Broadcom Tomahawk chip.

The Spectrum looks good, but we're seeing these errors on TH when I tried to delete an interface from the binding list:

Mar  2 21:06:48.733596 sonic NOTICE swss#orchagent: :- updateAclTablePorts: Deleting port Ethernet8 from ACL list DATAACL
Mar  2 21:06:48.733596 sonic NOTICE swss#orchagent: :- updateAclTablePorts: Unbind and Unlink:Ethernet8
Mar  2 21:06:48.734322 sonic NOTICE swss#orchagent: :- unbindRemoveAclTableGroup: Removing port OID 100000000000c ACL table group ID
Mar  2 21:06:48.734477 sonic NOTICE swss#orchagent: :- unbind: 100000000000c port is unbound from DATAACL ACL table
Mar  2 21:06:48.734545 sonic NOTICE swss#orchagent: :- updateAclTablePorts: Removed:Ethernet8 from portSet
Mar  2 21:06:48.734603 sonic NOTICE swss#orchagent: :- doAclTableTask: Successfully updated existing ACL table DATAACL
Mar  2 21:06:48.737609 sonic INFO caclmgrd: Issuing the following iptables commands:
Mar  2 21:06:48.738211 sonic INFO caclmgrd:   iptables -P INPUT ACCEPT
Mar  2 21:06:48.738590 sonic INFO caclmgrd:   iptables -P FORWARD ACCEPT
Mar  2 21:06:48.738969 sonic INFO caclmgrd:   iptables -P OUTPUT ACCEPT
Mar  2 21:06:48.739642 sonic INFO caclmgrd:   iptables -F
Mar  2 21:06:48.740025 sonic INFO caclmgrd:   iptables -X
Mar  2 21:06:48.740405 sonic INFO caclmgrd:   ip6tables -P INPUT ACCEPT
Mar  2 21:06:48.740767 sonic INFO caclmgrd:   ip6tables -P FORWARD ACCEPT
Mar  2 21:06:48.741128 sonic INFO caclmgrd:   ip6tables -P OUTPUT ACCEPT
Mar  2 21:06:48.741484 sonic INFO caclmgrd:   ip6tables -F
Mar  2 21:06:48.741839 sonic INFO caclmgrd:   ip6tables -X
Mar  2 21:06:48.742203 sonic INFO caclmgrd:   iptables -A INPUT -s 127.0.0.1 -i lo -j ACCEPT
Mar  2 21:06:48.742569 sonic INFO caclmgrd:   ip6tables -A INPUT -s ::1 -i lo -j ACCEPT
Mar  2 21:06:48.755328 sonic ERR syncd#syncd: [none] _brcm_sai_acl_obj_bind:5050 Invalid acl object type.
Mar  2 21:06:48.755573 sonic ERR syncd#syncd: [none] brcm_sai_set_port_attribute:476 ACL obj port bind failed with error -18.
Mar  2 21:06:48.755640 sonic ERR syncd#syncd: :- processEvent: VID: oid:0x100000000000c RID: oid:0x100000024
Mar  2 21:06:48.755701 sonic ERR syncd#syncd: :- processEvent: attr: SAI_PORT_ATTR_INGRESS_ACL: oid:0x0
Mar  2 21:06:48.755762 sonic ERR syncd#syncd: :- processEvent: failed to execute api: set, key: SAI_OBJECT_TYPE_PORT:oid:0x100000000000c, status: SAI_STATUS_INVALID_OBJECT_TYPE
Mar  2 21:06:48.755824 sonic ERR syncd#syncd: :- syncd_main: Runtime error: :- processEvent: failed to execute api: set, key: SAI_OBJECT_TYPE_PORT:oid:0x100000000000c, status: SAI_STATUS_INVALID_OBJECT_TYPE
Mar  2 21:06:48.755885 sonic NOTICE syncd#syncd: :- notify_OA_about_syncd_exception: sending switch_shutdown_request notification to OA
Mar  2 21:06:48.758038 sonic NOTICE syncd#syncd: :- notify_OA_about_syncd_exception: notification send successfull
Mar  2 21:06:48.762175 sonic NOTICE swss#orchagent: :- handle_switch_shutdown_request: switch shutdown request
Mar  2 21:06:48.773367 sonic INFO swss#supervisord: orchagent terminate called after throwing an instance of 'std::invalid_argument'
Mar  2 21:06:48.773367 sonic INFO swss#supervisord: orchagent   what():  parse error - unexpected end of input

Can you guys check and see if you're able to repro this? I was able to hit it with just this:

redis-cli -n 4 hmset "ACL_TABLE|DATAACL" "ports@" "Ethernet0,Ethernet4,Ethernet8" "type" "L3" "policy_desc" "test"
redis-cli -n 4 hmset "ACL_TABLE|DATAACL" "ports@" "Ethernet0,Ethernet4"

I can send over some more logs if you want as well. Aside from this, everything seems to be working great.

(I should specify, I used a master image from Jenkins and installed the swss package from this PR on top of that. Let me know if you're using some other build process and aren't able to reproduce the issue.)

[vasant17]

We have already hit this issue. Raised Broadcom ticket and its fixed. We have also verified the fix. Ticket: CS9616135

[daall]

Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :)

[vasant17]

Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :)

Any update on this? Please let me know, if anything missing from my end.

[daall]

Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :)

Any update on this? Please let me know, if anything missing from my end.

I think we're all good from you end. I was fighting some issues with the build system, but it's running OK now so hopefully I can get that fix in today.

[vasant17]

Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :)

Any update on this? Please let me know, if anything missing from my end.

I think we're all good from you end. I was fighting some issues with the build system, but it's running OK now so hopefully I can get that fix in today.

I have fixed the merge conflicts. Appreciate if we can push this ASAP so that I don't have to continue to merge conflicts.

[daall]

Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :)

Any update on this? Please let me know, if anything missing from my end.

I think we're all good from you end. I was fighting some issues with the build system, but it's running OK now so hopefully I can get that fix in today.

I have fixed the merge conflicts. Appreciate if we can push this ASAP so that I don't have to continue to merge conflicts.

@yxieca and I are actively working on getting the BRCM SAI updated with the patch that you mentioned. I will merge this PR as soon as that update is done.

[vasant17]

Thank you!

…

On Fri, Mar 6, 2020 at 4:55 PM Danny Allen ***@***.***> wrote: Cool, I'll go ahead and grab that change for the public 3.7 so we can close this out. :) Any update on this? Please let me know, if anything missing from my end. I think we're all good from you end. I was fighting some issues with the build system, but it's running OK now so hopefully I can get that fix in today. I have fixed the merge conflicts. Appreciate if we can push this ASAP so that I don't have to continue to merge conflicts. @yxieca <https://github.com/yxieca> and I are actively working on getting the BRCM SAI updated with the patch that you mentioned. I will merge this PR as soon as that update is done. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1148>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIWEL5UTJW6FKN337WDSIWLRGGLORANCNFSM4J2INXRA> .