Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

rck-innovium · 2023-04-14T09:37:36Z

What I did
Support a new ACL table type called L3V4V6.
This table supports both v4 and v6 Match types.
Add unit tests for this new ACL table type.
HLD: sonic-net/SONiC#1267

Signed-off-by: Ravi(Marvell) [email protected]

Why I did it
To optimise ACL TCAM usage in capable platforms.

How I verified it
Add unit tests for this new ACL table type.

sudo pytest -s test_acl_l3v4v6.py
collected 2 items

test_acl_l3v4v6.py ..

======================= 2 passed, 1 warning in 60.88s (0:01:00)

Support a new ACL table type called L3V4V6. This table supports both v4 and v6 Match types. Add unit tests for this new ACL table type. HLD: sonic-net/SONiC#1267 Signed-off-by: Ravi(Marvell) [email protected]

orchagent/aclorch.cpp

rck-innovium · 2023-05-03T17:43:43Z

@bingwang-ms I have addressed your comments and added corresponding UT as well. Please review and approve.

Add validations to allow mixing on IPv4 and IPv fields in the same ACL rule. Fix cases in the logs.

bingwang-ms · 2023-05-03T17:51:57Z

LGTM. Are you going to update acl loader to support this new type as well?

rck-innovium · 2023-05-04T04:05:36Z

LGTM. Are you going to update acl loader to support this new type as well?

Thank you, for the review. The acl-loader support is under review: sonic-net/sonic-utilities#2794

It is currently assigned to Prince and Qi Luo for review. But appreciate it if you can also review the acl-loader PR.

rck-innovium · 2023-05-08T04:28:50Z

@prsunny

The PR has been reviewed by Bing. Can you please merge before 202305 fork.

prsunny · 2023-05-12T21:05:23Z

orchagent/aclorch.cpp

@@ -3049,11 +3083,36 @@ void AclOrch::init(vector<TableConnector>& connectors, PortsOrch *portOrch, Mirr
        };
    }

+    if ( platform == MRVL_PLATFORM_SUBSTRING ||
+            platform == INVM_PLATFORM_SUBSTRING ||
+            platform == VS_PLATFORM_SUBSTRING)


Why change VS platform to combined? Does it impact any current acl tests? @bingwang-ms

I think the change to VS makes no difference. AFAIK, We don't run acl test on VS. But even though, I think it doesn't make sense to change the behavior of VS. I suggest removing the change to VS_PLATFORM. @rck-innovium

@bingwang-ms

After removing the VS platform, I ran the tests that create ACL tables of the new L3V4V6 type and saw that the ACL table creation fails.

marvell@cpss-testvm2:~/rck/sonic-dbg/sonic-buildimage/src/sonic-swss/tests$ sudo pytest -v test_acl_l3v4v6.py [sudo] password for marvell: ============================================================================================================================ test session starts ============================================================================================================================= : : test_acl_l3v4v6.py::TestAcl::test_L3V4V6AclTableCreationDeletion FAILED [ 20%] : : =================================================================================================================================== ERRORS =================================================================================================================================== ________________________________________________________________________________________________________ ERROR at setup of TestAcl.test_ValidAclRuleCreation_sip_dip _________________________________________________________________________________________________________ self = <test_acl_l3v4v6.TestAcl object at 0x7f8141afba60>, dvs_acl = <dvslib.dvs_acl.DVSAcl object at 0x7f8141a564c0> @pytest.fixture def l3v4v6_acl_table(self, dvs_acl): try: dvs_acl.create_acl_table(L3V4V6_TABLE_NAME, L3V4V6_TABLE_TYPE, L3V4V6_BIND_PORTS) > yield dvs_acl.get_acl_table_ids(1)[0] test_acl_l3v4v6.py:16:

The reason for the error is that the VS platform does NOT support the new ACL table type L3V4V6.

May 13 14:49:13.301367 e01c265b5ca5 ERR #orchagent: :- validate: Table L3_V4V6_TEST: table type L3V4V6 in stage 1 not supported on this platform. May 13 14:49:13.301645 e01c265b5ca5 ERR #orchagent: :- doAclTableTask: Failed to create ACL table L3_V4V6_TEST, invalid configuration

@prsunny

