Vlan snmp #1

raphaelt-nvidia · 2021-04-08T13:32:31Z

- 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 sonic-net#169.

- Description for the changelog

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

stepanblyschak · 2021-04-08T14:04:54Z

tests/test_mibs.py

+<<<<<<< HEAD
+        oid_lag_name_map, \
+        lag_sai_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, db_conn)
+=======
        oid_lag_name_map, _ = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, db_conn)
+>>>>>>> 4e063e4ade89943f2413a767f24564aecfa2cd1c


Remove git conflict markers

Missed that. Taking the HEAD branch as in https://github.com/Azure/sonic-snmpagent/pull/169/files.

stepanblyschak · 2021-04-08T14:05:57Z

tests/test_mibs.py

@@ -30,3 +35,4 @@ def test_init_sync_d_lag_tables(self):

        self.assertTrue("PortChannel_Temp" in lag_name_if_name_map)
        self.assertTrue(lag_name_if_name_map["PortChannel_Temp"] == [])
+        self.assertTrue(lag_sai_map["PortChannel01"] == "2000000000006")


What is "2000000000006"?

It was added in sonic-net#169.

stepanblyschak · 2021-04-08T14:08:05Z

src/sonic_ax_impl/mibs/ietf/rfc2863.py

+        self.update_if_counters()
+        self.update_rif_counters()
+        self.aggregate_counters()
+
+        self.lag_name_if_name_map, \
+        self.if_name_lag_name_map, \
+        self.oid_lag_name_map, \
+        self.lag_sai_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, self.db_conn)
+
+        self.if_range = sorted(list(self.oid_name_map.keys()) +
+                               list(self.oid_lag_name_map.keys()) +
+                               list(self.mgmt_oid_name_map.keys()) +
+                               list(self.vlan_oid_name_map.keys()))
+        self.if_range = [(i,) for i in self.if_range]
+
+    def update_if_counters(self):
+        for sai_id_key in self.if_id_map:
+            namespace, sai_id = mibs.split_sai_id_key(sai_id_key)
+            if_idx = mibs.get_index_from_str(self.if_id_map[sai_id_key])
+            counters_db_data = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB,
+                                                                        mibs.counter_table(sai_id),
+                                                                        blocking=True)
+            self.if_counters[if_idx] = {
+                counter: int(value) for counter, value in counters_db_data.items()
+            }
+
+    def update_rif_counters(self):
+        rif_sai_ids = list(self.rif_port_map) + list(self.vlan_name_map)
+        for sai_id in rif_sai_ids:
+            counters_db_data = Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB,
+                                                     mibs.counter_table(mibs.split_sai_id_key(sai_id)[1]),
+                                                     blocking=False)
+            self.rif_counters[sai_id] = {
+                counter: int(value) for counter, value in counters_db_data.items()
+            }


This seems to be duplicated code from rfc1213. Can we write this logic once and reuse in both mibs?

I can try, but to do that I need to understand the code better in order to write it in an understandable way. In update_data(), the following code was deleted in the original commit (PR 169) from rfc1213, but retained in rfc2863. Do you know why, and what the code does?

Basim and Stepan agreed that counters are not needed in rfc2863, therefore I have deleted update_if_counters(), update_rif_counters(), aggregate_counters(), and the calls to them.

stepanblyschak · 2021-04-08T14:08:14Z

src/sonic_ax_impl/mibs/ietf/rfc2863.py

@@ -153,6 +236,31 @@ def interface_alias(self, sub_id):

        return entry.get("description", "")

+    def aggregate_counters(self):


Same comment regarding code duplication

stepanblyschak · 2021-04-08T14:10:46Z

tests/namespace/test_interfaces.py

+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 10, 1001))))
+        self.assertEqual(value0.data, 100)
+
+    def test_in_ucast_portchannel(self):
+        """
+        For a l3 portchannel interface value is accumulated on members plus added Rif counters
+        """
+        oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 11, 1001))
+        get_pdu = GetPDU(
+            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
+            oids=[oid]
+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 11, 1001))))
+        self.assertEqual(value0.data, 100)
+
+    def test_in_errors_portchannel(self):
+        """
+          For a l3 portchannel interface value is accumulated on members plus added Rif counters
+        """
+        oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 14, 1001))
+        get_pdu = GetPDU(
+            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
+            oids=[oid]
+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 14, 1001))))
+        self.assertEqual(value0.data, 106)
+
+    def test_out_octets_portchannel(self):
+        """
+        For a l3 portchannel interface value is accumulated on members plus added Rif counters
+        """
+        oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 16, 1001))
+        get_pdu = GetPDU(
+            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
+            oids=[oid]
+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 16, 1001))))
+        self.assertEqual(value0.data, 100)
+
+    def test_out_ucast_portchannel(self):
+        """
+        For a l3 portchannel interface value is accumulated on members plus added Rif counters
+        """
+        oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 17, 1001))
+        get_pdu = GetPDU(
+            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
+            oids=[oid]
+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 17, 1001))))
+        self.assertEqual(value0.data, 100)
+
+    def test_out_errors_portchannel(self):
+        """
+        For a l3 portchannel interface value is accumulated on members plus added Rif counters
+        """
+        oid = ObjectIdentifier(11, 0, 0, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 20, 1001))
+        get_pdu = GetPDU(
+            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
+            oids=[oid]
+        )
+
+        response = get_pdu.make_response(self.lut)
+        print(response)
+
+        value0 = response.values[0]
+        self.assertEqual(value0.type_, ValueType.COUNTER_32)
+        self.assertEqual(str(value0.name), str(ObjectIdentifier(11, 0, 1, 0, (1, 3, 6, 1, 2, 1, 2, 2, 1, 20, 1001))))
+        self.assertEqual(value0.data, 106)
+


