Fix SNMP output having fewer unicast queues than expected #330

justin-wong-ce · 2024-09-05T16:48:06Z

SNMP assumes the first half of the queues to be unicast and the second half to be multicast. This assumption was fine for most hwSkus but not new ones being added to support, i.e. x86_64-arista_7060x6.

This change will allow configurations where there are more than half of maximum queues set as unicast. It however does not account for the case where there are more than half of the maximum queues set as multicast - that will cause problems for the sonic-mgmt snmp/test_snmp_queue.py test.

If the SNMP is expected to only return unicast queues, then I will make further changes to the unit test and source code to reflect this expectation in functionality.
If not, sonic-mgmt's snmp/test_snmp_queue.py should not expect only unicast queues to be returned, and SNMP should return all queues, not half (https://github.com/sonic-net/sonic-mgmt/blob/ac37f7f0fd7c7857559325a61d5ca6e6c4932194/tests/snmp/test_snmp_queue.py#L92) (grep UC)

Note:
Only tested on Arista hwSkus.

- What I did
Accommodate for the case where the number of Unicast queues exceed the number of Multicast queues.

- How I did it
Allow Unicast queues to be added to response when their queue number is greater than half of the total number of queues.

- How to verify it
Passing unit tests and SNMP tests in sonic-mgmt. Note: Only ran on Arista hwSkus

- Description for the changelog

SNMP assumes the first half of the queues to be unicast and the second half to be multicast. This assumption was fine for most hwSkus but not new ones being added to support, i.e. x86_64-arista_7060x6 Only tested on Arista hwSkus.

r12f · 2024-09-09T18:05:06Z

@Janetxxx , do you mind to cherry pick this change to our internal test branch?

Janetxxx · 2024-09-10T02:14:39Z

@Janetxxx , do you mind to cherry pick this change to our internal test branch?

@r12f Done.

r12f · 2024-09-10T02:15:30Z

Great! Thanks Janet!

StormLiangMS · 2024-09-12T04:20:32Z

hi @qiluo-msft @SuvarnaMeenakshi could you help to review?

Janetxxx

@justin-wong-ce @r12f I have tested it in the internal setup, it worked.

---------------------------------------------------------------- generated xml file: /data/sonic-mgmt-int/tests/logs/snmp/test_snmp_queue.xml ----------------------------------------------------------------
------------------------------------------------------------------------------------------- live log sessionfinish -------------------------------------------------------------------------------------------
04:57:31 __init__.pytest_terminal_summary         L0067 INFO   | Can not get Allure report URL. Please check logs
================================================================================= 1 passed, 1 warning in 1169.64s (0:19:29) ==================================================================================

Janetxxx · 2024-09-23T06:48:54Z

@qiluo-msft Could you please help to merge this PR, thanks!

SuvarnaMeenakshi · 2024-09-26T17:22:16Z

@justin-wong-ce , did you verify if the output of SNMP walk of queue counter from any other Arista hwsku is the same before and after the fix?
Can you share the output if possible?

justin-wong-ce · 2024-09-26T22:12:53Z

@justin-wong-ce , did you verify if the output of SNMP walk of queue counter from any other Arista hwsku is the same before and after the fix? Can you share the output if possible?

I checked its the same for the following hwSkus:

Arista-7050CX3-32S C32
Arista-7060CX-32S D48C8
Arista-7260CX3 D108C8

I will follow up with the output via email.

justin-wong-ce · 2024-10-03T21:36:34Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

+                if self.queue_type_map[namespace].get(queue_sai_oid) == 'SAI_QUEUE_TYPE_UNICAST':
+                   pq_count = pq_count + 1
+
+            # If there are fewer unicast queues than half of max queues, we use the old assumption of second half mcast


It is best that we don't do this - instead return only unicast queues at all times.

justin-wong-ce · 2024-10-03T21:37:45Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

            # To simulate vendor OID, we wrap queues by max priority groups
            port_max_queues = Namespace.dbs_get_all(self.db_conn, mibs.STATE_DB,
                                                    mibs.buffer_max_parm_table(self.oid_name_map[if_index]))['max_queues']
