[sonic-snmpagent] SONiC physical entity mib extension #168
Conversation
|
retest this please |
Conflicts: src/sonic_ax_impl/mibs/ietf/rfc2737.py
| REPLACEABLE = 'is_replaceable' | ||
|
|
||
| @unique | ||
| class ThermalInfoDB(bytes, Enum): |
There was a problem hiding this comment.
bytes [](start = 20, length = 5)
str instead of bytes?
Please be aware of latest change on master:
57e54d9 2020-10-22 | Interact with Redis by str instead of bytes, migrate to SonicV2Connector with `decode_responses=True` (#171)
``` #Closed
| 'temperature': 'Temperature', | ||
| 'power': 'Power', | ||
| 'current': 'Current', | ||
| 'voltage': 'Voltage' |
| """ | ||
| if not value or value == NOT_AVAILABLE or value == str(None): |
There was a problem hiding this comment.
not value [](start = 7, length = 9)
isinstance(value, str) #Closed
| """ | ||
| if not value or value == NOT_AVAILABLE or value == str(None): |
There was a problem hiding this comment.
str(None) [](start = 55, length = 9)
use None ? #Closed
There was a problem hiding this comment.
Not sure I understand.
There was a problem hiding this comment.
| """ | ||
| ret = [] | ||
| for field in enum_type: | ||
| value = info_dict.get(field.value, "") |
There was a problem hiding this comment.
"" [](start = 43, length = 2)
The default value could not be decode() as below #Closed
src/sonic_ax_impl/mibs/__init__.py
Outdated
| @@ -386,40 +505,89 @@ def get_device_metadata(db_conn): | |||
| device_metadata = db_conn.get_all(db_conn.STATE_DB, DEVICE_METADATA) | |||
| return device_metadata | |||
|
|
|||
|
|
|||
| def get_chassis_thermal_sub_id(position): | |||
There was a problem hiding this comment.
get_chassis_thermal_sub_id [](start = 4, length = 26)
Move non shared functions into rfc2737.py or another python file?
I think many functions and constants are related to the comment section above. Move them into a seperated file may be easier to read. #Closed
| name = 'MGMT' | ||
| self.physical_description_map[chassis_mgmt_sub_id] = name | ||
| self.physical_name_map[chassis_mgmt_sub_id] = name | ||
| self.physical_fru_map[chassis_mgmt_sub_id] = self.NOT_REPLACEABLE |
There was a problem hiding this comment.
I see most contents are static, why we need to reinit_data() every time? #Closed
There was a problem hiding this comment.
Because we clear all existing data before reinit_data(), see line 304~316. And in those data, we have both static/dynamic data, it is hard to tell which one is static or dynamic unless we hardcode it or add a tag for each data(which is even complicated). So I choose to reinit these static content every time. Any suggestion?
|
Could you write a design/implementation document to help understand? #Closed |
|
|
This pull request introduces 1 alert when merging 94957eb into 57e54d9 - view on LGTM.com new alerts:
|
|
@qiluo-msft, I have fixed/replied all comments, could you pleast review it again? Thanks! |
src/sonic_ax_impl/mibs/__init__.py
Outdated
| @@ -10,6 +10,8 @@ | |||
| from ax_interface.util import oid2tuple | |||
| from sonic_ax_impl import logger | |||
|
|
|||
| from .physical_entity_sub_oid_generator import * | |||
There was a problem hiding this comment.
import * [](start = 40, length = 8)
Don't import wildcard in production code. #Closed
src/sonic_ax_impl/mibs/__init__.py
Outdated
| @@ -10,6 +10,8 @@ | |||
| from ax_interface.util import oid2tuple | |||
| from sonic_ax_impl import logger | |||
|
|
|||
| from .physical_entity_sub_oid_generator import * | |||
There was a problem hiding this comment.
physical_entity_sub_oid_generator [](start = 6, length = 33)
This file is only used by rfc2737.py. You may move code to
- src/sonic_ax_impl/lib/
- or, src/sonic_ax_impl/mibs/ietf
And only import in required file. #Closed
- What I did
Add new physical entity fan drawer, fan, fan tachometers, PSU, PSU sensor, chassis sensors to physical entity mib.
Implement all mib objects defined in EntPhysicalEntry of RFC 2737.
- How I did it
Refactor rfc2737.py to have different mib updater for different physical entity: XcvrCacheUpdater, FanCacheUpdater, FanDrawerCacheUpdater, PsuCacheUpdater, ThermalCacheUpdater.
- How to verify it
Manual test on MSN2410
- Description for the changelog