[inner hashing] extend inner hashing test with dynamic Policy Base Hashing configurations by AntonHryshchuk · Pull Request #4078 · sonic-net/sonic-mgmt

AntonHryshchuk · 2021-08-19T06:38:47Z

Description of PR

Summary:
extending of inner hashing test with dynamic Policy Base Hashing configurations.

Include 4 parameterized tests:
outer_ipver - inner_ipver
ipv4 - ipv4
ipv6 - ipv4
ipv6 - ipv4
ipv6 - ipv6

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

201911

Approach

What is the motivation for this PR?

Validation of PBH feature

How did you verify/test it?

The test executed on different MLNX platforms, supported the PBH feature

Supported testbed topology if it's a new test case?

t0

Documentation

https://github.com/anish-n/SONiC/blob/c27b933b0b3e7a13ebabc7b294ae15f7e5c6340e/doc/ecmp/inner_packet_hashing_test_plan.md

sonic-net/SONiC#824

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

liat-grozovik · 2021-08-25T18:47:36Z

@anish-n please help to review or assign a reviewer.
@AntonHryshchuk please check failures

liat-grozovik · 2021-09-09T08:38:01Z

/azp run

azure-pipelines · 2021-09-09T08:38:10Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2021-09-09T08:38:47Z

@anish-n kindly reminder, please review or assign someone.
@nazariig please also review

nazariig · 2021-09-09T10:14:28Z

tests/ecmp/test_inner_hashing.py

+    "inner_src_ipv4": {"field": "INNER_SRC_IPV4", "sequence": "3", "mask": "255.255.255.255"},
+    "inner_dst_ipv4": {"field": "INNER_DST_IPV4", "sequence": "3", "mask": "255.255.255.255"},
+    "inner_src_ipv6": {"field": "INNER_SRC_IPV6", "sequence": "4", "mask": "ffff:ffff::"},
+    "inner_dst_ipv6": {"field": "INNER_DST_IPV6", "sequence": "4", "mask": "ffff:ffff::"}


@AntonHryshchuk this mask doesn't fit the IPv6 address range, so load balancing won't be working:

SRC_IPV6_RANGE = ['20D0:A800:0:00::', '20D0:A800:0:00::FFFF'] DST_IPV6_RANGE = ['20D0:A800:0:01::', '20D0:A800:0:01::FFFF']

Please fix the test logic.

the ipv6 mask changed to "::ffff:ffff"

@anish-n please have a look

nazariig · 2021-09-09T10:14:55Z

@anish-n kindly reminder, please review or assign someone.
@nazariig please also review

@liat-grozovik done

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

AntonHryshchuk · 2021-09-10T05:39:30Z

trying to rebuild

roy-sror · 2021-09-12T14:20:38Z

/azp run

azure-pipelines · 2021-09-12T14:20:43Z

Commenter does not have sufficient privileges for PR 4078 in repo Azure/sonic-mgmt

liat-grozovik · 2021-09-12T14:24:38Z

