support for some bridge-mib and q-bridge-mib objects #227

suresh-rupanagudi · 2021-07-15T10:35:29Z

- What I did
rfc4188 mib objects supported:
1) dot1dBaseBridgeAddress 2) dot1dBaseNumPorts 3) dot1dBaseType 4) dot1dBasePort
5)dot1dBasePortIfIndex 6) dot1dBasePortDelayExceededDiscards 7) dot1dBasePortMtuExceededDiscards
8)dot1dTpAgingTime
rfc4363 mib objects supported:
1)dot1qBase 2) dot1qFdbTable 3) dot1qTpFdbTable 4) dot1qVlanNumDeletes 5) dot1qVlanCurrentTable
6)dot1qVlanStaticTable 7) dot1qPvid

- How I did it

- How to verify it
1)Configure Vlan and mac.

UT:

snmpwalk -v2c -c public 10.59.143.175 1.3.6.1.2.1.17

SNMPv2-SMI::mib-2.17.1.1.0 = STRING: "80:a2:35:26:06:61"
SNMPv2-SMI::mib-2.17.1.2.0 = INTEGER: 1
SNMPv2-SMI::mib-2.17.1.3.0 = INTEGER: 2
SNMPv2-SMI::mib-2.17.1.4.1.1.0 = INTEGER: 0
SNMPv2-SMI::mib-2.17.1.4.1.2.0 = INTEGER: 1
SNMPv2-SMI::mib-2.17.1.4.1.4.0 = Counter32: 0
SNMPv2-SMI::mib-2.17.1.4.1.5.0 = Counter32: 0
SNMPv2-SMI::mib-2.17.4.2.0 = INTEGER: 600
SNMPv2-SMI::mib-2.17.7.1.1.1.0 = INTEGER: 2
SNMPv2-SMI::mib-2.17.7.1.1.2.0 = INTEGER: 4094
SNMPv2-SMI::mib-2.17.7.1.1.3.0 = Gauge32: 4094
SNMPv2-SMI::mib-2.17.7.1.1.4.0 = Gauge32: 1
SNMPv2-SMI::mib-2.17.7.1.2.1.1.2.100 = Counter32: 0
SNMPv2-SMI::mib-2.17.7.1.2.2.1.2.100.0.1.2.3.4.5 = INTEGER: 1
SNMPv2-SMI::mib-2.17.7.1.2.2.1.3.100.0.1.2.3.4.5 = INTEGER: 5
SNMPv2-SMI::mib-2.17.7.1.4.1.0 = Counter32: 0
SNMPv2-SMI::mib-2.17.7.1.4.2.1.4.0.100 = STRING: "80 00"
SNMPv2-SMI::mib-2.17.7.1.4.2.1.5.0.100 = STRING: "0"
SNMPv2-SMI::mib-2.17.7.1.4.2.1.6.0.100 = INTEGER: 2
SNMPv2-SMI::mib-2.17.7.1.4.3.1.1.100 = STRING: "Vlan100"
SNMPv2-SMI::mib-2.17.7.1.4.3.1.2.100 = STRING: "80 00"
SNMPv2-SMI::mib-2.17.7.1.4.3.1.4.100 = STRING: "0"
SNMPv2-SMI::mib-2.17.7.1.4.3.1.5.100 = INTEGER: 1
SNMPv2-SMI::mib-2.17.7.1.4.5.1.1.0 = INTEGER: 0

- Description for the changelog

lgtm-com · 2021-07-15T10:46:27Z

This pull request introduces 2 alerts when merging 1adb270 into 21d7d97 - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'

lguohan · 2022-03-01T21:14:56Z

/azp run

azure-pipelines · 2022-03-01T21:15:05Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-03-01T21:42:09Z

Please fix LGTM 2 new alerts.

qiluo-msft · 2022-03-01T21:42:48Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+    def get_tp_fdb_status(self, ent, fdb):
+        #TODO:add code for self type
+        if 'SAI_FDB_ENTRY_ATTR_TYPE' not  in ent:
+            return  TpFdbStatusConst.invalid


Could you reformat new code? I see two blanks in this line and above line

we will take care

qiluo-msft · 2022-03-01T21:48:52Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+        return (int(vlan_id),) + mac_decimals(fdb["mac"]), int(vlan_id)
+
+    def get_tp_fdb_status(self, ent, fdb):
+        #TODO:add code for self type


#TODO:add code for self type

What is the plan on this TODO? possible to do in this PR?

qiluo-msft · 2022-03-01T21:53:00Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

-            return None
-        return (int(vlan_id),) + mac_decimals(fdb["mac"])
+            return None, None 
+        return (int(vlan_id),) + mac_decimals(fdb["mac"]), int(vlan_id)


int(vlan_id)

Since normally it is the head element in original return tuple. No need to add extra return value.

I will check and change

qiluo-msft · 2022-03-01T21:59:11Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+                    else:
+                        self.vlan_dynamic_count_map[vlanid] = 1
+                self.vlan_id_list.append(vlanid)
+            except:


except

What is the intention of this except? It catches all types, which is not common.

dictionary access error if SAI_FDB_ENTRY_ATTR_TYPE key does not exist in ent

qiluo-msft · 2022-03-01T21:59:28Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+        return self.tp_fdb_status_map.get(sub_id, None)
+
+    def get_vlan_version(self):
+        return 2


2

Explain magic number?

The IEEE 802.1Q VLAN Version number. Reported as “1” by Bridges that support only SST
operation, and reported as “2” by Bridges that support MST operation;

SuvarnaMeenakshi · 2022-03-01T23:25:07Z

src/sonic_ax_impl/mibs/ietf/rfc4188.py

