Add mandatory ACL actions when creating mirror ACL table #2205

ysmanman · 2022-03-28T04:40:04Z

What I did
Add mandatory ACL actions when creating mirror ACL table

Why I did it
BRCM SAI requires to pass ACL actions when creating mirror ACL table.

How I verified it
Loaded image on switch and checked SAI redis record to make sure counter & mirror_ingress actions are passed to SAI in mirror ACL creatation.

Details if related

bingwang-ms · 2022-04-14T03:27:20Z

orchagent/aclorch.cpp

+        return true;
+    }
+
+    if (type.getName() == TABLE_TYPE_MIRROR || type.getName() == TABLE_TYPE_MIRRORV6)


Please also include TABLE_TYPE_MIRROR_DSCP

Thanks. Done.

bingwang-ms · 2022-04-14T03:29:35Z

@stepanblyschak Could you help take a look?

arlakshm · 2022-04-21T19:57:20Z

/Azp run Azure.sonic-swss

azure-pipelines · 2022-04-21T19:57:30Z

Azure Pipelines successfully started running 1 pipeline(s).

arlakshm · 2022-04-21T20:13:12Z

/Azp run Azure.sonic-swss

azure-pipelines · 2022-04-21T20:13:19Z

Azure Pipelines successfully started running 1 pipeline(s).

arlakshm · 2022-04-21T20:14:02Z

@ysmanman, can you resolve the open comment

arlakshm · 2022-04-21T21:13:15Z

orchagent/aclorch.cpp

+bool AclTableType::addAction(sai_acl_action_type_t action)
+{
+    m_aclAcitons.insert(action);
+    returne true;


@arlakshm Thanks for catching this. Fixed the typo.

ysmanman · 2022-04-23T17:34:01Z

@ysmanman, can you resolve the open comment

@arlakshm Sorry for the delay. Resolved the comment.

bingwang-ms · 2022-04-24T01:39:49Z

/azp run

azure-pipelines · 2022-04-24T01:39:58Z

Azure Pipelines successfully started running 1 pipeline(s).

ysmanman · 2022-05-05T02:53:31Z

@arlakshm @bingwang-ms I added test coverage as requested. Please take a look at it.

arlakshm · 2022-05-06T23:04:18Z

/Azp run Azure.sonic-swss

azure-pipelines · 2022-05-06T23:04:28Z

Azure Pipelines successfully started running 1 pipeline(s).

bingwang-ms · 2022-05-13T07:59:20Z

/azp run

azure-pipelines · 2022-05-13T07:59:29Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2022-05-17T11:21:56Z

orchagent/aclorch.cpp

@@ -301,6 +301,12 @@ const set<sai_acl_action_type_t>& AclTableType::getActions() const
    return m_aclAcitons;
 }

+bool AclTableType::addAction(sai_acl_action_type_t action)


AclTableType was designed to be an immutable data structure, so you cannot change it after it has been created. This is due to the fact that matches, actions, bind point types are CREATE_ONLY SAI attributes. Adding mutable methods breaks this invariant. An object of type AclTableType may have been used to create tables already and changing the definition of a table type may cause divergence between software and hardware state.
Instead, a builder AclTableTypeBuilder is used to create AclTableTypes.
Why not adding neccessary actions in initDefaultTableTypes - https://github.com/Azure/sonic-swss/blob/master/orchagent/aclorch.cpp#L2895 ?

AclTableType was designed to be an immutable data structure, so you cannot change it after it has been created. This is due to the fact that matches, actions, bind point types are CREATE_ONLY SAI attributes. Adding mutable methods breaks this invariant. An object of type AclTableType may have been used to create tables already and changing the definition of a table type may cause divergence between software and hardware state. Instead, a builder AclTableTypeBuilder is used to create AclTableTypes. Why not adding neccessary actions in initDefaultTableTypes - https://github.com/Azure/sonic-swss/blob/master/orchagent/aclorch.cpp#L2895 ?

We also saw similar issue on broadcom paltform. Please see sonic-net/sonic-buildimage#10425.
We may have two options to workaround this issue

Hardcode is_mandatory to False for the general ACL type, such as L3, L3V6

Add default action list for these ACL tables
What do you think?

Ok, I see, so the issue may be due to SAI telling that action list is mandatory even if it is not (because it worked without actions before ACL table types changes). In this case, hardcoding is_mandatory to be False for the types we know should work without explicitly passing action list should be a simpler workaround. Put a comment that this hardcode needs to be removed once SAI is fixed.