/azp run Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: azure-pipelines[bot] ***@***.***> Sent: Sunday, September 12, 2021 5:20:54 PM To: Azure/sonic-mgmt ***@***.***> Cc: Liat Grozovik ***@***.***>; Mention ***@***.***> Subject: Re: [Azure/sonic-mgmt] [inner hashing] extend inner hashing test with dynamic Policy Base Hashing configurations (#4078) Commenter does not have sufficient privileges for PR 4078 in repo Azure/sonic-mgmt — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4078 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKTABA3FN6RLGLVN3W7LIODUBSZMNANCNFSM5CNQ7KOA>. Triage notifications on the go with GitHub Mobile for iOS<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cliatg%40nvidia.com%7Caa327545086744964a1508d975f88845%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637670532573750505%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZeMSEhxlV%2BW9WTSOUm5VplSyHSQyORH0qJOZiWtS%2BwI%3D&reserved=0> or Android<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cliatg%40nvidia.com%7Caa327545086744964a1508d975f88845%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637670532573760459%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pyx1TwJx9xm6BOBlxHLIwMryoyFhFmT3yu4l95zivF0%3D&reserved=0>.

azure-pipelines · 2021-09-12T14:24:43Z

Command 'run Get' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

liat-grozovik · 2021-09-12T14:32:19Z

/azp run

azure-pipelines · 2021-09-12T14:32:27Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Anton <antonh@nvidia.com>

liat-grozovik · 2021-09-29T08:56:25Z

@nazariig could you please help review following PR update?
@anish-n should you assign someone to review from the team?

ansible/roles/test/files/ptftests/inner_hash_test.py

tests/ecmp/inner_hashing/conftest.py

tests/ecmp/inner_hashing/test_inner_hashing.py

anish-n · 2021-10-09T00:52:22Z

tests/ecmp/inner_hashing/conftest.py

+    mg_facts = duthost.get_extended_minigraph_facts(tbinfo)
+    for intf, index in mg_facts['minigraph_ptf_indices'].items():
+        if index in vlan_ptf_ports:
+            test_intfs.append(intf)


I feel like we may need validation to check if this works with other interface types like portchannels, Ethernet interface for comprehensive test coverage

The test is dedicated to t0 topology.
I'm using here the ports which are src_ports in ptf test (vlan_ptf_ports).

To use other interface types, need to update the test to t1-lag topology(as an example).

No not necessary, you can take ports out of the vlan and configure them in a static portchannel or Ethernet interface and use that as the src port too.

In this case, it's a static(custom) config. Need to run the static test. This part of the code is related to dynamic config only.

Or maybe I don't quite understand what you mean.
In a dynamic test, take out some ports from vlan, create portchannel/s, add ports w/o vlan as members to portchannel/s, add these portchannel/s to pbh table?

Yes I mean we can modify the switch config here in the dynamic test to cover the other L3 interface types like portchannel interface and validate hashing with that

As I understand, it's mean to change the topology configurations especially for the test, and not use the base configurations
of topology with feature config only.
Also, it's not a simple change. Need time to design, implement, and testing.
I suggest creating the feature request for test extended with L3/PortChannel interfaces or/and supporting other topologies, on which the requirement will be implemented. Not block the test implementation and start to check the PBH feature in the regression.

tests/ecmp/inner_hashing/conftest.py

ansible/roles/test/files/ptftests/inner_hash_test.py

Signed-off-by: Anton <antonh@nvidia.com>

ansible/roles/test/files/ptftests/inner_hash_test.py

Signed-off-by: Anton <antonh@nvidia.com>

AntonHryshchuk · 2021-10-20T08:25:42Z

/azp run

azure-pipelines · 2021-10-20T08:25:49Z

Commenter does not have sufficient privileges for PR 4078 in repo Azure/sonic-mgmt

liat-grozovik · 2021-10-20T08:53:23Z

/azp run

azure-pipelines · 2021-10-20T08:53:32Z

Azure Pipelines successfully started running 1 pipeline(s).

AntonHryshchuk · 2021-10-21T07:33:22Z

/azpw run

tests/ecmp/inner_hashing/test_wr_inner_hashing.py

ansible/roles/test/files/ptftests/inner_hash_test.py

anish-n · 2021-10-21T17:01:15Z

tests/ecmp/inner_hashing/conftest.py

+
+def config_ipv4_rules(duthost, inner_ipver):
+    config_vxlan_rule(duthost, " --ip-protocol {}", V4_ETHER_TYPE, "ipv4", inner_ipver)
+    config_nvgre_rule(duthost, " --ip-protocol {}", V4_ETHER_TYPE, "ipv4", inner_ipver)


I feel like we maybe lacking test coverage for matching on:
gre_key = h32 "/" h32 ; GRE key (32 bits)

So would suggest adding a 3rd random packet format with a gre_key match, that way we can get full test coverage.

will be covered in the next PR

anish-n · 2021-10-21T17:03:24Z

As of now I found 2 places where we didn't have full coverage:

Multiple interface types
2.All possible match fields in pbh_rule

If you tell me any of the above are not needed for full coverage because it is covered in some way via other testing or implicit means, happy to close this. Otherwise would suggest incorporating these improvements. I won't block the PR on these comments, we can add it as a follow up PR, because this is a large change, but want to note these.

Once you close out the other open comments, happy to approve.

liat-grozovik · 2021-10-27T11:53:03Z

As of now I found 2 places where we didn't have full coverage:

Multiple interface types
2.All possible match fields in pbh_rule

If you tell me any of the above are not needed for full coverage because it is covered in some way via other testing or implicit means, happy to close this. Otherwise would suggest incorporating these improvements. I won't block the PR on these comments, we can add it as a follow up PR, because this is a large change, but want to note these.

Once you close out the other open comments, happy to approve.

@anish-n thanks for the valuable feedback on the coverage. I totally agree that the improvements are needed but no reason to block this PR. The improvements will be done by Anton on a different PR. We can discuss offline when it will be available.
If not other comments, please approve.
@AntonHryshchuk please make sure checkers are fully passing so we can merge it.

liat-grozovik · 2021-10-27T11:53:17Z

/azp run

azure-pipelines · 2021-10-27T11:53:26Z

Azure Pipelines successfully started running 1 pipeline(s).

anish-n · 2021-10-28T16:31:14Z

As of now I found 2 places where we didn't have full coverage:

Multiple interface types
2.All possible match fields in pbh_rule

If you tell me any of the above are not needed for full coverage because it is covered in some way via other testing or implicit means, happy to close this. Otherwise would suggest incorporating these improvements. I won't block the PR on these comments, we can add it as a follow up PR, because this is a large change, but want to note these.
Once you close out the other open comments, happy to approve.

@anish-n thanks for the valuable feedback on the coverage. I totally agree that the improvements are needed but no reason to block this PR. The improvements will be done by Anton on a different PR. We can discuss offline when it will be available. If not other comments, please approve. @AntonHryshchuk please make sure checkers are fully passing so we can merge it.

@liat-grozovik, @AntonHryshchuk awaiting closure of the other open comment, please mark resolved all incorporated comments. Will approve it then, thanks!

Signed-off-by: Anton <antonh@nvidia.com>

AntonHryshchuk · 2021-11-01T15:54:57Z

@anish-n - the definition of OUTER_ENCAP_FORMATS updated, now it's defined only in conftest. Please review.

…shing configurations (sonic-net#4078) Extending of inner hashing test with dynamic Policy Base Hashing configurations. Include 4 parameterized tests: * outer_ipver - inner_ipver * ipv4 - ipv4 * ipv6 - ipv4 * ipv6 - ipv4 * ipv6 - ipv6 - How did you verify/test it? The test executed on different Nvidia platforms, supported the PBH feature - Supported testbed topology if it's a new test case? t0 - Documentation sonic-net/SONiC#824

…lly (#24970) #### Why I did it src/sonic-swss ``` * 3c6ec95c - (HEAD -> 202511, origin/202511) [portsorch] fix crash when number of PGs returned 0 (sonic-net#4106) (29 hours ago) [mssonicbld] * f4c0865a - [orchagent] support single ASIC VOQ Fixed-System (sonic-net#4105) (30 hours ago) [mssonicbld] * 2a8deda1 - Change DB that DPU orchagents listens to for all orchs (sonic-net#4070) (2 days ago) [mssonicbld] * 15e017e8 - [ssw][ha] add ACTION_COUNTER to acl table type (sonic-net#4078) (2 days ago) [mssonicbld] ``` #### How I did it #### How to verify it #### Description for the changelog

AntonHryshchuk added 3 commits August 17, 2021 14:42

[inner hashing] extending inner hashing test with dynamic Policy Base…

93c7315

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

[inner hashing] extending inner hashing test with dynamic Policy Base…

ad2e616

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

[inner hashing] extending inner hashing test with dynamic Policy Base…

c1aa30c

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

AntonHryshchuk requested a review from a team as a code owner August 19, 2021 06:38

nazariig requested changes Sep 9, 2021

View reviewed changes

qiluo-msft requested a review from liat-grozovik September 9, 2021 10:59

[inner hashing] extending inner hashing test with dynamic Policy Base…

b2b82f5

… Hashing configurations Signed-off-by: Anton <antonh@nvidia.com>

AntonHryshchuk closed this Sep 10, 2021

AntonHryshchuk reopened this Sep 10, 2021

AntonHryshchuk and others added 3 commits September 19, 2021 09:19

Merge branch 'master' into inner_hashing_d_PBH

c0f39a6

Add warm-reboot test to PBH inner hashing

62464c8

Signed-off-by: Anton <antonh@nvidia.com>

Add warm-reboot test to PBH inner hashing

ffc4b50

Signed-off-by: Anton <antonh@nvidia.com>

nazariig approved these changes Oct 4, 2021

View reviewed changes

anish-n reviewed Oct 9, 2021

View reviewed changes

CR fixes to PBH inner hashing

70261e2

Signed-off-by: Anton <antonh@nvidia.com>

nazariig reviewed Oct 11, 2021

View reviewed changes

ansible/roles/test/files/ptftests/inner_hash_test.py Show resolved Hide resolved

anish-n reviewed Oct 11, 2021

View reviewed changes

ansible/roles/test/files/ptftests/inner_hash_test.py Outdated Show resolved Hide resolved

AntonHryshchuk added 3 commits October 11, 2021 20:08

CR fixes to PBH inner hashing

cac7489

Signed-off-by: Anton <antonh@nvidia.com>

CR fixes to PBH inner hashing

67a7413

Signed-off-by: Anton <antonh@nvidia.com>

CR fixes to PBH inner hashing

a299230

Signed-off-by: Anton <antonh@nvidia.com>

anish-n reviewed Oct 21, 2021

View reviewed changes

CR fixes to PBH inner hashing

b4eed52

Signed-off-by: Anton <antonh@nvidia.com>

anish-n approved these changes Nov 4, 2021

View reviewed changes

liat-grozovik approved these changes Nov 8, 2021

View reviewed changes

liat-grozovik merged commit 71cd0ca into sonic-net:master Nov 8, 2021

AntonHryshchuk deleted the inner_hashing_d_PBH branch December 6, 2021 11:35

Conversation

AntonHryshchuk commented Aug 19, 2021 • edited by liat-grozovik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you verify/test it?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

liat-grozovik commented Aug 25, 2021

Uh oh!

liat-grozovik commented Sep 9, 2021

Uh oh!

azure-pipelines bot commented Sep 9, 2021

Uh oh!

liat-grozovik commented Sep 9, 2021

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!

nazariig commented Sep 9, 2021

Uh oh!

AntonHryshchuk commented Sep 10, 2021

Uh oh!

roy-sror commented Sep 12, 2021

Uh oh!

azure-pipelines bot commented Sep 12, 2021

Uh oh!

liat-grozovik commented Sep 12, 2021 via email

Uh oh!

azure-pipelines bot commented Sep 12, 2021

Uh oh!

liat-grozovik commented Sep 12, 2021

Uh oh!

azure-pipelines bot commented Sep 12, 2021

Uh oh!

liat-grozovik commented Sep 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anish-n Oct 9, 2021 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AntonHryshchuk commented Oct 20, 2021

Uh oh!

azure-pipelines bot commented Oct 20, 2021

Uh oh!

liat-grozovik commented Oct 20, 2021

Uh oh!

azure-pipelines bot commented Oct 20, 2021

Uh oh!

AntonHryshchuk commented Oct 21, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AntonHryshchuk commented Aug 19, 2021 •

edited by liat-grozovik

Loading

anish-n Oct 9, 2021 •

edited

Loading