+
+        for vmem_entry in vlanmem_entries:
+            ifname = vmem_entry.split('|')[2]
+            #if_index = port_util.get_index_from_str(ifname)


Remove comment.

SuvarnaMeenakshi · 2022-03-02T00:04:08Z

src/sonic_ax_impl/mibs/ietf/rfc4188.py

+            if_index = mibs.get_index_from_str(ifname)
+            if if_index is None:
+                continue
+            self.dot1dbase_port_map[if_index-1] = if_index


why is " if_index-1" used as the OID index?
In interface MIB, if index is directly used as OID.
Example: https://github.com/Azure/sonic-snmpagent/blob/master/src/sonic_ax_impl/mibs/ietf/rfc1213.py#L298

As per if-mib ifIndex should be (1..2147483647) . if_index is 1 based not zero based.

SuvarnaMeenakshi · 2022-03-02T00:19:09Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

@@ -79,7 +108,10 @@ def update_data(self):
                mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': {}.".format(fdb_str, e))
                continue

-            ent = Namespace.dbs_get_all(self.db_conn, mibs.ASIC_DB, s, blocking=True)
+            try:


Why this change? if blocking=false, why try catch is required?

Just to continue with next iteration in case if we get any exception

SuvarnaMeenakshi · 2022-03-02T00:40:52Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+class FdbTableUpdater(MIBUpdater):
+    def __init__(self):
+        super().__init__()
+        self.db_conn = mibs.init_db()


Currently we do not have support for the exising FDB MIB for multi-asic platform.
But will be good to keep it in sync to use Namespace.init_namespace_dbs() instead of mibs.init_db().
Same is used in fdb_updater as well.
https://github.com/Azure/sonic-snmpagent/blob/master/src/sonic_ax_impl/mibs/ietf/rfc4363.py#L12

we will take care

SuvarnaMeenakshi · 2022-03-02T00:59:33Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+                    bs_hex = " ".join([format(i, '02x') for i in bs])
+                    return bs_hex
+            else:
+                return '0'


could define the default return value instead of using a magic string.

I have defined ZERO_STRING = '0' and using this above. I hope this is what you are expecting.

SuvarnaMeenakshi · 2022-03-02T00:59:47Z

src/sonic_ax_impl/mibs/ietf/rfc4363.py

+                    bs_hex = " ".join([format(i, '02x') for i in bs])
+                    return bs_hex
+            else:
+                return '0'


same comment as above

SuvarnaMeenakshi · 2022-03-02T01:06:34Z

tests/test_fdb.py

@@ -114,7 +114,7 @@ def test_getnextpdu_empty(self):
        get_pdu = GetNextPDU(
            header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0),
            oids=(
-                ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 3)),
+                ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 4)),


Why is this change required here? what is the corresponding mock_db/code change?

Here we need to provide oid that is not supported. Since (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 3) is supported, we added next oid which is not supported

…nmpagent into snmp_bridge_q_bridge_mib

lgtm-com · 2022-03-14T09:18:29Z

This pull request introduces 2 alerts when merging bfc9f6e into 6bd51c4 - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'

support for some bridge-mib and q-bridge-mib objects

1adb270

zhangyanzhao requested review from qiluo-msft and SuvarnaMeenakshi January 25, 2022 22:46

qiluo-msft reviewed Mar 1, 2022

View reviewed changes

SuvarnaMeenakshi reviewed Mar 1, 2022

View reviewed changes

SuvarnaMeenakshi reviewed Mar 2, 2022

View reviewed changes

Merge branch 'master' of https://github.com/suresh-rupanagudi/sonic-s…

bfc9f6e

…nmpagent into snmp_bridge_q_bridge_mib

changes based on review comments

feff673

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for some bridge-mib and q-bridge-mib objects #227

support for some bridge-mib and q-bridge-mib objects #227

suresh-rupanagudi commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

lguohan commented Mar 1, 2022

azure-pipelines bot commented Mar 1, 2022

qiluo-msft commented Mar 1, 2022

qiluo-msft Mar 1, 2022

suresh-rupanagudi Mar 10, 2022

qiluo-msft Mar 1, 2022

suresh-rupanagudi Mar 10, 2022

qiluo-msft Mar 1, 2022

suresh-rupanagudi Mar 10, 2022

qiluo-msft Mar 1, 2022

suresh-rupanagudi Mar 10, 2022

qiluo-msft Mar 1, 2022

suresh-rupanagudi Mar 10, 2022

SuvarnaMeenakshi Mar 1, 2022

SuvarnaMeenakshi Mar 2, 2022

suresh-rupanagudi Mar 10, 2022

SuvarnaMeenakshi Mar 2, 2022

suresh-rupanagudi Mar 10, 2022

SuvarnaMeenakshi Mar 2, 2022

suresh-rupanagudi Mar 10, 2022

SuvarnaMeenakshi Mar 2, 2022

suresh-rupanagudi Mar 11, 2022

SuvarnaMeenakshi Mar 2, 2022

SuvarnaMeenakshi Mar 2, 2022

suresh-rupanagudi Mar 10, 2022

lgtm-com bot commented Mar 14, 2022

support for some bridge-mib and q-bridge-mib objects #227

Are you sure you want to change the base?

support for some bridge-mib and q-bridge-mib objects #227

Conversation

suresh-rupanagudi commented Jul 15, 2021

snmpwalk -v2c -c public 10.59.143.175 1.3.6.1.2.1.17

lgtm-com bot commented Jul 15, 2021

lguohan commented Mar 1, 2022

azure-pipelines bot commented Mar 1, 2022

qiluo-msft commented Mar 1, 2022

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

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

lgtm-com bot commented Mar 14, 2022