Based on our communication with broadcom, action list is mandatory when creating mirror acl table. Otherwise, SAI acl table creation will fail. Can you elaborate how would hardcoding is_mandatory to be False work around the SAI limitation?

I think the easiest change may be bypass the below check for the known ACL types
https://github.com/Azure/sonic-swss/blob/bbbd5f44f2c55808785672177e44527f635204d6/orchagent/aclorch.cpp#L1881-L1888

@stepanblyschak Mirroring probably worked in fixed box before. My PR is to get mirroring work in broadcom VOQ chassis.

@ysmanman
Thanks for the update. Could you clarify why SAI call will fail if we bypass this check? Before this change, there is no check for action list existance. Is it a new feature of SAI?

@bingwang-ms @stepanblyschak Let me provide some context of this PR. We were testing mirroring in broadcom DNX or VOQ chassis with broadcom SAI 5.2 and observed orchagent failed to add ACL rule in mirroring ACL table. The failure is because the ACL actions is not provided at the time of mirror ACL table creation. Broadcom DNX devices need ACL action list at the time of ACL table creation. To address this issue, this PR adds required ACL actions to mirroring type ACL table if the action list is mandatory on table creation.

Thanks @ysmanman .
I saw similar issues when testing on Broadcom XGS devices. Please see sonic-net/sonic-buildimage#10425.
Per your clarification, the action_list for ACL table is a must when we creating ACL table now? If that's the case, my by-pass solution will not work. The solution in this PR makes sense. We have to do similar change for other ACL table types (L3, L3V6, and etc.)

@ysmanman @bingwang-ms Thanks for explanation. This PR makes sense. I noticed you do not modify the acl table type in m_aclTableTypes but a copy in AclTable. That is fine to me. You can mark this as resolved

bingwang-ms · 2022-05-26T03:48:15Z

/azp run

azure-pipelines · 2022-05-26T03:48:24Z

Azure Pipelines successfully started running 1 pipeline(s).

arlakshm · 2022-05-26T06:10:17Z

/Azp run

azure-pipelines · 2022-05-26T06:10:27Z

Azure Pipelines successfully started running 1 pipeline(s).

bingwang-ms · 2022-05-27T02:52:28Z

@ysmanman Can you add some vstest case (in python) to cover the change? As far as I know, the C++ UT is not caculated as coverage. Thanks

bingwang-ms · 2022-05-27T06:55:16Z

The change is verified on 7050cx3 device

collected 10 items                                                                                                                                                                                                                                             

everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_basic_forwarding[cli-downstream] PASSED                                                                                                                          [ 10%] ^H
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_basic_forwarding[cli-upstream] PASSED                                                                                                                            [ 20%] ^H
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_neighbor_mac_change[cli-downstream] PASSED                                                                                                                       [ 30%]
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_neighbor_mac_change[cli-upstream] PASSED                                                                                                                         [ 40%] ^H
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_remove_unused_ecmp_next_hop[cli-downstream] PASSED                                                                                                               [ 50%] ^H
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_remove_unused_ecmp_next_hop[cli-upstream] PASSED                                                                                                                 [ 60%]
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_remove_used_ecmp_next_hop[cli-downstream]  ^HPASSED                                                                                                                 [ 70%]
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_remove_used_ecmp_next_hop[cli-upstream] PASSED                                                                                                                   [ 80%] ^H
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_dscp_with_policer[cli-downstream] PASSED                                                                                                                         [ 90%]
everflow/test_everflow_testbed.py::TestEverflowV4IngressAclIngressMirror::test_everflow_dscp_with_policer[cli-upstream]  ^HPASSED                                                                                                                           [100%]

bingwang-ms · 2022-05-27T09:07:55Z

@ysmanman The change has been verified. I have raised another PR #2298 in the same way as you did to fix other ACL table types. Changes in this PR is included. Could you help take a look? Thanks

ysmanman · 2022-05-27T16:15:50Z

@ysmanman The change has been verified. I have raised another PR #2298 in the same way as you did to fix other ACL table types. Changes in this PR is included. Could you help take a look? Thanks

@bingwang-ms Sure, I will take a look. Since your PR includes the change in my PR, I will close my PR once your PR is merged. Thanks.

