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

[rfc1213] Interface MIB add l3 vlan interfaces & aggregate rif counters #169

Merged
merged 26 commits into from
Dec 8, 2020

Conversation

stepanblyschak
Copy link
Contributor

- What I did

Rework based on #133 and feedback #148.

- How I did it

- How to verify it

  • Disable rif counters
  • restart swss
  • snmpwalk
  • Verify no UnavailableDataError in logs
  • Verify port counters are returned
  • Enable rif counters
  • snmpwalk
  • Verify no error in logs
  • Verify error in/out counters are aggregated for rif ports and portchannels
  • Verify VLAN RIF counters are present in the MIB.

- Description for the changelog

Added support for aggregated router interface counters and L3 VLAN counters to RFC1213 MIB.

Mykola Faryma and others added 21 commits May 15, 2020 12:32
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Fix for the following error in syslog:

Oct  9 10:07:21.427210 mts-sonic-dut ERR snmp#snmp-subagent [ax_interface] ERROR: MIBUpdater.start() caught an unexpected exception during update_data()#012Traceback (most recent call last):sonic-net#12  File "/usr/local/lib/python3.7/dist-packa
ges/ax_interface/mib.py", line 43, in start#012    self.update_data()sonic-net#12  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/ietf/rfc1213.py", line 245, in update_data#012    self.update_rif_counters()sonic-net#12  File "/usr/local
/lib/python3.7/dist-packages/sonic_ax_impl/mibs/ietf/rfc1213.py", line 271, in update_rif_counters#012    for sai_id in rif_sai_ids}sonic-net#12  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/ietf/rfc1213.py", line 271, in <dic
tcomp>sonic-net#12    for sai_id in rif_sai_ids}sonic-net#12  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/__init__.py", line 627, in dbs_get_all#012    ns_result = db_conn.get_all(db_name, _hash, *args, **tmp_kwargs)sonic-net#12  File "/usr/
local/lib/python3.7/dist-packages/swsssdk/dbconnector.py", line 295, in get_all#012    return self.dbintf.get_all(db_name, _hash, *args, **kwargs)sonic-net#12  File "/usr/local/lib/python3.7/dist-packages/swsssdk/interface.py", line 38, in wrapp
ed#012    ret_data = f(inst, db_name, *args, **kwargs)sonic-net#12  File "/usr/local/lib/python3.7/dist-packages/swsssdk/interface.py", line 314, in get_all#012    raise UnavailableDataError(message, _hash)#012swsssdk.exceptions.UnavailableDataE
rror: Key 'b'COUNTERS:oid:0x60000000005d3'' unavailable in database 'COUNTERS_DB'

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@stepanblyschak stepanblyschak changed the title Rif vlan counters [rfc1213] Interface MIB add l3 vlan interfaces & aggregate rif counters Oct 16, 2020
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Oct 26, 2020

Please resolve the conflicts. #Closed