This change does not affect any other existing ACL features since this capability check is used only for the newly introduced ACL table type L3V4V6. Also, I verified that all the current swss ACL tests pass as well as PTF test_acl.py.

@rck-innovium Probably we can mock the platform by setting DVS_FAKE_PLATFORM. Please search in existing test code.
Thanks

@bingwang-ms

I was not able to find DVS_FAKE_PLATFORM in existing swss test code

marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ grep -ir DVS_FAKE_PLATFORM *
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ grep -r FAKE_PLATFORM *
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ cd ..
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src$ grep -r DVS_FAKE_PLATFORM *
marvell@cpss-testvm2:~/rck/sonic-dbg/sonic-buildimage/src$

@rck-innovium Sorry for the misleading. I just realize that the DVS_FAKE_PLATFORM has been removed from the latest testing code. I don't know if we have other method to mock the platform string. @prsunny Any suggestions?

E.g:

DVS_FAKE_PLATFORM = "marvell" fakeplatform = getattr(request.module, "DVS_FAKE_PLATFORM", None) dvs = DockerVirtualSwitch(name, keeptb, fakeplatform)

@prsunny @bingwang-ms

DVS_FAKE_PLATFORM has been removed from the entire SONiC codebase and in SWSS it has been replaced with DVS_ENV.

I digged through the past versions of SONiC to find that Nvidia has changed DVS_FAKE_PLATFORM to DVS_ENV = ["HWSKU=Mellanox-SN2700"]. The reason I presume is to support mimicking at a deeper HwSKU level instead of just the platform.

However, I see in today’s swss/tests, only Mellanox-SN2700 SKU is being faked. In fact, I ran into several issues when faking Marvell and Innovium platforms.
I have raised PR #2785 to track the issues.

@prsunny
Coming back to the original question, the new checks do NOT affect any other feature since this capability check is only used by the new L3V4V6 ACL table feature.
So adding VS platform does not have any impact on any of the existing feature.

…onic-net#2735) * Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267

rck-innovium requested a review from prsunny as a code owner April 14, 2023 09:37

rck-innovium mentioned this pull request Apr 14, 2023

Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net/SONiC#1267

Merged

prsunny requested a review from bingwang-ms April 14, 2023 20:16

Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267

f4062e8

Support a new ACL table type called L3V4V6. This table supports both v4 and v6 Match types. Add unit tests for this new ACL table type. HLD: sonic-net/SONiC#1267 Signed-off-by: Ravi(Marvell) [email protected]

rck-innovium force-pushed the swss_acl_l3v4v6 branch from e0a138a to f4062e8 Compare April 22, 2023 18:37

bingwang-ms reviewed May 1, 2023

View reviewed changes

orchagent/aclorch.cpp Show resolved Hide resolved

bingwang-ms reviewed May 1, 2023

View reviewed changes

orchagent/aclorch.cpp Outdated Show resolved Hide resolved

rck-innovium requested a review from bingwang-ms May 3, 2023 17:15

Address review comments

456bbfe

Add validations to allow mixing on IPv4 and IPv fields in the same ACL rule. Fix cases in the logs.

rck-innovium force-pushed the swss_acl_l3v4v6 branch from b9b47c8 to 456bbfe Compare May 3, 2023 17:49

bingwang-ms approved these changes May 3, 2023

View reviewed changes

prsunny reviewed May 12, 2023

View reviewed changes

rck-innovium requested a review from prsunny May 14, 2023 06:38

prsunny merged commit 1b1c10a into sonic-net:master May 25, 2023

theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 20, 2023

Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267 (s…

7903721

…onic-net#2735) * Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

rck-innovium commented Apr 14, 2023

rck-innovium commented May 3, 2023

bingwang-ms commented May 3, 2023

rck-innovium commented May 4, 2023

rck-innovium commented May 8, 2023

prsunny May 12, 2023

bingwang-ms May 12, 2023

rck-innovium May 14, 2023

bingwang-ms May 15, 2023

rck-innovium May 16, 2023

bingwang-ms May 22, 2023

prsunny May 22, 2023

rck-innovium May 22, 2023

rck-innovium May 22, 2023

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

Conversation

rck-innovium commented Apr 14, 2023

rck-innovium commented May 3, 2023

bingwang-ms commented May 3, 2023

rck-innovium commented May 4, 2023

rck-innovium commented May 8, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment