[test_genric_hash.py]: cisco platform checks fix #16135

harjotsinghpawra · 2024-12-18T10:23:16Z

Description of PR

Cisco support different HASH algorithms and fields as of now than default

Some issues were because CISCO was not supporting certain hash algorithms and Fields which led to issues like below

/12/2024 03:58:04 __init__.pytest_runtest_call             L0040 ERROR  | Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/_pytest/python.py", line 1788, in runtest
    self.ihook.pytest_pyfunc_call(pyfuncitem=self)
  File "/usr/local/lib/python3.8/dist-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/usr/local/lib/python3.8/dist-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/usr/local/lib/python3.8/dist-packages/pluggy/_callers.py", line 139, in _multicall
    raise exception.with_traceback(exception.__traceback__)
  File "/usr/local/lib/python3.8/dist-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/usr/local/lib/python3.8/dist-packages/_pytest/python.py", line 194, in pytest_pyfunc_call
    result = testfunction(**testargs)
  File "/data/tests/hash/test_generic_hash.py", line 151, in test_ecmp_hash
    lag_hash_fields.remove(ecmp_test_hash_field)
ValueError: list.remove(x): x not in list

some algorithm config is missing i have listed one of many errors whose root cause is same

Summary:
Fixes # (issue)

Type of change

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

Back port request

Approach

What is the motivation for this PR?

To add support for CISCO specific supported hash alogrithms and fields

How did you do it?

added asic_type = 'cisco-8000'check for platform specific algo and fields to be picked

How did you verify/test it?

Tested it on T1/T0 cisco platforms where correct calulation was done after my fox and only supported fields were picked

Any platform specific information?

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

Documentation

mssonicbld · 2024-12-18T10:23:19Z

/azp run

azure-pipelines · 2024-12-18T10:23:31Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2024-12-18T10:40:15Z

/azp run

azure-pipelines · 2024-12-18T10:40:30Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2024-12-18T10:49:22Z

/azp run

azure-pipelines · 2024-12-18T10:49:34Z

Azure Pipelines successfully started running 1 pipeline(s).

harjotsinghpawra · 2024-12-18T22:38:53Z

/azp run

azure-pipelines · 2024-12-18T22:38:59Z

Commenter does not have sufficient privileges for PR 16135 in repo sonic-net/sonic-mgmt

rajendrat · 2024-12-19T19:58:51Z

@wsycqyz @bpar9 : Please help review

bpar9

@harjotsinghpawra can you add rest of the info in the PR description template - Motivation for PR (what the current checks were lacking) (2) also how/which topo it was verified and need verification for both lag & non-lag

harjotsinghpawra · 2024-12-19T23:47:50Z

@bpar9 Added more details in the PR

kevinskwang · 2024-12-20T05:57:05Z

ansible/roles/test/files/ptftests/py3/generic_hash_test.py

-                        logging.info('Check only one port in a portchannel received traffic')
-                        assert False, 'The traffic is balanced over portchannel members.'
+                    if hit_port_number == len(self.expected_port_groups[0]):
+                        logging.info('Check if all ports in a portchannel received traffic')


I think this change doesn't make sense here.The purpose of this function is to just check the load balancing not happening on the port channel.

kevinskwang · 2024-12-20T06:15:13Z

ansible/roles/test/files/ptftests/py3/generic_hash_test.py

            assert _calculate_balance(expected_hit_cnt_per_port), "The balancing result is beyond the range."

        expected_total_hit_cnt = self.balancing_test_times * len(self.expected_port_list)
+        lag_expected_total_hit_cnt = self.balancing_test_times * len(self.expected_port_groups[0])


all the packet should reach to the same ECMP path in this function, the expected hit cunt should use expected_total_hit_cnt, rather than lag_expected_total_hit_cnt

mssonicbld · 2025-01-09T12:32:58Z

/azp run

azure-pipelines · 2025-01-09T12:33:11Z

Azure Pipelines successfully started running 1 pipeline(s).

harjotsinghpawra · 2025-01-09T12:38:58Z

@kevinskwang Hi kevin , i think i miss understood the test cases . I though that normal ECMP and lag hashes are tested individually . But i understand in one case we don't have any ECMP hash algorithm enabled and only enable lag hash algo then we test lag hash. Vice versa is also true where ECMP hash is tested and lag hash is disabled . I have reverted my changes back and only kept platform checks specific to CISCO . I hope my understanding of case is now correct