-            pq_count = math.ceil(int(port_max_queues) / 2)
+            max_queues_half = math.ceil(int(port_max_queues) / 2)


Can achieve https://github.com/sonic-net/sonic-snmpagent/pull/330/files#r1786870229 by not doing this check and keeping pq_count as the number of actual unicast queues at all times.

qiluo-msft · 2024-10-05T00:19:55Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

-            pq_count = math.ceil(int(port_max_queues) / 2)
+            max_queues_half = math.ceil(int(port_max_queues) / 2)
+            if pq_count < max_queues_half:
+                pq_count = max_queues_half


Could you add a new testcase which could fail the code before your PR?

Added a new sub-case to an existing test case to check.

Note: If we will commit to returning all queues, or only returning unicast queues (no matter the number), then this test case, along with other test cases will need to be updated.

Added a unit test subcase and relevant mock tablesto check for SNMP behavior when there are more unicast queues than half of the total number of queues. This is to ensure SNMP does not operate on the false assumption where there is always a 50-50 split between unicast and multicast queues.

Janetxxx

LGTM. It passed tests in my local setup.

---------------------------------------------------------------- generated xml file: /data/sonic-mgmt-int/tests/logs/snmp/test_snmp_queue.xml ----------------------------------------------------------------
------------------------------------------------------------------------------------------- live log sessionfinish -------------------------------------------------------------------------------------------
10:03:29 __init__.pytest_terminal_summary         L0067 INFO   | Can not get Allure report URL. Please check logs
================================================================================= 1 passed, 1 warning in 1103.72s (0:18:23) ==================================================================================

Janetxxx · 2024-10-10T03:31:01Z

@qiluo-msft Could you please help to review the new testcase? Thank you

Janetxxx · 2024-10-14T06:15:39Z

@yxieca @bingwang-ms Could you please help to approve the labels?

) justin-wong-ce First-time contributor justin-wong-ce commented on Sep 6 • SNMP assumes the first half of the queues to be unicast and the second half to be multicast. This assumption was fine for most hwSkus but not new ones being added to support, i.e. x86_64-arista_7060x6. This change will allow configurations where there are more than half of maximum queues set as unicast. It however does not account for the case where there are more than half of the maximum queues set as multicast - that will cause problems for the sonic-mgmt snmp/test_snmp_queue.py test. If the SNMP is expected to only return unicast queues, then I will make further changes to the unit test and source code to reflect this expectation in functionality. If not, sonic-mgmt's snmp/test_snmp_queue.py should not expect only unicast queues to be returned, and SNMP should return all queues, not half (https://github.com/sonic-net/sonic-mgmt/blob/ac37f7f0fd7c7857559325a61d5ca6e6c4932194/tests/snmp/test_snmp_queue.py#L92) (grep UC) Note: Only tested on Arista hwSkus. - What I did Accommodate for the case where the number of Unicast queues exceed the number of Multicast queues. - How I did it Allow Unicast queues to be added to response when their queue number is greater than half of the total number of queues. - How to verify it Passing unit tests and SNMP tests in sonic-mgmt. Note: Only ran on Arista hwSkus - Description for the changelog

mssonicbld · 2024-10-14T06:35:42Z

Cherry-pick PR to 202311: #333

mssonicbld · 2024-10-14T06:35:42Z

Cherry-pick PR to 202405: #334

justin-wong-ce First-time contributor justin-wong-ce commented on Sep 6 • SNMP assumes the first half of the queues to be unicast and the second half to be multicast. This assumption was fine for most hwSkus but not new ones being added to support, i.e. x86_64-arista_7060x6. This change will allow configurations where there are more than half of maximum queues set as unicast. It however does not account for the case where there are more than half of the maximum queues set as multicast - that will cause problems for the sonic-mgmt snmp/test_snmp_queue.py test. If the SNMP is expected to only return unicast queues, then I will make further changes to the unit test and source code to reflect this expectation in functionality. If not, sonic-mgmt's snmp/test_snmp_queue.py should not expect only unicast queues to be returned, and SNMP should return all queues, not half (https://github.com/sonic-net/sonic-mgmt/blob/ac37f7f0fd7c7857559325a61d5ca6e6c4932194/tests/snmp/test_snmp_queue.py#L92) (grep UC) Note: Only tested on Arista hwSkus. - What I did Accommodate for the case where the number of Unicast queues exceed the number of Multicast queues. - How I did it Allow Unicast queues to be added to response when their queue number is greater than half of the total number of queues. - How to verify it Passing unit tests and SNMP tests in sonic-mgmt. Note: Only ran on Arista hwSkus - Description for the changelog

