Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update acl test case to permit nic simulator keepalive path in dualtor-aa topology. #15619

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

echuawu
Copy link
Contributor

@echuawu echuawu commented Nov 19, 2024

Description of PR

Update acl test case to permit nic simulator keepalive path in dualtor-aa topology.

Summary:
Fixes # (issue)
#15206

Type of change

  • [z] Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

Bug fix - #15206

How did you do it?

Permit nic simulator keepalive path in dualtor-aa topology

How did you verify/test it?

Run it in internal dualtor regression.

Any platform specific information?

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

Documentation

Permit nic simulator keepalive path

Change-Id: I32212d2bf9406efe1228ede7e8083aff44fb2b2f
@liat-grozovik liat-grozovik changed the title Permit nic simulator keepalive path Update acl test case to permit nic simulator keepalive path in dualtor-aa topology. Dec 3, 2024
@liat-grozovik liat-grozovik merged commit baefdc2 into sonic-net:master Dec 3, 2024
16 checks passed
@@ -46,7 +47,7 @@

# TODO: We really shouldn't have two separate templates for v4 and v6, need to combine them somehow
ACL_RULES_FULL_TEMPLATE = {
"ipv4": "acltb_test_rules.j2",
"ipv4": "acltb_test_rules_permit_loopback.j2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not add the loopback ACL rule to the original file?

Copy link
Contributor Author

@echuawu echuawu Dec 4, 2024

Choose a reason for hiding this comment

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

Hi @bingwang-ms , the acltb_test_rules.j2 has been shared by tests/ssh/test_ssh_stress.py too, if keep the change in the original file, it would affect tests/ssh/test_ssh_stress.py.

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Dec 4, 2024
…r-aa topology. (sonic-net#15619)

- How did you do it?
Permit nic simulator keepalive path in dualtor-aa topology

- How did you verify/test it?
Run it in internal dualtor regression.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #15866

@@ -25,6 +25,7 @@
from tests.common.fixtures.conn_graph_facts import conn_graph_facts # noqa F401
from tests.common.platform.processes_utils import wait_critical_processes
from tests.common.platform.interface_utils import check_all_interface_information
from tests.qos.tunnel_qos_remap_base import get_iface_ip
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @echuawu, there are cross feature dependency, which means, we are not allowed to import functions from tests/qos in tests/acl, can you fix this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @echuawu, there are cross feature dependency, which means, we are not allowed to import functions from tests/qos in tests/acl, can you fix this issue?

Hi @yutongzhang-microsoft , is it proper to define a get_iface_ip function in test_acl.py?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can.

@yutongzhang-microsoft
Copy link
Contributor

Hi, @echuawu , I raise the PR #15893 to fix the cross-dependency issue, can you help me review?

@echuawu
Copy link
Contributor Author

echuawu commented Dec 5, 2024

Hi, @echuawu , I raise the PR #15893 to fix the cross-dependency issue, can you help me review?

Done.

wangxin pushed a commit that referenced this pull request Dec 5, 2024
What is the motivation for this PR?
In PR #15619, a cross-feature dependency was introduced in tests/acl/test_acl.py due to the usage of the get_iface_ip function. To address this issue and streamline the code structure, we have refactored the function and moved it to a common location.

How did you do it?
To address this issue and streamline the code structure, we have refactored the function and moved it to a common location.

How did you verify/test it?
mssonicbld pushed a commit that referenced this pull request Jan 21, 2025
…r-aa topology. (#15619)

- How did you do it?
Permit nic simulator keepalive path in dualtor-aa topology

- How did you verify/test it?
Run it in internal dualtor regression.
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.

6 participants