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.

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

Vlan snmp #2

Closed
wants to merge 8 commits into from
Closed

Vlan snmp #2

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c467dc8
Revert "Revert "[rfc1213] Interface MIB add l3 vlan interfaces & aggr…
raphaelt-nvidia Mar 9, 2021
4e16579
Add vlan support in rfc2863
raphaelt-nvidia Apr 8, 2021
21678dd
Fix up asic_db.json
raphaelt-nvidia May 25, 2021
c9f495c
Changes after review by Stepan
raphaelt-nvidia May 25, 2021
c2375ab
Unit tests for rfc2863
raphaelt-nvidia May 25, 2021
cc61819
Fixed parameters for some caller of mibs.init_sync_d_lag_tables, and …
raphaelt-nvidia May 25, 2021
15cd0f2
Addressed review comments by Junchao
raphaelt-nvidia May 27, 2021
aa0d150
Added detailed explanations in _get_counter and get_counter
raphaelt-nvidia May 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 71 additions & 6 deletions src/sonic_ax_impl/mibs/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,23 @@

HOST_NAMESPACE_DB_IDX = 0

RIF_COUNTERS_AGGR_MAP = {
Copy link

Choose a reason for hiding this comment

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

should we move the map to the file which use it? i see it is only used in one place.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We don’t think so, as it seems to be the convention to define variables in init.py, even if they are only used in one other file. For example, Qi Luo recently added HOST_NAMESPACE_DB_IDX which you see above, defined in init.py and used only in rfc3433.py.

"SAI_PORT_STAT_IF_IN_OCTETS": "SAI_ROUTER_INTERFACE_STAT_IN_OCTETS",
"SAI_PORT_STAT_IF_IN_UCAST_PKTS": "SAI_ROUTER_INTERFACE_STAT_IN_PACKETS",
"SAI_PORT_STAT_IF_IN_ERRORS": "SAI_ROUTER_INTERFACE_STAT_IN_ERROR_PACKETS",
"SAI_PORT_STAT_IF_OUT_OCTETS": "SAI_ROUTER_INTERFACE_STAT_OUT_OCTETS",
"SAI_PORT_STAT_IF_OUT_UCAST_PKTS": "SAI_ROUTER_INTERFACE_STAT_OUT_PACKETS",
"SAI_PORT_STAT_IF_OUT_ERRORS": "SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_PACKETS"
}

RIF_DROPS_AGGR_MAP = {
"SAI_PORT_STAT_IF_IN_ERRORS": "SAI_ROUTER_INTERFACE_STAT_IN_ERROR_PACKETS",
"SAI_PORT_STAT_IF_OUT_ERRORS": "SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_PACKETS"
}

# IfIndex to OID multiplier for transceiver
IFINDEX_SUB_ID_MULTIPLIER = 1000
Copy link

Choose a reason for hiding this comment

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

where is this var used?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Apparently nowhere. Looks like it might have been obsoleted by some functions in physical_entity_sub_oid_generator.py. I removed it.


redis_kwargs = {'unix_socket_path': '/var/run/redis/redis.sock'}


Expand Down Expand Up @@ -136,6 +153,14 @@ def if_entry_table(if_name):
return 'PORT_TABLE:' + if_name


def vlan_entry_table(if_name):
"""
:param if_name: given interface to cast.
:return: VLAN_TABLE key.
"""
return 'VLAN_TABLE:' + if_name


