Skip to content

Enhance test_cacl_application to support dualtor#5895

Merged
StormLiangMS merged 3 commits intosonic-net:masterfrom
ZhaohuiS:guard/cacl_support_dualtor
Jul 1, 2022
Merged

Enhance test_cacl_application to support dualtor#5895
StormLiangMS merged 3 commits intosonic-net:masterfrom
ZhaohuiS:guard/cacl_support_dualtor

Conversation

@ZhaohuiS
Copy link
Contributor

@ZhaohuiS ZhaohuiS commented Jun 29, 2022

Signed-off-by: Zhaohui Sun [email protected]

Description of PR

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?

test_cacl_application failed on dualtor, if it randomly selects standby tor to run test case, it will failed with this kind of error
Failed: Unexpected iptables rules: set([u'-A DHCP -m mark --mark 0x67024 -j DROP', u'-A DHCP -m mark --mark 0x67012 -j DROP', u'-A DHCP -m mark --mark 0x67003 -j DROP', u'-A DHCP -m mark --mark 0x67002 -j DROP', u'-A DHCP -m mark --mark 0x67022 -j DROP', u'-A DHCP -m mark --mark 0x67020 -j DROP', u'-A DHCP -m mark --mark 0x67018 -j DROP', u'-A DHCP -m mark --mark 0x67014 -j DROP', u'-A DHCP -m mark --mark 0x67017 -j DROP', u'-A DHCP -m mark --mark 0x67006 -j DROP', u'-A DHCP -m mark --mark 0x67016 -j DROP', u'-A DHCP -m mark --mark 0x67019 -j DROP', u'-A DHCP -m mark --mark 0x67021 -j DROP', u'-A DHCP -m mark --mark 0x67008 -j DROP', u'-A DHCP -m mark --mark 0x67001 -j DROP', u'-A DHCP -m mark --mark 0x67010 -j DROP', u'-A DHCP -m mark --mark 0x67013 -j DROP', u'-A DHCP -m mark --mark 0x67011 -j DROP', u'-A DHCP -m mark --mark 0x67009 -j DROP', u'-A DHCP -m mark --mark 0x67007 -j DROP', u'-A DHCP -m mark --mark 0x67004 -j DROP', u'-A DHCP -m mark --mark 0x67015 -j DROP', u'-A DHCP -m mark --mark 0x67023 -j DR

How did you do it?

The failed reason is on standby tor, it will add one rule DHCP mark rule for each standby interface.
Changes in this PR:

  1. Add a new fixture duthost_dualtor to return duthost for dualtor test case test_cacl_application_dualtor, add another new fixture expected_dhcp_rules_for_standby to generate expected dhcp mark rules for standby tor.
  2. Move pytest.skip to tests_mark_conditions.yaml
  3. Add from tests.common.dualtor.dual_tor_common import * , otherwise it will throw error of fixture 'cable_type' not found.

How did you verify/test it?

Run cacl/test_cacl_application.py

cacl/test_cacl_application.py::test_cacl_application_nondualtor SKIPPED                                                                                                                                             [ 16%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[active_tor] PASSED                                                                                                                          [ 33%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[standby_tor] PASSED                                                                                                                         [ 50%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[None] SKIPPED                                                                                                                             [ 66%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv4 PASSED                                                                                                                                         [ 83%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv6 PASSED                                                                                                                                         [100%]

Any platform specific information?

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

Documentation

@ZhaohuiS ZhaohuiS requested a review from a team as a code owner June 29, 2022 12:51
@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2022

This pull request introduces 2 alerts when merging 3003024 into cde9e34 - view on LGTM.com

new alerts:

  • 2 for Unused import

def add_dhcp_rule_on_dualtor(tbinfo, duthosts, rand_one_dut_hostname, upper_tor_host, lower_tor_host,
toggle_all_simulator_ports_to_upper_tor):
global expect_dhcp_rules
# There are some hardcoded cacl rule for dualtot testbed, which should be ignored
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dualtot - dualtor

from tests.common.config_reload import config_reload
from tests.common.utilities import wait_until

from tests.common.dualtor.mux_simulator_control import toggle_all_simulator_ports_to_upper_tor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would add some extra cacl in standby tor for standby port, if that is the case, should we add one more test case to cover this other than randomly pick up a ToR to run the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would add some extra cacl in standby tor for standby port, if that is the case, should we add one more test case to cover this other than randomly pick up a ToR to run the test?

@StormLiangMS I agree with you, add a new test case test_cacl_application_dualtor in my latest commit. Please review, thanks.

@lgtm-com
Copy link

lgtm-com bot commented Jun 30, 2022

This pull request introduces 3 alerts when merging 9e05681 into 5c9cfaf - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for 'import *' may pollute namespace

Signed-off-by: Zhaohui Sun <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Jun 30, 2022

This pull request introduces 2 alerts when merging 2a8e46f into 5c9cfaf - view on LGTM.com

new alerts:

  • 2 for Unused import

Copy link
Collaborator

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StormLiangMS StormLiangMS merged commit 73f2dfb into sonic-net:master Jul 1, 2022
@wangxin
Copy link
Collaborator

wangxin commented Jul 4, 2022

@ZhaohuiS This change has conflicts if cherry-pick to 202012 branch. It depends on #5545 which was not in 202012. Can you help confirm whether it is really required for 202012 branch? If yes, can you help create a separate PR to 202012 branch since there is cherry-pick conflicts.

wangxin pushed a commit that referenced this pull request Jul 4, 2022
What is the motivation for this PR?
test_cacl_application failed on dualtor, if it randomly selects standby tor to run test case, it will failed with this kind of error
Failed: Unexpected iptables rules: set([u'-A DHCP -m mark --mark 0x67024 -j DROP', u'-A DHCP -m mark --mark 0x67012 -j DROP', u'-A DHCP -m mark --mark 0x67003 -j DROP', u'-A DHCP -m mark --mark 0x67002 -j DROP', u'-A DHCP -m mark --mark 0x67022 -j DROP', u'-A DHCP -m mark --mark 0x67020 -j DROP', u'-A DHCP -m mark --mark 0x67018 -j DROP', u'-A DHCP -m mark --mark 0x67014 -j DROP', u'-A DHCP -m mark --mark 0x67017 -j DROP', u'-A DHCP -m mark --mark 0x67006 -j DROP', u'-A DHCP -m mark --mark 0x67016 -j DROP', u'-A DHCP -m mark --mark 0x67019 -j DROP', u'-A DHCP -m mark --mark 0x67021 -j DROP', u'-A DHCP -m mark --mark 0x67008 -j DROP', u'-A DHCP -m mark --mark 0x67001 -j DROP', u'-A DHCP -m mark --mark 0x67010 -j DROP', u'-A DHCP -m mark --mark 0x67013 -j DROP', u'-A DHCP -m mark --mark 0x67011 -j DROP', u'-A DHCP -m mark --mark 0x67009 -j DROP', u'-A DHCP -m mark --mark 0x67007 -j DROP', u'-A DHCP -m mark --mark 0x67004 -j DROP', u'-A DHCP -m mark --mark 0x67015 -j DROP', u'-A DHCP -m mark --mark 0x67023 -j DR

How did you do it?
The failed reason is on standby tor, it will add one rule DHCP mark rule for each standby interface.
Changes in this PR:

Add a new fixture duthost_dualtor to return duthost for dualtor test case test_cacl_application_dualtor, add another new fixture expected_dhcp_rules_for_standby to generate expected dhcp mark rules for standby tor.
Move pytest.skip to tests_mark_conditions.yaml
Add from tests.common.dualtor.dual_tor_common import * , otherwise it will throw error of fixture 'cable_type' not found.
How did you verify/test it?
Run cacl/test_cacl_application.py

cacl/test_cacl_application.py::test_cacl_application_nondualtor SKIPPED                                                                                                                                             [ 16%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[active_tor] PASSED                                                                                                                          [ 33%]
cacl/test_cacl_application.py::test_cacl_application_dualtor[standby_tor] PASSED                                                                                                                         [ 50%]
cacl/test_cacl_application.py::test_multiasic_cacl_application[None] SKIPPED                                                                                                                             [ 66%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv4 PASSED                                                                                                                                         [ 83%]
cacl/test_cacl_application.py::test_cacl_scale_rules_ipv6 PASSED                                                                                                                                         [100%]
Any platform specific information?
Supported testbed topology if it's a new test case?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants