-
Notifications
You must be signed in to change notification settings - Fork 87
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
[port_util] add get_rif_port_map, get_vlan_interface_oid_map #78
Conversation
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
This pull request introduces 2 alerts when merging cabfb62 into 132f8d5 - view on LGTM.com new alerts:
|
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
SONIC_PORTCHANNEL_RE_PATTERN = "^PortChannel(\d+)$" | ||
SONIC_MGMT_PORT_RE_PATTERN = "^eth(\d+)$" | ||
|
||
|
||
class BaseIdx: | ||
ethernet_base_idx = 1 | ||
vlan_interface_base_idx = 2000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny @judyjoseph do you think it leaves enough indexes to portchannels in multiple-ASIC case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel 1000 port channel interfaces should be good enough even for Chassis. But I am looking to see why the vlan_interface_base_idx is defined .. is it for this API get_vlan_interface_oid_map() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this just simply leaves a gap from 6000-9000 after 2000+4094
, why not start from 4000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2K BaseIdx is what our Arch and Guohan agreed on. If there is any concern about the size of pool, should discuss with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I am looking to see why the vlan_interface_base_idx is defined .. is it for this API get_vlan_interface_oid_map() ?
Theoretically index can be used anywhere where a numeric identification for interface is needed.
In my case I am using it for interface ID in the SNMP Interface MIB (RFC1213).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm .. I see that the index range discussion already happened.
""" | ||
db.connect('COUNTERS_DB') | ||
rif_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_NAME_MAP', blocking=True) | ||
rif_type_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_TYPE_MAP', blocking=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also check None? #Closed
src/swsssdk/port_util.py
Outdated
return {} | ||
|
||
oid_pfx = len("oid:0x") | ||
vlan_if_name_map = {sai_oid[oid_pfx:]: if_name for if_name, sai_oid in rif_name_map.items() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement is too long to understand. Could you refactor to a loop with if-block? And add some comment to explain? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Add 2 new utility functions, to be used by snmpagent.
Related to #133