What I did This PR is derived from #2205 Fix sonic-net/sonic-buildimage#10425 We were seeing ACL table creation failure on some platform because action_list is mandatory, while the action_list is not provided by aclorch. Apr 1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table DATAACL is mandatory Apr 1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table DATAACL, invalid configuration Apr 1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOW is mandatory Apr 1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOW, invalid configuration Apr 1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOWV6 is mandatory Apr 1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOWV6, invalid configuration This PR fixed the issue by adding default action_list to the default ACL table type if not present. Why I did it Fix the ACL table creation issue. How I verified it Verified by running test_acl and test_everflow on Broadcom TD3 platform Signed-off-by: bingwang <[email protected]> Co-authored-by: syuan <[email protected]>

arlakshm · 2022-05-31T20:57:57Z

Hi @ysmanman, #2298, is merged now. Can you please close this PR if your changes are all present in #2298

ysmanman · 2022-05-31T21:06:21Z

Hi @ysmanman, #2298, is merged now. Can you please close this PR if your changes are all present in #2298

Hi @arlakshm, yes, my changes is included. I am closing the PR.

) What I did This PR is derived from sonic-net#2205 Fix sonic-net/sonic-buildimage#10425 We were seeing ACL table creation failure on some platform because action_list is mandatory, while the action_list is not provided by aclorch. Apr 1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table DATAACL is mandatory Apr 1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table DATAACL, invalid configuration Apr 1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOW is mandatory Apr 1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOW, invalid configuration Apr 1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOWV6 is mandatory Apr 1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOWV6, invalid configuration This PR fixed the issue by adding default action_list to the default ACL table type if not present. Why I did it Fix the ACL table creation issue. How I verified it Verified by running test_acl and test_everflow on Broadcom TD3 platform Signed-off-by: bingwang <[email protected]> Co-authored-by: syuan <[email protected]>

Add mandatory ACL actions when creating mirror ACL table

2f38e3d

ysmanman requested a review from prsunny as a code owner March 28, 2022 04:40

bingwang-ms reviewed Apr 14, 2022

View reviewed changes

bingwang-ms requested a review from stephenxs April 14, 2022 03:28

bingwang-ms removed the request for review from stephenxs April 14, 2022 03:29

arlakshm reviewed Apr 21, 2022

View reviewed changes

Add acl action to TABLE_TYPE_MIRROR_DSCP and also fix typo

a4fb759

bingwang-ms previously approved these changes Apr 24, 2022

View reviewed changes

ysmanman mentioned this pull request May 2, 2022

Add mandatory action list to mirror ACL table #2162

Closed

Add test coverage.

9b1b9bb

ysmanman dismissed bingwang-ms’s stale review via 9b1b9bb May 5, 2022 02:51

stepanblyschak reviewed May 17, 2022

View reviewed changes

stepanblyschak approved these changes May 25, 2022

View reviewed changes

bingwang-ms approved these changes May 26, 2022

View reviewed changes

bingwang-ms mentioned this pull request May 27, 2022

[ACL] Add default action_list for default ACL table type #2298

Merged

ysmanman closed this May 31, 2022

Add mandatory ACL actions when creating mirror ACL table #2205

Add mandatory ACL actions when creating mirror ACL table #2205

Conversation

ysmanman commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingwang-ms commented Apr 14, 2022

arlakshm commented Apr 21, 2022

azure-pipelines bot commented Apr 21, 2022

arlakshm commented Apr 21, 2022

azure-pipelines bot commented Apr 21, 2022

arlakshm commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysmanman commented Apr 23, 2022

bingwang-ms commented Apr 24, 2022

azure-pipelines bot commented Apr 24, 2022

ysmanman commented May 5, 2022

arlakshm commented May 6, 2022

azure-pipelines bot commented May 6, 2022

bingwang-ms commented May 13, 2022

azure-pipelines bot commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingwang-ms May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingwang-ms May 20, 2022 • edited Loading

Choose a reason for hiding this comment

stepanblyschak May 25, 2022 • edited Loading

Choose a reason for hiding this comment

bingwang-ms commented May 26, 2022

azure-pipelines bot commented May 26, 2022

arlakshm commented May 26, 2022

azure-pipelines bot commented May 26, 2022

bingwang-ms commented May 27, 2022

bingwang-ms commented May 27, 2022

bingwang-ms commented May 27, 2022

ysmanman commented May 27, 2022

arlakshm commented May 31, 2022

ysmanman commented May 31, 2022

bingwang-ms May 20, 2022 •

edited

Loading

bingwang-ms May 20, 2022 •

edited

Loading

stepanblyschak May 25, 2022 •

edited

Loading