bpar9 · 2025-01-10T05:12:55Z

@harjotsinghpawra , can you modify the title and description of the PR reflecting the reduced changes which the PR now has. Should specify that you are making only cisco related platform checks and remove the check_balance fix info as you have dropped those changes. Approved the PR for the current Cisco related changes

* [test_genric_hash.py]: cisco platform checks and some check_balance fix * line spaces and artifact fixes * indent fix * check_balance fixes revert commit

mssonicbld · 2025-01-13T12:49:11Z

Cherry-pick PR to 202405: #16496

* [test_genric_hash.py]: cisco platform checks and some check_balance fix * line spaces and artifact fixes * indent fix * check_balance fixes revert commit

mssonicbld · 2025-01-13T12:49:16Z

Cherry-pick PR to 202411: #16497

* [test_genric_hash.py]: cisco platform checks and some check_balance fix * line spaces and artifact fixes * indent fix * check_balance fixes revert commit

[test_genric_hash.py]: cisco platform checks and some check_balance fix

c3b1923

line spaces and artifact fixes

6a38265

indent fix

3684e85

harjotsinghpawra marked this pull request as ready for review December 19, 2024 19:41

bpar9 requested changes Dec 19, 2024

View reviewed changes

kevinskwang reviewed Dec 20, 2024

View reviewed changes

check_balance fixes revert commit

eb39e6b

kevinskwang approved these changes Jan 10, 2025

View reviewed changes

bpar9 approved these changes Jan 10, 2025

View reviewed changes

harjotsinghpawra changed the title ~~[test_genric_hash.py]: cisco platform checks and some check_balance fix~~ [test_genric_hash.py]: cisco platform checks fix Jan 10, 2025

kevinskwang merged commit e9a000f into sonic-net:master Jan 13, 2025
19 checks passed

kevinskwang added Request for 202405 branch Request for 202411 branch Approved for 202405 branch Approved for 202411 branch labels Jan 13, 2025

mssonicbld added the Created PR to 202405 branch label Jan 13, 2025

mssonicbld mentioned this pull request Jan 13, 2025

[action] [PR:16135] [test_genric_hash.py]: cisco platform checks fix #16496

Merged

8 tasks

mssonicbld added the Created PR to 202411 branch label Jan 13, 2025

mssonicbld mentioned this pull request Jan 13, 2025

[action] [PR:16135] [test_genric_hash.py]: cisco platform checks fix #16497

Merged

8 tasks

mssonicbld added Included in 202411 branch and removed Created PR to 202411 branch labels Jan 13, 2025

mssonicbld added Included in 202405 branch and removed Created PR to 202405 branch labels Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test_genric_hash.py]: cisco platform checks fix #16135

[test_genric_hash.py]: cisco platform checks fix #16135

harjotsinghpawra commented Dec 18, 2024 •

edited

Loading

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

harjotsinghpawra commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

rajendrat commented Dec 19, 2024

bpar9 left a comment

harjotsinghpawra commented Dec 19, 2024

kevinskwang Dec 20, 2024

kevinskwang Dec 20, 2024

mssonicbld commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

harjotsinghpawra commented Jan 9, 2025

bpar9 commented Jan 10, 2025

mssonicbld commented Jan 13, 2025

mssonicbld commented Jan 13, 2025

[test_genric_hash.py]: cisco platform checks fix #16135

[test_genric_hash.py]: cisco platform checks fix #16135

Conversation

harjotsinghpawra commented Dec 18, 2024 • edited Loading

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

mssonicbld commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

harjotsinghpawra commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

rajendrat commented Dec 19, 2024

bpar9 left a comment

Choose a reason for hiding this comment

harjotsinghpawra commented Dec 19, 2024

kevinskwang Dec 20, 2024

Choose a reason for hiding this comment

kevinskwang Dec 20, 2024

Choose a reason for hiding this comment

mssonicbld commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

harjotsinghpawra commented Jan 9, 2025

bpar9 commented Jan 10, 2025

mssonicbld commented Jan 13, 2025

mssonicbld commented Jan 13, 2025

harjotsinghpawra commented Dec 18, 2024 •

edited

Loading