NEIGH_TABLE:device:ipv4_address
"""
_, device, ip = neigh_key.split(':')
return device, ip
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 26, 2020

Choose a reason for hiding this comment

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

de [](start = 11, length = 2)

If you merge latest master, you don't need to use byte array and just use string. #Closed

@@ -219,24 +194,40 @@ def reinit_data(self):
self.mgmt_oid_name_map, \
self.mgmt_alias_map = mibs.init_mgmt_interface_tables(self.db_conn[0])

self.vlan_name_map, \
Copy link
Contributor

Choose a reason for hiding this comment

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

bytes [](start = 29, length = 5)

If you merge latest master, you don't need to encoding to byte array.

@@ -4,6 +4,7 @@
from bisect import bisect_right

from sonic_ax_impl import mibs
from sonic_ax_impl import logger
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 26, 2020

Choose a reason for hiding this comment

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

logger [](start = 26, length = 6)

Merge with above line #Closed

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi left a comment

Choose a reason for hiding this comment

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

similar to test_interfaces, can you add changes in tests/namespace/test_interfaces.
Also, mock_db changes for multi-asic mock-dbs as well.

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2020

This pull request introduces 1 alert when merging 144658f into 57e54d9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Nov 4, 2020

Please resolve LGTM alert. Others look good to me. #Closed

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
…an_counters

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@svc-acs
Copy link
Collaborator

svc-acs commented Nov 19, 2020

Can one of the admins verify this patch?

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Please also check with other reviewers.

@@ -237,7 +237,7 @@ def test_if_type_portchannel(self):
self.assertEqual(value0.data, 161)

def test_getnextpdu_first_bp_ifindex(self):
oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 1,1004))
oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 1, 8999))
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Nov 20, 2020

Choose a reason for hiding this comment

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

Is this change required? There is no new interface added in namespace mock_dbs, is that right? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

@stepanblyschak This comment is still applicable.


In reply to: 527836254 [](ancestors = 527836254)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuvarnaMeenakshi
Copy link
Contributor

similar to test_interfaces, can you add changes in tests/namespace/test_interfaces.
Also, mock_db changes for multi-asic mock-dbs as well.

Closed.

@liat-grozovik
Copy link
Collaborator

@qiluo-msft can you please merge so we can proceed with the submodule update?

@qiluo-msft qiluo-msft merged commit e54036c into sonic-net:master Dec 8, 2020
if port_sai_id in self.if_id_map:
port_idx = mibs.get_index_from_str(self.if_id_map[port_sai_id])
for port_counter_name, rif_counter_name in mibs.RIF_DROPS_AGGR_MAP.items():
self.if_counters[port_idx][port_counter_name] = \
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2020

Choose a reason for hiding this comment

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

self.if_counters [](start = 20, length = 16)

I see you change self.if_counters[][] type from str to int.
Although this is working in python, however we are moving the library to sonic-swss-common SWIG wrapper, and this will make SWIG confused.
Please make type fixed. #Closed

stepanblyschak added a commit to stepanblyschak/sonic-snmpagent that referenced this pull request Jan 12, 2021
qiluo-msft pushed a commit that referenced this pull request Jan 13, 2021
…f counters (#169)" (#191)

**- What I did**
Revert all drop SNMP counters issue. Because it introduces inconsistency with RFC2863

**- How I did it**
Revert 3 commits related to this feature.

**- How to verify it**
The RFC1213 MIB does not have VLAN interfaces and does not perform counters accumulation.
raphaelt-nvidia added a commit to raphaelt-nvidia/sonic-snmpagent that referenced this pull request Apr 8, 2021
raphaelt-nvidia added a commit to raphaelt-nvidia/sonic-snmpagent that referenced this pull request May 25, 2021
qiluo-msft pushed a commit that referenced this pull request Jun 10, 2021
…for RFC2863 (#218)

- What I did

Restored snmp vlan support per RFC1213, which had been reverted due to inconsistency with RFC2863, and added the missing support for RFC2863. Added unit tests for RFC2863.

- How I did it

Reverted #191, which was itself a revert of #169. Then added code to support vlan in rfc2863.py and unit tests.

Since a long time has elapsed since the original commit and other code has been pushed in the meantime, great care is needed in merging. Note in particular that mibs.init_sync_d_lag_tables now returns five parameters, the last two of which were added: lag_sai_map and sai_lap_map. This PR needs one of those maps, and a concurrent commit supporting RFC4363 needs the other, so all the callers were updated and use the one they need.

- How to verify it

Unit tests are run via make target/python-wheels/asyncsnmp-2.1.0-py3-none-any.whl.
See also Azure#169.

- Description for the changelog

Restored support for aggregated router interface counters and L3 VLAN counters to RFC1213 MIB, and extended to RFC2863.
SuvarnaMeenakshi pushed a commit to SuvarnaMeenakshi/sonic-snmpagent that referenced this pull request Feb 24, 2023
…for RFC2863 (sonic-net#218)

- What I did

Restored snmp vlan support per RFC1213, which had been reverted due to inconsistency with RFC2863, and added the missing support for RFC2863. Added unit tests for RFC2863.

- How I did it

Reverted sonic-net#191, which was itself a revert of sonic-net#169. Then added code to support vlan in rfc2863.py and unit tests.

Since a long time has elapsed since the original commit and other code has been pushed in the meantime, great care is needed in merging. Note in particular that mibs.init_sync_d_lag_tables now returns five parameters, the last two of which were added: lag_sai_map and sai_lap_map. This PR needs one of those maps, and a concurrent commit supporting RFC4363 needs the other, so all the callers were updated and use the one they need.

- How to verify it

Unit tests are run via make target/python-wheels/asyncsnmp-2.1.0-py3-none-any.whl.
See also Azure#169.

- Description for the changelog

Restored support for aggregated router interface counters and L3 VLAN counters to RFC1213 MIB, and extended to RFC2863.

(cherry picked from commit 266bd15)
SuvarnaMeenakshi pushed a commit to SuvarnaMeenakshi/sonic-snmpagent that referenced this pull request Feb 28, 2023
…for RFC2863 (sonic-net#218)

- What I did

Restored snmp vlan support per RFC1213, which had been reverted due to inconsistency with RFC2863, and added the missing support for RFC2863. Added unit tests for RFC2863.

- How I did it

Reverted sonic-net#191, which was itself a revert of sonic-net#169. Then added code to support vlan in rfc2863.py and unit tests.

Since a long time has elapsed since the original commit and other code has been pushed in the meantime, great care is needed in merging. Note in particular that mibs.init_sync_d_lag_tables now returns five parameters, the last two of which were added: lag_sai_map and sai_lap_map. This PR needs one of those maps, and a concurrent commit supporting RFC4363 needs the other, so all the callers were updated and use the one they need.

- How to verify it

Unit tests are run via make target/python-wheels/asyncsnmp-2.1.0-py3-none-any.whl.
See also Azure#169.

- Description for the changelog

Restored support for aggregated router interface counters and L3 VLAN counters to RFC1213 MIB, and extended to RFC2863.

(cherry picked from commit 266bd15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants