Skip to content

[test_snmp_queue] Adjusted the test according to the new buffer queue/pg counters optimization #6744

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
Merged
merged 1 commit into from
Nov 9, 2022
Merged

[test_snmp_queue] Adjusted the test according to the new buffer queue/pg counters optimization #6744

merged 1 commit into from
Nov 9, 2022

Conversation

vadymhlushko-mlnx
Copy link
Contributor

@vadymhlushko-mlnx vadymhlushko-mlnx commented Nov 4, 2022 •
edited
Loading

Description of PR

Summary: Adjusted the test test_snmp_queue.py according to the new buffer queue/pg counters optimization
Fixes for submodule update PR pull/12498

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?

Because of the new implementation sonic-swss/pull/2432 from now on, there is a valid case if there are no queue counters inside the COUNTERS_DB.

In order for queue counters to be presented in COUNTERS_DB the appropriate queues should be configured inside the CONFIG_DB (e.g "QUEUE|Ethernet14|7-8")

How did you do it?

Change the test

How did you verify/test it?

Any platform specific information?

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

Documentation

@vadymhlushko-mlnx
Copy link
Contributor Author

@neethajohn could you please review it?

@azure-pipelines
Copy link

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

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/snmp/test_snmp_queue.py:10:1: E302 expected 2 blank lines, found 1
tests/snmp/test_snmp_queue.py:10:121: E501 line too long (122 > 120 characters)
tests/snmp/test_snmp_queue.py:24:121: E501 line too long (162 > 120 characters)
tests/snmp/test_snmp_queue.py:31:53: W601 .has_key() is deprecated, use 'in'

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>

@vadymhlushko-mlnx vadymhlushko-mlnx force-pushed the master-test-snmp-queue-fixes branch from b9c4156 to 3a44d7d Compare November 4, 2022 14:40
@azure-pipelines
Copy link

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

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/snmp/test_snmp_queue.py:10:1: E302 expected 2 blank lines, found 1
tests/snmp/test_snmp_queue.py:10:121: E501 line too long (122 > 120 characters)
tests/snmp/test_snmp_queue.py:24:121: E501 line too long (162 > 120 characters)

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>

@vadymhlushko-mlnx
[test_snmp_queue] Adjusted the test according to the new buffer queue…
9c99b36 
…/pg counters optimization

Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx vadymhlushko-mlnx force-pushed the master-test-snmp-queue-fixes branch from 3a44d7d to 9c99b36 Compare November 7, 2022 12:50
@vadymhlushko-mlnx
Copy link
Contributor Author

@neethajohn could you please review & merge it?

@yxieca yxieca merged commit 164b15c into sonic-net:master Nov 9, 2022
@kartik-arista kartik-arista mentioned this pull request Feb 28, 2023
Closed
wangxin pushed a commit that referenced this pull request Mar 7, 2023
@vadymhlushko-mlnx @wangxin
[test_snmp_queue] Adjusted the test according to the new buffer queue…
582e368 
…/pg counters optimization (#6744)

What is the motivation for this PR?
Because of the new implementation sonic-swss/pull/2432 from now on, there is a valid case if there are no queue counters inside the COUNTERS_DB.

In order for queue counters to be presented in COUNTERS_DB the appropriate queues should be configured inside the CONFIG_DB (e.g "QUEUE|Ethernet14|7-8")

Signed-off-by: Vadym Hlushko <[email protected]>
@SuvarnaMeenakshi SuvarnaMeenakshi mentioned this pull request Jul 25, 2023
Merged
6 tasks
SuvarnaMeenakshi added a commit that referenced this pull request Aug 22, 2023
@SuvarnaMeenakshi
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
8123ab7 
#9115)

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in #6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Aug 28, 2023
@SuvarnaMeenakshi @mssonicbld
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
97f9bd9 
sonic-net#9115)

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in sonic-net#6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.
@mssonicbld mssonicbld mentioned this pull request Aug 28, 2023
Merged
6 tasks
mssonicbld pushed a commit that referenced this pull request Aug 28, 2023
@SuvarnaMeenakshi @mssonicbld
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
c59a40b 
#9115)

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in #6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.
SuvarnaMeenakshi added a commit to SuvarnaMeenakshi/sonic-mgmt that referenced this pull request Sep 14, 2023
@SuvarnaMeenakshi
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
bd8db3e 
sonic-net#9115)

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in sonic-net#6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.

(cherry picked from commit 8123ab7)
@SuvarnaMeenakshi SuvarnaMeenakshi mentioned this pull request Sep 14, 2023
Merged
6 tasks
wangxin pushed a commit that referenced this pull request Sep 15, 2023
@SuvarnaMeenakshi
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
7d3e372 
#9115) (#9973)

Cherry-pick of #9115

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in #6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.

(cherry picked from commit 8123ab7)
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
@SuvarnaMeenakshi @AharonMalkin
[snmp][multi-asic]: Fix test_snmp_queue to support multi-asic platform (
ac1d8d9 
sonic-net#9115)

What is the motivation for this PR?
test_snmp_queue was modified to get keys "QUEUE|*" from config_db in sonic-net#6744.
This has to be modified to get keys from all namespace config_db on mulit-asic platform.
Currently, test tries to retrieve from host config_db and skips the test for multi-asic platform.

Test is currently skipped for supervisor node, but should not be skipped for supervisor node of packet chassis.

How did you do it?
Modified to get interfaces with queue configuration from all namespaces.
The test case was not checking the right field in snmp_interfaces SNMP result. Test case was checking for "description" of each interface, from description, interface name was extracted and checked if the interface name is present in QUEUE table.
This check will always be false and pytest fail condition was never hit. The test case was always incorrectly passing.
(Pdb) snmp_facts['snmp_interfaces']['117']['description']
u'ARISTA02T1:Ethernet1'

   for k, v in list(snmp_facts['snmp_interfaces'].items()):
        if "Ethernet" in v['description']:
            intf = v['description'].split(':')
            # 'ARISTA*:Ethernet*'
            if len(intf) == 2:
                if intf[1] in q_interfaces and 'queues' not in v: -- intf[1] in q_interfaces will always be false
                    pytest.fail(
                        "port %s does not have queue counters" % v['name'])  -- will never hit this condition
                intf[1] will always be Ethernet1 which is the interface of the neighbor
Modified test to use 'name' which gives the interface alias instead of interface name present in 'description'

(Pdb) snmp_facts['snmp_interfaces']['117']['name']
u'fortyGigE0/116'
Remove skip of supervisor node.
On voq chassis LC, QUEUE configuration will include hostname and asic namespace as queue configuration is created on system port. Test is modified to get the interface name for voq chassis.
"QUEUE": {
        "<hostname>|asic0|Ethernet0|0": {
            "scheduler": "scheduler.0"
        },
How did you verify/test it?
Verified on single asic VS testbed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yxieca yxieca yxieca approved these changes

@volodymyrsamotiy volodymyrsamotiy volodymyrsamotiy approved these changes

@neethajohn neethajohn Awaiting requested review from neethajohn

@wangxin wangxin Awaiting requested review from wangxin

Assignees
No one assigned
Labels
Included in 202205 branch Request for 202205 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vadymhlushko-mlnx @yxieca @volodymyrsamotiy @wangxin @arlakshm