[masic] test_cacl_application.py: Fix argument issue in verify_cacl by wenyiz2021 · Pull Request #6170 · sonic-net/sonic-mgmt

wenyiz2021 · 2022-08-15T20:11:12Z

Signed-off-by: Wenyi Zhang [email protected]

Description of PR

PR #5895 introduced 2 fixture which should be intended to work for dualtor, while it added argument in verify_cacl, which is also used for test_multiasic_cacl_application, failing to add this argument in this test leads to an argument misleading at runtime, between expected_dhcp_rules_for_standby and enum_frontend_asic_index

Summary:
Fixes # (issue)

Type of change

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

Back port request

201911
202012

Approach

What is the motivation for this PR?

Fix issue seen on multi-asic seen after above PR merged

How did you do it?

Add argument expected_dhcp_rules_for_standby in test_cacl_application_dualtor, as the fixture take fixture duthost_dualtor that for non-dualtor, there is only 1 active_tor and returns empty list.

How did you verify/test it?

Before:
30/07/2022 07:12:07 init._log_sep_line L0160 INFO | ==================== cacl/test_cacl_application.py::test_multiasic_cacl_application[3] call ====================
30/07/2022 07:12:07 init.pytest_runtest_call L0040 ERROR | Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/_pytest/python.py", line 1464, in runtest
self.ihook.pytest_pyfunc_call(pyfuncitem=self)
File "/usr/local/lib/python2.7/dist-packages/pluggy/hooks.py", line 286, in call
return self._hookexec(self, self.get_hookimpls(), kwargs)
File "/usr/local/lib/python2.7/dist-packages/pluggy/manager.py", line 93, in _hookexec
return self._inner_hookexec(hook, methods, kwargs)
File "/usr/local/lib/python2.7/dist-packages/pluggy/manager.py", line 87, in
firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
File "/usr/local/lib/python2.7/dist-packages/pluggy/callers.py", line 208, in _multicall
return outcome.get_result()
File "/usr/local/lib/python2.7/dist-packages/pluggy/callers.py", line 81, in get_result
_reraise(*ex) # noqa
File "/usr/local/lib/python2.7/dist-packages/pluggy/callers.py", line 187, in _multicall
res = hook_impl.function(*args)
File "/usr/local/lib/python2.7/dist-packages/_pytest/python.py", line 174, in pytest_pyfunc_call
testfunction(**testargs)
File "/azp/agent/_work/2/s/sonic-mgmt-int/tests/cacl/test_cacl_application.py", line 838, in test_multiasic_cacl_application
verify_cacl(duthost, localhost, creds, docker_network, enum_frontend_asic_index)
File "/azp/agent/_work/2/s/sonic-mgmt-int/tests/cacl/test_cacl_application.py", line 745, in verify_cacl
expected_iptables_rules, expected_ip6tables_rules = generate_expected_rules(duthost, docker_network, asic_index, expected_dhcp_rules_for_standby)
File "/azp/agent/_work/2/s/sonic-mgmt-int/tests/cacl/test_cacl_application.py", line 416, in generate_expected_rules
iptables_rules.extend(expected_dhcp_rules_for_standby)
TypeError: 'int' object is not iterable

