[sonic-snmpagent] SONiC physical entity mib extension by Junchao-Mellanox · Pull Request #168 · sonic-net/sonic-snmpagent

Junchao-Mellanox · 2020-10-16T06:16:45Z

- 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

Junchao-Mellanox · 2020-10-16T06:19:06Z

retest this please

Conflicts: src/sonic_ax_impl/mibs/ietf/rfc2737.py

qiluo-msft · 2020-10-26T05:18:13Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+    REPLACEABLE       = 'is_replaceable'
+
+@unique
+class ThermalInfoDB(bytes, Enum):


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

qiluo-msft · 2020-10-26T05:19:27Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+    'temperature': 'Temperature',
+    'power': 'Power',
+    'current': 'Current',
+    'voltage': 'Voltage'


align by : ? #Closed

qiluo-msft · 2020-10-26T05:22:08Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+    :param value: input string value
+    :return: True is string value is empty or equal to 'N/A' or 'None'
    """
+    if not value or value == NOT_AVAILABLE or value == str(None):


not value [](start = 7, length = 9)

isinstance(value, str) #Closed

qiluo-msft · 2020-10-26T05:22:21Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+    :param value: input string value
+    :return: True is string value is empty or equal to 'N/A' or 'None'
    """
+    if not value or value == NOT_AVAILABLE or value == str(None):


str(None) [](start = 55, length = 9)

use None ? #Closed

Not sure I understand.

I mean why not use 'None' directly?

In reply to: 511724801 [](ancestors = 511724801)

qiluo-msft · 2020-10-26T05:23:21Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+    """
+    ret = []
+    for field in enum_type:
+        value = info_dict.get(field.value, "")


"" [](start = 43, length = 2)

The default value could not be decode() as below #Closed

qiluo-msft · 2020-10-26T05:31:22Z

src/sonic_ax_impl/mibs/__init__.py

    return device_metadata

+
+def get_chassis_thermal_sub_id(position):


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

qiluo-msft · 2020-10-26T06:03:56Z

src/sonic_ax_impl/mibs/ietf/rfc2737.py

+        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


I see most contents are static, why we need to reinit_data() every time? #Closed

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?

qiluo-msft · 2020-10-26T06:10:25Z

Could you write a design/implementation document to help understand? #Closed

Junchao-Mellanox · 2020-10-26T06:22:47Z

Could you write a design/implementation document to help understand?

HLD: https://github.com/Junchao-Mellanox/SONiC/blob/phy-mibs/doc/snmp/extension-to-physical-entity-mib.md

lgtm-com · 2020-10-27T02:22:00Z

This pull request introduces 1 alert when merging 94957eb into 57e54d9 - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

Junchao-Mellanox · 2020-11-05T01:14:33Z

@qiluo-msft, I have fixed/replied all comments, could you pleast review it again? Thanks!

qiluo-msft · 2020-11-10T00:05:15Z

src/sonic_ax_impl/mibs/__init__.py

 from ax_interface.util import oid2tuple
 from sonic_ax_impl import logger

+from .physical_entity_sub_oid_generator import *


import * [](start = 40, length = 8)

Don't import wildcard in production code. #Closed

qiluo-msft · 2020-11-10T00:08:07Z

src/sonic_ax_impl/mibs/__init__.py

 from ax_interface.util import oid2tuple
 from sonic_ax_impl import logger

+from .physical_entity_sub_oid_generator import *


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

qiluo-msft

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

Junchao-Mellanox and others added 9 commits August 24, 2020 14:53

SONiC MIB Extension feature first commit

9640bc1

Add unit test cases and fix found issues

625f492

Add more comments

1228c97

fix issue found in manual tests

be0e817

Fix issue found in manual test

caf96dc

Fix issue: PSU fan is not in entity mib

10569b6

Add chassis name and description

b89cde1

Change physical index generating rule

e160479

Merge branch 'master' into phy-mibs

da2adb2

Junchao-Mellanox mentioned this pull request Oct 16, 2020

[sonic-snmpagent] SONiC physical entity mib extension Junchao-Mellanox/sonic-snmpagent#1

Closed

Junchao-Mellanox marked this pull request as ready for review October 16, 2020 08:01

Junchao-Mellanox mentioned this pull request Oct 21, 2020

[snmp] Add test cases for SONiC physical entity MIB feature sonic-net/sonic-mgmt#2379

Merged

3 tasks

Merge branch 'master' into phy-mibs

ce9e952

Conflicts: src/sonic_ax_impl/mibs/ietf/rfc2737.py

qiluo-msft reviewed Oct 26, 2020

View reviewed changes

Fix review comment

96d4897

Change all bytearray to str

94957eb

Junchao-Mellanox requested a review from qiluo-msft October 30, 2020 01:35

qiluo-msft reviewed Nov 10, 2020

View reviewed changes

qiluo-msft requested a review from SuvarnaMeenakshi November 10, 2020 00:27

Junchao-Mellanox added 2 commits November 10, 2020 16:14

Move physical_entity_sub_oid_generator to src/sonic_ax_impl/mibs/ietf

aa727d5

Fix review comment

9f8723b

Junchao-Mellanox requested a review from qiluo-msft November 11, 2020 02:34

qiluo-msft approved these changes Nov 12, 2020

View reviewed changes

qiluo-msft merged commit b8f19ee into sonic-net:master Nov 12, 2020

		return device_metadata


		def get_chassis_thermal_sub_id(position):

Conversation

Junchao-Mellanox commented Oct 16, 2020

Uh oh!

Junchao-Mellanox commented Oct 16, 2020

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Junchao-Mellanox Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Junchao-Mellanox Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Junchao-Mellanox commented Oct 26, 2020

Uh oh!

lgtm-com bot commented Oct 27, 2020

Uh oh!

Junchao-Mellanox commented Nov 5, 2020

Uh oh!

qiluo-msft Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft Oct 26, 2020 •

edited

Loading

qiluo-msft commented Oct 26, 2020 •

edited

Loading

qiluo-msft Nov 10, 2020 •

edited

Loading

qiluo-msft Nov 10, 2020 •

edited

Loading