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

Vlan snmp #2

Closed
wants to merge 8 commits into from
Closed

Vlan snmp #2

wants to merge 8 commits into from

Conversation

raphaelt-nvidia
Copy link
Owner

- 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 Azure#191, which was itself a revert of Azure#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.

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()

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.

@@ -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

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.

@@ -27,6 +27,23 @@

HOST_NAMESPACE_DB_IDX = 0

RIF_COUNTERS_AGGR_MAP = {

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.

}

# IfIndex to OID multiplier for transceiver
IFINDEX_SUB_ID_MULTIPLIER = 1000

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.

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)

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.

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?

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.

2 participants