After:
cacl/test_cacl_application.py::test_cacl_application_nondualtor PASSED [ 7%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[active_tor] SKIPPED [ 15%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[standby_tor] SKIPPED [ 23%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[active_tor-0] PASSED [ 30%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[standby_tor-0] PASSED [ 38%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[active_tor-1] PASSED [ 46%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[standby_tor-1] PASSED [ 53%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[active_tor-2] PASSED [ 61%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[standby_tor-2] PASSED [ 69%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[active_tor-3] PASSED [ 76%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[standby_tor-3] PASSED [ 84%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv4 PASSED [ 92%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv6 PASSED [100%]

--------------------------------------------------------------------------------------------- generated xml file: /var/src/sonic-mgmt-int/tests/logs/tr.xml ----------------------------------------------------------------------------------------------
================================================================================================================ short test summary info =================================================================================================================
SKIPPED [2] cacl/test_cacl_application.py: test_cacl_application_dualtor is only supported on dualtor topology
========================================================================================================= 11 passed, 2 skipped in 895.93 seconds =========================================================================================================

Any platform specific information?

Fix for multi-asic

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

Documentation

Signed-off-by: Wenyi Zhang <[email protected]>

linux-foundation-easycla · 2022-08-15T20:11:14Z

The committers listed above are authorized under a signed CLA.

✅ login: wenyiz2021 (b1cd519, ad29a4a, 1091807, 5d3ce7a, eedc16c)

wenyiz2021 · 2022-08-15T21:57:07Z

/easycla

wenyiz2021 · 2022-08-15T22:16:45Z

/easycla

commit b1cd519 Author: Wenyi Zhang <[email protected]> Date: Mon Aug 15 20:09:12 2022 +0000 Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]>

…-mgmt into cacl_appl

wenyiz2021 · 2022-08-15T22:47:02Z

/easycla

Signed-off-by: wenyiz2021 <[email protected]>

…c-mgmt into cacl_appl

wenyiz2021 · 2022-08-15T23:02:30Z

/easycla

wenyiz2021 · 2022-08-15T23:37:48Z

/easycla

SuvarnaMeenakshi · 2022-08-16T01:46:04Z

tests/cacl/test_cacl_application.py

    """
    duthost = duthosts[rand_one_dut_hostname]
-    verify_cacl(duthost, tbinfo, localhost, creds, docker_network, enum_frontend_asic_index)
+    verify_cacl(duthost, tbinfo, localhost, creds, docker_network, expected_dhcp_rules_for_standby, enum_frontend_asic_index)


I think the expectation from the changes in #5895 is that, if it is not dualtor, then we have to keep expected_dhcp_rules_for_standby as None in verify_cacl.
So for mulit-asic platform, in

def test_multiasic_cacl_application, verify_cacl(duthost, tbinfo, localhost, creds, docker_network, None, enum_frontend_asic_index)

the expectation is None/empty-list.
fixture expected_dhcp_rules_for_standby will return empty list for non-dualtor scenarios.

@SuvarnaMeenakshi think you are right, passing None makes code cleaner. Changed in this way.

bingwang-ms · 2022-08-16T05:08:10Z

/azp run

azure-pipelines · 2022-08-16T05:08:19Z

Azure Pipelines successfully started running 1 pipeline(s).

ZhaohuiS · 2022-08-16T07:47:16Z

tests/cacl/test_cacl_application.py

    verify_cacl(duthost_dualtor, tbinfo, localhost, creds, docker_network, expected_dhcp_rules_for_standby)

-def test_multiasic_cacl_application(duthosts, tbinfo, rand_one_dut_hostname, localhost, creds,docker_network, enum_frontend_asic_index):
+def test_multiasic_cacl_application(duthosts, tbinfo, rand_one_dut_hostname, localhost, creds, docker_network, expected_dhcp_rules_for_standby, enum_frontend_asic_index):


@wenyiz2021 Thank you for your hard work to fix this issue. I think your solution is better, should we add expected_dhcp_rules_for_standby for test_cacl_application_nondualtor as well, even it works fine based on current code.
One wired thing is in the traceback of test_cacl_application_nondualtor, expected_dhcp_rules_for_standby is 1, that's the reason why this case failed. I verified test_cacl_application_nondualtor, the expected_dhcp_rules_for_standby is null. What's the difference for these 2 cases, I didn't figure out.

@ZhaohuiS I didn't change for test_cacl_application_nondualtor, because it is calling verify_cacl this way:
verify_cacl(duthost, localhost, creds, docker_network, None)
where def is:
def verify_cacl(duthost, localhost, creds, docker_network, expected_dhcp_rules_for_standby, asic_index = None)

so it is passing None to expected_dhcp_rules_for_standby, and leave asic_index = None, so it should be good.

may I know where you find "traceback of test_cacl_application_nondualtor, expected_dhcp_rules_for_standby is 1"?

Alternatively, I gave default None to expected_dhcp_rules_for_standby at verify_cacl() definition.

So that only test_multiasic_cacl_application that called verify_cacl with an asic_index need to specifically pass None to expected_dhcp_rules_for_standby.
test_cacl_application_dualtor can pass its expected_dhcp_rules_for_standby.

Other callers don't need to specify for the 2 arguments.

@wenyiz2021 I got it, this the difference, if I put expected_dhcp_rules_for_standby behind asic_index, it should be fine. As you said, it's correct to set it to None. Thank you for fixing this.

the log of "traceback of test_cacl_application_nondualtor, expected_dhcp_rules_for_standby is 1" is here:

@ZhaohuiS yeah, that traceback comes from test_multiasic_cacl_application(), where the integer comes from asic_index iso intending for expected_dhcp_rules_for_standby .

of expected_dhcp_rules is list before adding it to the iptables_rules

lgtm-com · 2022-08-16T17:58:38Z

This pull request introduces 1 alert and fixes 2 when merging 50711ad into 55b23fd - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

2 for Unused import

lgtm-com · 2022-08-16T21:47:54Z

This pull request introduces 1 alert and fixes 2 when merging d76799a into 55b23fd - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

2 for Unused import

…6170) * Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]> * Squashed commit of the following: commit b1cd519 Author: Wenyi Zhang <[email protected]> Date: Mon Aug 15 20:09:12 2022 +0000 Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]> * Squash commit with new signature Signed-off-by: wenyiz2021 <[email protected]> * Add pytest_assert to ensure type of expected_dhcp_rules is list before adding it to the iptables_rules * Change expected_dhcp.. to None if not needed * add backslash * Fix syntax Signed-off-by: Wenyi Zhang <[email protected]> Signed-off-by: wenyiz2021 <[email protected]>

…onic-net#6170) * Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]> * Squashed commit of the following: commit b1cd519 Author: Wenyi Zhang <[email protected]> Date: Mon Aug 15 20:09:12 2022 +0000 Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]> * Squash commit with new signature Signed-off-by: wenyiz2021 <[email protected]> * Add pytest_assert to ensure type of expected_dhcp_rules is list before adding it to the iptables_rules * Change expected_dhcp.. to None if not needed * add backslash * Fix syntax Signed-off-by: Wenyi Zhang <[email protected]> Signed-off-by: wenyiz2021 <[email protected]>

Fix argument in test_multiasic_cacl_application

b1cd519

Signed-off-by: Wenyi Zhang <[email protected]>

wenyiz2021 added the Request for 202012 branch label Aug 15, 2022

wenyiz2021 requested a review from ZhaohuiS August 15, 2022 20:11

wenyiz2021 added 2 commits August 15, 2022 22:44

Squashed commit of the following:

ad29a4a

commit b1cd519 Author: Wenyi Zhang <[email protected]> Date: Mon Aug 15 20:09:12 2022 +0000 Fix argument in test_multiasic_cacl_application Signed-off-by: Wenyi Zhang <[email protected]>

Merge branch 'cacl_appl' of https://github.com/wenyiz2021/wenyi-sonic…

1091807

…-mgmt into cacl_appl

wenyiz2021 added 2 commits August 15, 2022 23:00

Squash commit with new signature

5d3ce7a

Signed-off-by: wenyiz2021 <[email protected]>

WMerge branch 'cacl_appl' of https://github.com/wenyiz2021/wenyi-soni…

eedc16c

…c-mgmt into cacl_appl

SuvarnaMeenakshi reviewed Aug 16, 2022

View reviewed changes

ZhaohuiS reviewed Aug 16, 2022

View reviewed changes

wenyiz2021 added 2 commits August 16, 2022 10:27

Add pytest_assert to ensure type

e94f4ff

of expected_dhcp_rules is list before adding it to the iptables_rules

Change expected_dhcp.. to None if not needed

50711ad

add backslash

d76799a

Fix syntax

aad7158

ZhaohuiS approved these changes Aug 17, 2022

View reviewed changes

wenyiz2021 merged commit 6738170 into sonic-net:master Aug 17, 2022

wenyiz2021 deleted the cacl_appl branch August 17, 2022 17:24

wangxin added the Included in 202012 branch label Aug 22, 2022

ZhaohuiS mentioned this pull request Sep 18, 2022

Don't add dhcp rules for multiasic testbed #6165

Closed

5 tasks

Conversation

wenyiz2021 commented Aug 15, 2022

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

Uh oh!

linux-foundation-easycla bot commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenyiz2021 commented Aug 15, 2022

Uh oh!

wenyiz2021 commented Aug 15, 2022

Uh oh!

wenyiz2021 commented Aug 15, 2022

Uh oh!

wenyiz2021 commented Aug 15, 2022

Uh oh!

wenyiz2021 commented Aug 15, 2022

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!

bingwang-ms commented Aug 16, 2022

Uh oh!

azure-pipelines bot commented Aug 16, 2022

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!

lgtm-com bot commented Aug 16, 2022

Uh oh!

lgtm-com bot commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linux-foundation-easycla bot commented Aug 15, 2022 •

edited

Loading