def lag_entry_table(lag_name):
"""
:param lag_name: given lag to cast.
Expand Down Expand Up @@ -292,11 +317,51 @@ def init_sync_d_interface_tables(db_conn):

return if_name_map, if_alias_map, if_id_map, oid_name_map


def init_sync_d_rif_tables(db_conn):
"""
Initializes map of RIF SAI oids to port SAI oid.
:return: dict
"""
rif_port_map = {get_sai_id_key(db_conn.namespace, rif): get_sai_id_key(db_conn.namespace, port)
for rif, port in port_util.get_rif_port_map(db_conn).items()}
port_rif_map = {port: rif for rif, port in rif_port_map.items()}
logger.debug("Rif port map:\n" + pprint.pformat(rif_port_map, indent=2))

return rif_port_map, port_rif_map


def init_sync_d_vlan_tables(db_conn):
"""
Initializes vlan interface maps for SyncD-connected MIB(s).
:return: tuple(vlan_name_map, oid_sai_map, oid_name_map)
"""

vlan_name_map = port_util.get_vlan_interface_oid_map(db_conn)

logger.debug("Vlan oid map:\n" + pprint.pformat(vlan_name_map, indent=2))

# { OID -> sai_id }
oid_sai_map = {get_index_from_str(if_name): sai_id for sai_id, if_name in vlan_name_map.items()
Copy link

Choose a reason for hiding this comment

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

it always call get_index_from_str twice for an entry, maybe it is better to use a loop to generate oid_sai_map and oid_name_map together. Something like:

for sai_id, if_name in vlan_name_map.items():
    port_index = get_index_from_str(if_name)
    if not port_index:
        continue
    oid_sai_map[port_index] = sai_id
    oid_name_map[port_index] = if_name

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done, but it was also necessary to initialize the arrays before the loop.

# only map the interface if it's a style understood to be a SONiC interface.
if get_index_from_str(if_name) is not None}
logger.debug("OID sai map:\n" + pprint.pformat(oid_sai_map, indent=2))

# { OID -> if_name (SONiC) }
oid_name_map = {get_index_from_str(if_name): if_name for sai_id, if_name in vlan_name_map.items()
# only map the interface if it's a style understood to be a SONiC interface.
if get_index_from_str(if_name) is not None}

logger.debug("OID name map:\n" + pprint.pformat(oid_name_map, indent=2))

return vlan_name_map, oid_sai_map, oid_name_map


def init_sync_d_lag_tables(db_conn):
"""
Helper method. Connects to and initializes LAG interface maps for SyncD-connected MIB(s).
:param db_conn: database connector
:return: tuple(lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map)
:return: tuple(lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map, sai_lag_map)
"""
# { lag_name (SONiC) -> [ lag_members (if_name) ] }
# ex: { "PortChannel0" : [ "Ethernet0", "Ethernet4" ] }
Expand All @@ -316,7 +381,7 @@ def init_sync_d_lag_tables(db_conn):
lag_entries = db_conn.keys(APPL_DB, "LAG_TABLE:*")

if not lag_entries:
return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map
return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map, sai_lag_map

db_conn.connect(COUNTERS_DB)
lag_sai_map = db_conn.get_all(COUNTERS_DB, "COUNTERS_LAG_NAME_MAP")
Expand Down Expand Up @@ -345,7 +410,7 @@ def member_name_str(val, lag_name):
if idx:
oid_lag_name_map[idx] = if_name

return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, sai_lag_map
return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map, sai_lag_map

def init_sync_d_queue_tables(db_conn):
"""
Expand All @@ -360,7 +425,7 @@ def init_sync_d_queue_tables(db_conn):

# Parse the queue_name_map and create the following maps:
# port_queues_map -> {"port_index : queue_index" : sai_oid}
# queue_stat_map -> {"port_index : queue stat table name" : {counter name : value}}
# queue_stat_map -> {"port_index : queue stat table name" : {counter name : value}}
# port_queue_list_map -> {port_index: [sorted queue list]}
port_queues_map = {}
queue_stat_map = {}
Expand Down Expand Up @@ -424,7 +489,7 @@ class RedisOidTreeUpdater(MIBUpdater):
def __init__(self, prefix_str):
super().__init__()

self.db_conn = Namespace.init_namespace_dbs()
self.db_conn = Namespace.init_namespace_dbs()
if prefix_str.startswith('.'):
prefix_str = prefix_str[1:]
self.prefix_str = prefix_str
Expand Down Expand Up @@ -537,7 +602,7 @@ def dbs_get_all(dbs, db_name, _hash, *args, **kwargs):
db get_all function executed on global and all namespace DBs.
"""
result = {}
# If there are multiple namespaces, _hash might not be
# If there are multiple namespaces, _hash might not be
# present in all namespace, ignore if not present in a
# specfic namespace.
if len(dbs) > 1:
Expand Down
96 changes: 86 additions & 10 deletions src/sonic_ax_impl/mibs/ietf/rfc1213.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class DbTables(int, Enum):
class IfTypes(int, Enum):
""" IANA ifTypes """
ethernetCsmacd = 6
ieee8023adLag = 161
l3ipvlan = 136
ieee8023adLag = 161

class ArpUpdater(MIBUpdater):
def __init__(self):
Expand Down Expand Up @@ -192,8 +193,13 @@ def __init__(self):
self.lag_name_if_name_map = {}
self.if_name_lag_name_map = {}
self.oid_lag_name_map = {}
self.lag_sai_map = {}
self.mgmt_oid_name_map = {}
self.mgmt_alias_map = {}
self.vlan_oid_name_map = {}
self.vlan_name_map = {}
self.rif_port_map = {}
self.port_rif_map = {}

# cache of interface counters
self.if_counters = {}
Expand All @@ -202,6 +208,8 @@ def __init__(self):
self.if_alias_map = {}
self.if_id_map = {}
self.oid_name_map = {}
self.rif_counters = {}

self.namespace_db_map = Namespace.get_namespace_db_map(self.db_conn)

def reinit_data(self):
Expand All @@ -220,26 +228,56 @@ 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, \
self.vlan_oid_sai_map, \
self.vlan_oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_vlan_tables, self.db_conn)

self.rif_port_map, \
self.port_rif_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_rif_tables, self.db_conn)

def update_data(self):
"""
Update redis (caches config)
Pulls the table references for each interface.
"""
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])
self.if_counters[if_idx] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, \
mibs.counter_table(sai_id), blocking=True)

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, _ = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, self.db_conn)
self.oid_lag_name_map, \
self.lag_sai_map, self.sai_lag_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.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()
}

def get_next(self, sub_id):
"""
:param sub_id: The 1-based sub-identifier query.
Expand Down Expand Up @@ -281,6 +319,8 @@ def interface_description(self, sub_id):
return self.oid_lag_name_map[oid]
elif oid in self.mgmt_oid_name_map:
return self.mgmt_alias_map[self.mgmt_oid_name_map[oid]]
elif oid in self.vlan_oid_name_map:
return self.vlan_oid_name_map[oid]

