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.

Sign up for GitHub

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

Jump to bottom

Fix GCU test_dynamic_acl failure on 2vlan config testbed #16637

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix GCU test_dynamic_acl failure on 2vlan config testbed #16637

wants to merge 4 commits into from

Conversation

ZhaohuiS
Copy link
Contributor

@ZhaohuiS ZhaohuiS commented Jan 22, 2025 •
edited
Loading

Description of PR

Summary:
Fixes # (issue)

  1. failures of test_gcu_acl_arp_rule_creation when enabling 2 vlan config
    With 2vlan config on testbed, GCU test_dynamic_acl failed due to ip conflict with the ip address of the second vlan interface,
    192.168.0.129.
    ping success, but there is no arp entry for this ip address.
    Then case failed.

  2. Failures of test_gcu_acl_dhcp_rule_creation when enabling 2 vlan config
    Previous logic just picks up the latest ipv4 address or ipv6 address from mg_facts['minigraph_vlan_interfaces'], which doesn't work for the first vlan case

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405
  • 202411

Approach

What is the motivation for this PR?

  1. With 2vlan config on testbed, such as t0-118, by default, ipv4 range for vlan1000 is 192.168.0.1/25 and ipv4 range for vlan2000 192.168.0.129/25, so set increment to 65.

Otherwise, incrementing by 129 will cause IP overlap within the second VLAN's IP range, 192.168.0.129.

      two_vlan_a:
        Vlan1000:
          id: 1000
          intfs: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62]
          prefix: 192.168.0.1/25
          prefix_v6: fc02:1000::1/64
          tag: 1000
        Vlan2000:
          id: 2000
          intfs: [63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117]
          prefix: 192.168.0.129/25
          prefix_v6: fc02:1000:0:1::1/64
          tag: 2000
  1. Get the ipv4 or ipv6 address for specific vlan interface from configuration.

How did you do it?

  1. For vlan 1000, intf_ipv4_addr.network_address is 192.168.0.0, after increase 129, it becomes 192.168.0.129, which is same with ip address of the second vlan interface.

ptf_intf_ipv4_addr = increment_ipv4_addr(intf_ipv4_addr.network_address, incr=129)

  1. Parse ipv4 or ipv6 address from config_facts['VLAN_INTERFACE'][vlan_name], different vlan_name will get differnet ip address

How did you verify/test it?

run tests/generic_config_updater/test_dynamic_acl.py

-------------------------------------------------------------------------------------------------------------------------------------------------- live log sessionfinish --------------------------------------------------------------------------------------------------------------------------------------------------10:29:19 __init__.pytest_terminal_summary         L0067 INFO   | Can not get Allure report URL. Please check logs
====================================================================================================================================== 24 passed, 236 warnings in 4379.67s (1:12:59) =======================================================================================================================================DEBUG:tests.conftest:[log_custom_msg] item: <Function test_gcu_acl_nonexistent_table_removal[default-Vlan1000]>
INFO:root:Can not get Allure report URL. Please check logs

#### Any platform specific information?

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

Documentation

@ZhaohuiS
Fix GCU test_dynamic_acl failure on 2vlan config testbed
7af787e 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ZhaohuiS ZhaohuiS requested a review from StormLiangMS January 22, 2025 08:18
@ZhaohuiS
Support 2vlan config for case test_gcu_acl_dhcp_rule_creation
9995e22 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ZhaohuiS
Copy link
Contributor Author

ZhaohuiS commented Feb 5, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

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

@ZhaohuiS ZhaohuiS closed this Feb 5, 2025
@ZhaohuiS ZhaohuiS reopened this Feb 5, 2025
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

matthew-soulsby
matthew-soulsby reviewed Feb 5, 2025
View reviewed changes
tests/generic_config_updater/test_dynamic_acl.py Outdated
logging.info("It has vlan: {}".format(config_facts['VLAN_INTERFACE'].keys()))
increment = 65
else:
increment = 129
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we be able to calculate the increment instead? Something like:

vlan_num = len(config_facts['VLAN_INTERFACE'])
increment = math.ceil(129 / vlan_num)

# For 1 vlan, increment is 129
# for 2, increment is 65
# for 3, increment is 43 etc

Just to cover ourselves in future for similar issues, if we need to have testbeds with more VLANs

matthew-soulsby
matthew-soulsby reviewed Feb 5, 2025
View reviewed changes
tests/generic_config_updater/test_dynamic_acl.py
increment = 65
else:
increment = 129

# Increment address by 3 to offset it from the intf on which the address may be learned
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 this comment is outdated as of this PR (which must have been missed back then): #12711

Would we be able to update/remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

a little confusing here, why 3 vlan, increment is 43?

Copy link
Contributor Author

@ZhaohuiS ZhaohuiS Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As discussed offline, if pick 129 as increment for 2vlan scenarios, ip address chosen in the script will be overlapped with the second vlan's scope, which will cause case failure.

For 3vlans, we don't have this scenarios for now.

@ZhaohuiS
address comment
819d37d 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ZhaohuiS
Copy link
Contributor Author

ZhaohuiS commented Feb 8, 2025

@StormLiangMS could you please help review?

@ZhaohuiS
make comment easy to understand
ccd14fd 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

StormLiangMS
StormLiangMS approved these changes Feb 10, 2025
View reviewed changes
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthew-soulsby matthew-soulsby matthew-soulsby left review comments

@StormLiangMS StormLiangMS StormLiangMS approved these changes

Assignees
No one assigned
Labels
Request for 202405 branch Request for 202411 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZhaohuiS @mssonicbld @matthew-soulsby @StormLiangMS