[AclOrch] aclOrch enhancement to handle LAG port not configured case by keboliu · Pull Request #494 · sonic-net/sonic-swss

keboliu · 2018-05-04T09:47:25Z

What I did

Add STATE_DB db connector to Orchdaemon so AclOrch can use it.
AclOrch subscribe to STATE_DB and handle the LAG add/del notificaiton from STATE_DB.
When AclOrch detected a LAG port in it's port list not configured yet, it will continue to handle the rest ports in the list and add this LAG port to the pending port list.
When AclOrch received the LAG port created notification, and this LAG is in it's pending list, it will bind the ACL table to this LAG.
If one LAG deleted, the related ACL table that have this LAG, will add this LAG to the pending port list.

Why I did it
it's possible that LAG port configured later or not configured yet while ACL table created, in this case we can add this LAG to the pending list and wait for it created, instead of fail the whole ACL table creating.

How I verified it
manual test, run ACL and Everflow test case on T1 and T1-LAG topo.

Details if related

merge from Azure/sonic-swss master

qiluo-msft · 2018-05-05T01:04:16Z

orchagent/main.cpp

@@ -249,8 +249,9 @@ int main(int argc, char **argv)
    /* Initialize orchestration components */
    DBConnector *appl_db = new DBConnector(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
    DBConnector *config_db = new DBConnector(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);


new [](start = 29, length = 3)

There are memory leak risk at extreme memory situation. #Closed

@qiluo-msft Hi Qi, could you elaborate the concern here? #Closed

There are several 'new' statements here. If memory is used up in the middle, there will be memory leak issue.
The bug should be here for a long time, and your added code make it worse a little bit.

In reply to: 186248077 [](ancestors = 186248077)

qiluo-msft · 2018-05-05T01:07:34Z

orchagent/aclorch.h

    map<string, shared_ptr<AclRule>> rules;
+    // Set to store the ACL table port alias 
+	set<string> portListsSet;
+	// Set tje store the not cofigured ACL table port alias


typo #Closed

qiluo-msft · 2018-05-05T04:24:44Z

orchagent/aclorch.cpp

+        }
+        else
+        {
+            it = consumer.m_toSync.erase(it);


it = consumer.m_toSync.erase(it); [](start = 12, length = 33)

erase is common for all cases, so you can do it outside the if-else. #Closed

qiluo-msft · 2018-05-05T04:27:08Z

orchagent/aclorch.cpp

    split(portsList, strList, ',');

    set<string> strSet(strList.begin(), strList.end());
+    aclTable.portListsSet = strSet;


portListsSet [](start = 13, length = 12)

Call it portSet? #Closed

qiluo-msft · 2018-05-05T04:30:07Z

orchagent/aclorch.cpp

+    return true;
+}
+
 bool AclOrch::processAclTableType(string type, acl_table_type_t &table_type)


processAclTableType [](start = 14, length = 19)

Lots of repeated code, you can call one from the other. #Closed

Sorry, I mean processPorts() and processPendingPort() share some code, it is possible to call one from the other?

In reply to: 186252087 [](ancestors = 186252087)

@qiluo-msft They do share some same code, I also have considered calling processPendingPort(previously processSinglePort) in processPorts. But if we look into the whole procedure, processPorts only "link" ports to tables and the action to "bind" table to ports are not included.
For processPendingPort, it's dedicated to receiving the notification and handle the pending port, in this one function "link" and "bind" all performed. To keep the code more straightforward and readable I decide to have these two functions separated through will have some slight redundant code.

you can move aclTable.bind out of the processPendingPort() function and make the code reuse.

you can do aclTable.bind in doAclTablePortUpdateTask().

In reply to: 186911317 [](ancestors = 186911317)

qiluo-msft · 2018-05-05T04:35:32Z

orchagent/aclorch.cpp