justin-wong-ce added 2 commits September 5, 2024 16:31

Updated comment and better formatting

664cd6b

StormLiangMS requested review from qiluo-msft and SuvarnaMeenakshi September 12, 2024 04:20

Janetxxx previously approved these changes Sep 23, 2024

View reviewed changes

Pterosaur added Request for 202311 Branch Request for 202405 Branch labels Sep 24, 2024

justin-wong-ce commented Oct 3, 2024

View reviewed changes

qiluo-msft reviewed Oct 5, 2024

View reviewed changes

justin-wong-ce dismissed Janetxxx’s stale review via 6f8f173 October 9, 2024 18:48

Janetxxx approved these changes Oct 10, 2024

View reviewed changes

Pterosaur merged commit c5301b2 into sonic-net:master Oct 14, 2024
5 checks passed

yxieca added Approved for 202311 Branch Approved for 202405 Branch labels Oct 14, 2024

This was referenced Oct 14, 2024

[action] [PR:330] Fix SNMP output having fewer unicast queues than expected (#330) #333

Merged

[action] [PR:330] Fix SNMP output having fewer unicast queues than expected (#330) #334

Merged

mssonicbld added the Created PR to 202311 Branch label Oct 14, 2024

mssonicbld added the Created PR to 202405 Branch label Oct 14, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Oct 14, 2024

mssonicbld added Included in 202311 Branch and removed Created PR to 202311 Branch labels Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SNMP output having fewer unicast queues than expected #330

Fix SNMP output having fewer unicast queues than expected #330

justin-wong-ce commented Sep 5, 2024 •

edited

Loading

r12f commented Sep 9, 2024

Janetxxx commented Sep 10, 2024

r12f commented Sep 10, 2024

StormLiangMS commented Sep 12, 2024

Janetxxx left a comment •

edited

Loading

Janetxxx commented Sep 23, 2024

SuvarnaMeenakshi commented Sep 26, 2024

justin-wong-ce commented Sep 26, 2024

justin-wong-ce Oct 3, 2024

justin-wong-ce Oct 3, 2024

qiluo-msft Oct 5, 2024

justin-wong-ce Oct 9, 2024

Janetxxx left a comment •

edited

Loading

Janetxxx commented Oct 10, 2024

Janetxxx commented Oct 14, 2024

mssonicbld commented Oct 14, 2024

mssonicbld commented Oct 14, 2024

Fix SNMP output having fewer unicast queues than expected #330

Fix SNMP output having fewer unicast queues than expected #330

Conversation

justin-wong-ce commented Sep 5, 2024 • edited Loading

r12f commented Sep 9, 2024

Janetxxx commented Sep 10, 2024

r12f commented Sep 10, 2024

StormLiangMS commented Sep 12, 2024

Janetxxx left a comment • edited Loading

Choose a reason for hiding this comment

Janetxxx commented Sep 23, 2024

SuvarnaMeenakshi commented Sep 26, 2024

justin-wong-ce commented Sep 26, 2024

justin-wong-ce Oct 3, 2024

Choose a reason for hiding this comment

justin-wong-ce Oct 3, 2024

Choose a reason for hiding this comment

qiluo-msft Oct 5, 2024

Choose a reason for hiding this comment

justin-wong-ce Oct 9, 2024

Choose a reason for hiding this comment

Janetxxx left a comment • edited Loading

Choose a reason for hiding this comment

Janetxxx commented Oct 10, 2024

Janetxxx commented Oct 14, 2024

mssonicbld commented Oct 14, 2024

mssonicbld commented Oct 14, 2024

justin-wong-ce commented Sep 5, 2024 •

edited

Loading

Janetxxx left a comment •

edited

Loading

Janetxxx left a comment •

edited

Loading