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

[port_util] add get_rif_port_map, get_vlan_interface_oid_map #78

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

mykolaf
Copy link
Contributor

@mykolaf mykolaf commented Jun 1, 2020

Add 2 new utility functions, to be used by snmpagent.

Related to #133

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 1, 2020

This pull request introduces 2 alerts when merging cabfb62 into 132f8d5 - view on LGTM.com

new alerts:

  • 2 for Unused local variable

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
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@mykolaf mykolaf Jun 6, 2020

Choose a reason for hiding this comment

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

@judyjoseph

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

Copy link
Contributor

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)
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 3, 2020

Choose a reason for hiding this comment

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

also check None? #Closed

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()
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 3, 2020

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

@qiluo-msft qiluo-msft self-assigned this Jun 3, 2020
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
@qiluo-msft qiluo-msft merged commit a4d40f2 into sonic-net:master Jun 23, 2020
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.

6 participants