sonic-mgmt icon indicating copy to clipboard operation
sonic-mgmt copied to clipboard

Generalizing GCU test suite to verify both ip types (IPV4, IPV6) for BGP Neighbors

Open okaravasi opened this issue 1 year ago • 2 comments

Currently, under generic config update there is a test suite verifying only IPV6 BGP Neighbors' changes. This PR extends the existing test suite and adds support for verifying IPV4 BGP NEighbors also. This is achieved by generalizing the test code with adding dynamic variable for neighbor ip type and with test mark parameters. Also, renamed the test suite to a generic name.

Description of PR

Summary: This is an improvement on existing test suite generic_config_updater/test_ipv6.py Currently, under generic config update there is a test suite verifying only IPV6 BGP Neighbors' changes. This PR extends the existing test suite and adds support for verifying IPV4 BGP NEighbors also. This is achieved by generalizing the test code with adding dynamic variable for neighbor ip type and with test mark parameters. Also, renamed the test suite to a generic name.

Type of change

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

Back port request

  • [ ] 202012
  • [ ] 202205
  • [ ] 202305
  • [ ] 202311
  • [x] 202405

Approach

What is the motivation for this PR?

Currently, there are no test cases for IPv4 BGP Neighbors in the verification of generic config updates functionality. This PR extends the existing test suite, which verifies IPv6 BGP Peers, to add support for IPv4 BGP Neighbors verification.

How did you do it?

Added argument for ip_version in test functions to dynamically verify the relevant peer IP type. Added test mark parameters to run the same test case for both IP types. Renamed the test suite to a generic name that reflects new test cases content.

How did you verify/test it?

Ran the newly created test suite and verified that the test cases pass for both IP types.

Any platform specific information?

N/A

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

N/A

Documentation

okaravasi avatar Jul 12 '24 14:07 okaravasi

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: okaravasi / name: Olympia Karavasili Arapogianni (84cfe6ba5795b3658ad90228a5b89cd51c47faa3, 6cf668c11e65e9a7dc5b413e3e739e1dfebcc32c, 51bbb52a026da18af3628048b7df274e519247ac)

The pre-commit check detected issues in the files touched by this pull request. The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results: trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/generic_config_updater/test_ip_bgp.py:199:1: E302 expected 2 blank lines, found 1

flake8...............................................(no files to check)Skipped
check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

mssonicbld avatar Jul 12 '24 15:07 mssonicbld

Hello @judyjoseph @arlakshm , Could you please help on reviewing this? Thank you.

okaravasi avatar Jul 15 '24 13:07 okaravasi

LGTM, @okaravasi could you resolve the conflicts. Also do add a snapshot of the test run for ipv4 and ipv6 under the section "How did you verify/test it?"

judyjoseph avatar Oct 01 '24 22:10 judyjoseph

LGTM, @okaravasi could you resolve the conflicts. Also do add a snapshot of the test run for ipv4 and ipv6 under the section "How did you verify/test it?"

Done: Updated the branch and included screenshot from kvm-t0 run in the description.

okaravasi avatar Oct 02 '24 16:10 okaravasi

@okaravasi Arvind had a comment earlier -- that we need to add support for multi-asic as well. Do you plan to do it in this PR ?

judyjoseph avatar Oct 07 '24 20:10 judyjoseph

ad a comment earlier -- that we need to add support for multi-asic as well. Do you plan to do it in this PR ?

@judyjoseph Support to multi-asic will be handled by different PRs.

  1. Adding support to existing test scope shall be handled via : https://github.com/sonic-net/sonic-mgmt/pull/14070
  2. Adding new test for t2/chassis GCU shall be handled via separate ticket: https://github.com/sonic-net/sonic-mgmt/pull/14887. I will comment soon when it is opened as non-draft.

okaravasi avatar Oct 08 '24 19:10 okaravasi

@wen587 Can you please check if this change is required for 202405?

bingwang-ms avatar Nov 14 '24 21:11 bingwang-ms

@wen587 Can you please check if this change is required for 202405?

Hi @bingwang-ms , this is an improvement from ipv6 only test to ipv4+ipv6 test. It is good to have in 202405 but not bug fix related.

wen587 avatar Nov 15 '24 03:11 wen587