return self.if_alias_map[self.oid_name_map[oid]]

Expand All @@ -296,13 +336,38 @@ def _get_counter(self, oid, table_name):
try:
counter_value = self.if_counters[oid][_table_name]
# truncate to 32-bit counter (database implements 64-bit counters)
counter_value = int(counter_value) & 0x00000000ffffffff
counter_value = counter_value & 0x00000000ffffffff
Copy link

Choose a reason for hiding this comment

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

is it safe to remove int()

Copy link
Owner Author

Choose a reason for hiding this comment

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

According to Stepan, yes. Originally, the int() was used to convert from string to int, but now self.if_counters contains integers.

# done!
return counter_value
except KeyError as e:
mibs.logger.warning("SyncD 'COUNTERS_DB' missing attribute '{}'.".format(e))
return None

def aggregate_counters(self):
"""
For ports with l3 router interfaces l3 drops may be counted separately (RIF counters)
add l3 drops to l2 drop counters cache according to mapping

For l3vlan map l3 counters to l2 counters
"""
for rif_sai_id, port_sai_id in self.rif_port_map.items():
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] = \
self.if_counters[port_idx][port_counter_name] + \
self.rif_counters[rif_sai_id][rif_counter_name]

for vlan_sai_id, vlan_name in self.vlan_name_map.items():
for port_counter_name, rif_counter_name in mibs.RIF_COUNTERS_AGGR_MAP.items():
vlan_idx = mibs.get_index_from_str(vlan_name)
vlan_rif_counters = self.rif_counters[vlan_sai_id]
if rif_counter_name in vlan_rif_counters:
self.if_counters.setdefault(vlan_idx, {})
self.if_counters[vlan_idx][port_counter_name] = \
vlan_rif_counters[rif_counter_name]


def get_counter(self, sub_id, table_name):
"""
:param sub_id: The 1-based sub-identifier query.
Expand All @@ -322,7 +387,13 @@ def get_counter(self, sub_id, table_name):
counter_value = 0
for lag_member in self.lag_name_if_name_map[self.oid_lag_name_map[oid]]:
counter_value += self._get_counter(mibs.get_index_from_str(lag_member), table_name)

sai_lag_id = self.lag_sai_map[self.oid_lag_name_map[oid]]
sai_lag_rif_id = self.port_rif_map[sai_lag_id]
if sai_lag_rif_id in self.rif_port_map:
table_name = getattr(table_name, 'name', table_name)
Copy link

Choose a reason for hiding this comment

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

why we need getattr here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Stepan said the purpose is to return the entry in table_name corresponding to 'name', but if there is no 'name' attribute, return table_name. There are other ways of doing it, but this is the most concise.

Copy link

Choose a reason for hiding this comment

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

It looks strange, could you add some comment here? I suppose table_name should be a str which has no 'name' attribute, could you please explain the situation that table_name would have a 'name' attribute?

if table_name in mibs.RIF_DROPS_AGGR_MAP:
rif_table_name = mibs.RIF_DROPS_AGGR_MAP[table_name]
counter_value += self.rif_counters[sai_lag_rif_id].get(rif_table_name, 0)
# truncate to 32-bit counter
return counter_value & 0x00000000ffffffff
else:
Expand Down Expand Up @@ -352,6 +423,8 @@ def _get_if_entry(self, sub_id):
elif oid in self.mgmt_oid_name_map:
if_table = mibs.mgmt_if_entry_table(self.mgmt_oid_name_map[oid])
db = mibs.CONFIG_DB
elif oid in self.vlan_oid_name_map:
if_table = mibs.vlan_entry_table(self.vlan_oid_name_map[oid])
elif oid in self.oid_name_map:
if_table = mibs.if_entry_table(self.oid_name_map[oid])
else:
Expand Down Expand Up @@ -456,6 +529,7 @@ def get_if_type(self, sub_id):

ethernetCsmacd(6), -- for all ethernet-like interfaces,
-- regardless of speed, as per RFC3635
l3ipvlan(136) -- Layer 3 Virtual LAN using IP
ieee8023adLag(161) -- IEEE 802.3ad Link Aggregate
"""
oid = self.get_oid(sub_id)
Expand All @@ -464,6 +538,8 @@ def get_if_type(self, sub_id):

if oid in self.oid_lag_name_map:
return IfTypes.ieee8023adLag
elif oid in self.vlan_oid_name_map:
return IfTypes.l3ipvlan
else:
return IfTypes.ethernetCsmacd

Expand Down
Loading