Can you add similar tests for rfc2863?

Again, after speaking with Basim, RFC2863 counters are not required, so I looked for vlan references in the file. The only reference is in test_in_ucast_vlan_subinterface(), which is a counter, so I don't see what additional tests I should write.

liat-grozovik · 2021-04-27T17:01:57Z

@raphaelt-nvidia have you upstream this already? i could not find an open PR so it is either not upstream or merged. if merged, lets close this one, if not, lets please have it done.

**- What I did** Extend RFC3433 implementation with: 1. FAN tachometers 2. PSU current sensor 3. PSU voltage sensor 4. PSU power sensor 5. PSU temp sensor 6. Chassis temp sensor MIB HLD update PR to reflect this change please refer to: sonic-net/SONiC#766 A fix for the LGTM checker **- How I did it** 1. Refactor sensor data parsing class by adding a base class BaseSensorData; inherit TransceiverSensorData, PSUSensorData, FANSensorData, and ThermalSensorData from it to reduce redundant code. 2. Adding more sensor MIB entry class: PSUTempSensor, PSUVoltageSensor, PSUCurrentSensor, PSUPowerSensor, FANSpeedSensor, and ThermalSensor. 3. Separate MIB update to different functions according to different sensors types: update_xcvr_dom_data, update_psu_sensor_data, update_fan_sensor_data, and update_thermal_sensor_data. 4. Add unit test cases to cover the new added MIB entries. 5. Add lgtm.yaml to fix the LGTM checker. **- How to verify it** Manual test and run updated community SNMP test case(sonic-net/sonic-mgmt#3357).

…egate rif counters (sonic-net#169)" (sonic-net#191)" This reverts commit dfde06e.

Signed-off-by: Raphael Tryster <[email protected]>

…import paths Signed-off-by: Raphael Tryster <[email protected]>

raphaelt-nvidia self-assigned this Apr 8, 2021

stepanblyschak suggested changes Apr 8, 2021

View reviewed changes

qiluo-msft and others added 7 commits May 18, 2021 03:49

Revert "Revert "[rfc1213] Interface MIB add l3 vlan interfaces & aggr…

c467dc8

…egate rif counters (sonic-net#169)" (sonic-net#191)" This reverts commit dfde06e.

Add vlan support in rfc2863

4e16579

Fix up asic_db.json

21678dd

Signed-off-by: Raphael Tryster <[email protected]>

Changes after review by Stepan

c9f495c

Signed-off-by: Raphael Tryster <[email protected]>

Unit tests for rfc2863

c2375ab

Signed-off-by: Raphael Tryster <[email protected]>

Fixed parameters for some caller of mibs.init_sync_d_lag_tables, and …

cc61819

…import paths Signed-off-by: Raphael Tryster <[email protected]>

raphaelt-nvidia force-pushed the vlan_snmp branch from 8842851 to cc61819 Compare May 25, 2021 16:28

raphaelt-nvidia closed this May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vlan snmp #1

Vlan snmp #1

raphaelt-nvidia commented Apr 8, 2021 •

edited

Loading

stepanblyschak Apr 8, 2021

raphaelt-nvidia Apr 12, 2021

stepanblyschak Apr 8, 2021

raphaelt-nvidia Apr 12, 2021

stepanblyschak Apr 13, 2021

stepanblyschak Apr 8, 2021

raphaelt-nvidia Apr 13, 2021

raphaelt-nvidia Apr 13, 2021

stepanblyschak Apr 8, 2021

raphaelt-nvidia Apr 13, 2021

stepanblyschak Apr 8, 2021

raphaelt-nvidia Apr 13, 2021

liat-grozovik commented Apr 27, 2021

		@@ -153,6 +236,31 @@ def interface_alias(self, sub_id):

		return entry.get("description", "")

		def aggregate_counters(self):

Vlan snmp #1

Vlan snmp #1

Conversation

raphaelt-nvidia commented Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liat-grozovik commented Apr 27, 2021

raphaelt-nvidia commented Apr 8, 2021 •

edited

Loading