+            {
+                auto table = itmap.second;
+
+                if (table.pendingPortListSet.find(port_alias) != table.pendingPortListSet.end())


pendingPortListSet [](start = 26, length = 18)

Call it pendingPortSet? #Closed

qiluo-msft

As comments.

keboliu · 2018-05-08T11:28:29Z

@qiluo-msft all comments handled, please check the latest commit. #Closed

qiluo-msft

As comments.

prsunny · 2018-05-08T23:49:52Z

orchagent/main.cpp

+    DBConnector config_db(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
+    DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);

-    OrchDaemon *orchDaemon = new OrchDaemon(appl_db, config_db);


I don't think we would need to have orchDaemon allocate from stack. This could still be retained as it is (allocate via new) so we wouldn't have to worry about the class holding more data in future just in case.

prsunny · 2018-05-09T00:25:32Z

orchagent/aclorch.cpp

+
+                if (table.pendingPortSet.find(port_alias) != table.pendingPortSet.end())
+                {
+                    SWSS_LOG_NOTICE("found the port: %s in ACL table: %s pending port list, bind it to ACL table.", port_alias.c_str(), table.description.c_str());


You may want to put this as INFO instead of NOTICEs?

prsunny · 2018-05-09T00:33:04Z

orchagent/aclorch.cpp

+            for (auto itmap : m_AclTables)
+            {
+                auto table = itmap.second;
+


Can you remove the extra lines. There are few other redundant lines as well in the PR.

prsunny · 2018-05-09T01:30:40Z

orchagent/aclorch.cpp


    SWSS_LOG_INFO("%s table %s to ports", bind ? "Bind" : "Unbind", aclTable.id.c_str());
+
+    if (aclTable.portSet.empty())


I don't think this condition will be ever true in this flow. Can you check?

you are correct, I removed this flow.

prsunny · 2018-05-09T01:39:46Z

orchagent/aclorch.cpp

-        {
-            return SAI_STATUS_SUCCESS;
-        }
+        SWSS_LOG_WARN("Binding port list is empty for %s table", aclTable.id.c_str());


It could be a normal flow to delete aclTable which is not yet bind to any port list. We wouldn't want to log warning if it is in the delete flow.

condition check added.

qiluo-msft

Please also get Prince's approval.

prsunny

Changes looks good. Could you please add/update VS test cases for ACL to verify this changes?

liatgrozovik · 2018-05-10T08:18:29Z

As for VS, it should be added but for now we have the testbed to cover this flow.
Lets please approve the change and we will add new VS cases for port channel.
It is currently don't have any reference to port channel although it was supported before.

lguohan

need vs switch test.

keboliu · 2018-05-22T12:51:09Z

3 new VS test case added

negative test: bind acl table to lag member port
bind acl table to lag
bing acl table to non-exist lag and then configure this lag.

root@arc-host74-006:/sonic-swss/tests# pytest -v --dvsname=vs test_acl.py
=================== test session starts =====================
platform linux2 -- Python 2.7.12, pytest-3.3.0, py-1.5.2, pluggy-0.6.0 -- /usr/bin/python
cachedir: .cache
rootdir: /.autodirect/rdmzsysgwork/kebol/sonic-2/sonic-buildimage/src/sonic-swss/tests, inifile:
collected 19 items                                                                                                                                                                                                                         

test_acl.py::TestAcl::test_AclTableCreation PASSED                                                                                                                                                                                   [  5%]
test_acl.py::TestAcl::test_AclRuleL4SrcPort PASSED                                                                                                                                                                                   [ 10%]
test_acl.py::TestAcl::test_AclTableDeletion PASSED                                                                                                                                                                                   [ 15%]
test_acl.py::TestAcl::test_V6AclTableCreation PASSED                                                                                                                                                                                 [ 21%]
test_acl.py::TestAcl::test_V6AclRuleIPv6Any PASSED                                                                                                                                                                                   [ 26%]
test_acl.py::TestAcl::test_V6AclRuleIPv6AnyDrop PASSED                                                                                                                                                                               [ 31%]
test_acl.py::TestAcl::test_V6AclRuleIpProtocol PASSED                                                                                                                                                                                [ 36%]
test_acl.py::TestAcl::test_V6AclRuleSrcIPv6 PASSED                                                                                                                                                                                   [ 42%]
test_acl.py::TestAcl::test_V6AclRuleDstIPv6 PASSED                                                                                                                                                                                   [ 47%]
test_acl.py::TestAcl::test_V6AclRuleL4SrcPort PASSED                                                                                                                                                                                 [ 52%]
test_acl.py::TestAcl::test_V6AclRuleL4DstPort PASSED                                                                                                                                                                                 [ 57%]
test_acl.py::TestAcl::test_V6AclRuleTCPFlags PASSED                                                                                                                                                                                  [ 63%]
test_acl.py::TestAcl::test_V6AclRuleL4SrcPortRange PASSED                                                                                                                                                                            [ 68%]
test_acl.py::TestAcl::test_V6AclRuleL4DstPortRange PASSED                                                                                                                                                                            [ 73%]
test_acl.py::TestAcl::test_V6AclTableDeletion PASSED                                                                                                                                                                                 [ 78%]
test_acl.py::TestAcl::test_InsertAclRuleBetweenPriorities PASSED                                                                                                                                                                     [ 84%]
test_acl.py::TestAcl::test_AclTableCreationOnLAGMember PASSED                                                                                                                                                                        [ 89%]
test_acl.py::TestAcl::test_AclTableCreationOnLAG PASSED                                                                                                                                                                              [ 94%]
test_acl.py::TestAcl::test_AclTableCreationBeforeLAG PASSED

lguohan · 2018-05-23T11:20:40Z

orchagent/aclorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    sai_object_id_t port_id;


sai_object_id_t port_id == SAI_NULL_OBJECT_ID; do it here. then you do not need to assign later in the function.

lguohan · 2018-05-23T11:20:48Z

orchagent/aclorch.cpp

+        if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
+        {
+            SWSS_LOG_ERROR("Failed to process port. Bind table to LAG member %s is not allowed", alias.c_str());
+            port_id = SAI_NULL_OBJECT_ID;


port_id = SAI_NULL_OBJECT_ID; [](start = 12, length = 29)

remove this.

lguohan · 2018-05-23T11:45:55Z

orchagent/aclorch.cpp

+
    vector<string> strList;

    SWSS_LOG_INFO("Processing ACL table port list %s", portsList.c_str());


SWSS_LOG_INFO(" [](start = 4, length = 15)

this is debug level message.

lguohan · 2018-05-23T11:46:47Z

orchagent/aclorch.cpp

        {
-            SWSS_LOG_ERROR("Failed to process port. Port %s doesn't exist", alias.c_str());
-            return false;
+            SWSS_LOG_WARN("Port %s not configured yet, add it to ACL table %s pending list", alias.c_str(), aclTable.description.c_str());


SWSS_LOG_WARN [](start = 12, length = 13)

why warning level? this should be info level since this is a common situation your code is handling, so this should not be warning level.

lguohan · 2018-05-23T11:46:58Z

orchagent/aclorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    SWSS_LOG_INFO("Processing ACL table port %s", portAlias.c_str());


SWSS_LOG_INFO [](start = 4, length = 13)

debug level.

lguohan · 2018-05-23T11:47:28Z

orchagent/aclorch.cpp

+    Port port;
+    if (!gPortsOrch->getPort(portAlias, port))
+    {
+        SWSS_LOG_WARN("Port %s not configured yet, add it to ACL table %s pending list", portAlias.c_str(), aclTable.description.c_str());


SWSS_LOG_WARN [](start = 8, length = 13)

info level.

lguohan · 2018-05-23T11:52:52Z

tests/test_acl.py

+        self.verify_acl_group_member(adb, 0, test_acl_table_id)
+
+        # check port binding
+        self.verify_acl_lag_binding(adb, 0)


move this after port channel member creation, before state db creation.

lguohan · 2018-05-23T11:55:29Z

tests/test_acl.py

+        keys = atbl.getKeys()
+        assert len(keys) == 0
+
+    def verify_acl_group(self, adb, expt):


expt [](start = 36, length = 4)

rename to "acl_group_num" for clarity.

rename to "acl_group_num" for clarity.

In reply to: 190217928 [](ancestors = 190217928)

already changed?

lguohan · 2018-05-23T12:02:50Z

tests/test_acl.py

+        self.verify_acl_group(adb, 2)
+
+        # check acl table group member
+        self.verify_acl_group_member(adb, 2, test_acl_table_id)


verify_acl_group_member [](start = 13, length = 23)

this function signature should be

verify_acl_group_member(adb, [acl_group_ids], acl_table_id)

it should verify the acl_groups have corresponding acl_group_member which has the acl_table_id.

lguohan · 2018-05-23T12:10:44Z

tests/test_acl.py

        assert set(port_groups) == set(acl_table_groups)
-
+
+    def verify_acl_lag_binding(self, adb, expt):


verify_acl_lag_binding(self, adb, expt): [](start = 8, length = 40)

like the verifying the port binding, we need to give the lag name as input.

I didn't find out how a lag port name
was mapped to a SAI_OBJECT_TYPE_LAG_MEMBER in ASIC DB, unless you create one LAG and then know that the only SAI_OBJECT_TYPE_LAG_MEMBER record in ASIC DB is yours?

use lag_object_ids here instead of lag name.

In reply to: 190546287 [](ancestors = 190546287)

lguohan · 2018-05-23T12:11:45Z

tests/test_acl.py

-
-        # check port binding 
+
+    def verify_acl_port_binding(self, dvs, adb, expt, bind_ports):


expt [](start = 48, length = 4)

you do not need give expt here, just use the len(bind_ports)

lguohan · 2018-05-23T12:12:07Z

tests/test_acl.py

        assert set(port_groups) == set(acl_table_groups)
-
+
+    def verify_acl_lag_binding(self, adb, expt):


expt [](start = 42, length = 4)

change to bind_ports as well, do not need expt here.

for lag case it different with port case, bind_ports len may not equal to the binding lag number if some LAG not configured yet, so I think we still need a expt?

lguohan · 2018-05-23T12:12:42Z

tests/test_acl.py

-
-        # check acl table group member
+
+    def verify_acl_group_member(self, adb, expt, test_acl_table_id):


expt [](start = 43, length = 4)

change to acl_group_id list.

lguohan · 2018-05-24T18:29:49Z

orchagent/aclorch.h

+    bool processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter);
+    bool processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter);
    bool validateAclTable(AclTable &aclTable);
+    sai_object_id_t getValidPortId(string alias, Port port);


getValidPortId [](start = 20, length = 14)

to reflect the true meaning, it is better to rename it to getBindPortId. Besides, it makes more sense to move this into portsyncd since it is retrieving a port property.

renamed and move it to portOrch.

lguohan · 2018-05-25T07:32:41Z

orchagent/portsorch.cpp

+            break;
+        default:
+          SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type);
+          return false;


align spaces

lguohan · 2018-05-25T09:05:10Z

tests/test_acl.py

+        self.verify_acl_group_member(adb, acl_group_ids, test_acl_table_id)
+
+        # check port binding
+        self.verify_acl_lag_binding(adb, 1)


[](start = 40, length = 3)

use lag id

lguohan

…ed ACL table configuration (#1712) * Fix minigraph parser issue when handling LAG related ACL table configuration * rephrase the warning message. * pick up swss change in sonic-net/sonic-swss#494

…494) * enhance acl orch to handle the LAG port not configured case * rename variable, function; redunce redundant code; fix memory issue * revise according to comments * one more blank line * add new VS test cases for acl on LAG * update with PR comments fix * rename getValidPortId and move it to portOrch * fix indent * fix test case comments

…ed ACL table configuration (sonic-net#1712) * Fix minigraph parser issue when handling LAG related ACL table configuration * rephrase the warning message. * pick up swss change in sonic-net/sonic-swss#494

…ed ACL table configuration (#1712) * Fix minigraph parser issue when handling LAG related ACL table configuration * rephrase the warning message. * pick up swss change in sonic-net/sonic-swss#494

Signed-off-by: Wenda Ni <[email protected]>

…onic-net#494) * enhance acl orch to handle the LAG port not configured case * rename variable, function; redunce redundant code; fix memory issue * revise according to comments * one more blank line * add new VS test cases for acl on LAG * update with PR comments fix * rename getValidPortId and move it to portOrch * fix indent * fix test case comments

keboliu added 3 commits January 12, 2018 11:39

Merge pull request #1 from Azure/master

3bf6269

merge from Azure/sonic-swss master

Merge branch 'master' of https://github.com/Azure/sonic-swss

bf93016

enhance acl orch to handle the LAG port not configured case

f2d3035

lguohan requested a review from prsunny May 5, 2018 00:15

qiluo-msft reviewed May 5, 2018

View reviewed changes

qiluo-msft requested changes May 5, 2018

View reviewed changes

rename variable, function; redunce redundant code; fix memory issue

032e862

qiluo-msft reviewed May 8, 2018

View reviewed changes

prsunny reviewed May 9, 2018

View reviewed changes

qiluo-msft approved these changes May 9, 2018

View reviewed changes

keboliu added 2 commits May 9, 2018 10:50

revise according to comments

c6dcd31

one more blank line

de11d51

prsunny approved these changes May 10, 2018

View reviewed changes

liatgrozovik mentioned this pull request May 10, 2018

[minigraph parser] Fix minigraph parser issue when handling LAG related ACL table configuration sonic-net/sonic-buildimage#1609

Merged

lguohan requested changes May 10, 2018

View reviewed changes

keboliu mentioned this pull request May 15, 2018

[minigraph parser] Fix minigraph parser issue when handling LAG related ACL table configuration sonic-net/sonic-buildimage#1712

Merged

keboliu added 3 commits May 20, 2018 06:44

Merge branch 'master' of https://github.com/Azure/sonic-swss

d32c1c1

Merge branch 'master' into acl-sequence

c670c3b

add new VS test cases for acl on LAG

3e56f49

qiluo-msft approved these changes May 22, 2018

View reviewed changes

lguohan reviewed May 23, 2018

View reviewed changes

update with PR comments fix

de198bd

lguohan reviewed May 24, 2018

View reviewed changes

rename getValidPortId and move it to portOrch

be203be

lguohan reviewed May 25, 2018

View reviewed changes

fix indent

0499f41

lguohan reviewed May 25, 2018

View reviewed changes

fix test case comments

879d7fe

lguohan approved these changes May 25, 2018

View reviewed changes

qiluo-msft merged commit 4df9c28 into sonic-net:master May 25, 2018

keboliu mentioned this pull request May 26, 2018

[virtual switch] Does virtual switch support LAG ? sonic-net/sonic-buildimage#1715

Closed

keboliu deleted the acl-sequence branch August 22, 2018 10:28

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Fix TODO to get/set active ports only (sonic-net#494)

b7b1070

Signed-off-by: Wenda Ni <[email protected]>

jianyuewu pushed a commit to jianyuewu/sonic-swss that referenced this pull request Dec 24, 2025

added missing headers for building with gcc-10.3 (sonic-net#494)

27dbcea


		SWSS_LOG_INFO("%s table %s to ports", bind ? "Bind" : "Unbind", aclTable.id.c_str());

		if (aclTable.portSet.empty())


		vector<string> strList;

		SWSS_LOG_INFO("Processing ACL table port list %s", portsList.c_str());

		assert set(port_groups) == set(acl_table_groups)


		def verify_acl_lag_binding(self, adb, expt):


		# check port binding

		def verify_acl_port_binding(self, dvs, adb, expt, bind_ports):


		# check acl table group member

		def verify_acl_group_member(self, adb, expt, test_acl_table_id):

Conversation

keboliu commented May 4, 2018

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keboliu May 8, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keboliu May 9, 2018 • edited by lguohan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

keboliu commented May 8, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

liatgrozovik commented May 10, 2018

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

qiluo-msft May 5, 2018 •

edited

Loading

keboliu May 8, 2018 •

edited by qiluo-msft

Loading

qiluo-msft May 5, 2018 •

edited

Loading

qiluo-msft May 5, 2018 •

edited

Loading

qiluo-msft May 5, 2018 •

edited

Loading

qiluo-msft May 5, 2018 •

edited

Loading

keboliu May 9, 2018 •

edited by lguohan

Loading

qiluo-msft May 5, 2018 •

edited

Loading

keboliu commented May 8, 2018 •

edited by qiluo-msft

Loading