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

Add a test case to verify that BGP packets use queue7 #8097

Merged
merged 6 commits into from
Aug 7, 2023

Conversation

prabhataravind
Copy link
Contributor

@prabhataravind prabhataravind commented Apr 20, 2023

Description of PR

  • Add a test case to verify that BGP packets use queue7 on all platforms
  • All control packets including BGP are expected to use queue7, separate from data packet queues

Summary:
Fixes #7695

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

It was observed that control packets use queue 0 in TD2 platforms which can lead to these packets being lost when link utilization becomes close to 100%. This can lead to critical issues in field like BGP keepalive failures. This management test is a way to make sure that this doesn't happen again in production.

How did you do it?

By bringing up BGP sessions with neighbors and making sure that BGP control traffic uses queue 7 by default using "show queue counters".

How did you verify/test it?

By running the test and making sure that it works as expected, reporting success or failure depending on which queues are used by control packets.

Any platform specific information?

This is applicable to all platforms.

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

Documentation

* All control packets including BGP are expected to use Q7, separate
  from data packet queues

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@prabhataravind prabhataravind requested a review from prsunny April 20, 2023 23:46
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@mssonicbld
Copy link
Collaborator

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/bgp/test_bgp_queue.py:62:47: E231 missing whitespace after ','
tests/bgp/test_bgp_queue.py:64:47: E231 missing whitespace after ','

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>

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@prabhataravind prabhataravind marked this pull request as ready for review June 12, 2023 20:33
 * Check all queues for the test instead of just q0
 * Remove check for q7 counters until regression due to
   sonic-net/sonic-swss#2143 is fixed
 * Perform interface queue counter checks only once when there are v4 and v6
   neighbors over the same interface

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@prabhataravind prabhataravind merged commit 127b197 into sonic-net:master Aug 7, 2023
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Sep 15, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202305: #9991

@wangxin
Copy link
Collaborator

wangxin commented Sep 15, 2023

@prabhataravind This is a new case. Generally, we only backport bug fixes to release branches. Can you elaborate why this PR is also required for 202205 and 202012 branch?

@prabhataravind
Copy link
Contributor Author

@prabhataravind This is a new case. Generally, we only backport bug fixes to release branches. Can you elaborate why this PR is also required for 202205 and 202012 branch?

@wangxin, Neetha added request labels for these branches. I also think this is a good test case for all release branches.. BGP packets using another queue can potentially lead to unwanted BGP flaps in production. This was a repair item for an issue found in 201911 but also seen on 202012 and 202205 on certain bcm and cisco platforms..

mssonicbld pushed a commit that referenced this pull request Sep 16, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@rlhui
Copy link

rlhui commented Nov 4, 2023

@wangxin , please help expediting the cherry-pick, thanks.

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202012: #10641

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202205: #10642

mssonicbld pushed a commit that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit that referenced this pull request Nov 10, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
@Blueve
Copy link
Collaborator

Blueve commented Nov 16, 2023

@prabhataravind FYI, we need to skip this test case for some platforms that do not have QoS related feature supported

AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
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.

[testgap] Add a test case to verify that control packets are using TX queue 7
6 participants