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

Extend rfc3433.py to support more Physical Entity Sensor MIB entries #211

Merged

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Apr 16, 2021

- What I did

Extend RFC3433 implementation with:

  1. FAN tachometers
  2. PSU current sensor
  3. PSU voltage sensor
  4. PSU power sensor
  5. PSU temp sensor
  6. Chassis temp sensor

MIB HLD update PR to reflect this change please refer to: sonic-net/SONiC#766

A fix for the LGTM checker

- How I did it

  1. Refactor sensor data parsing class by adding a base class BaseSensorData; inherit TransceiverSensorData, PSUSensorData, FANSensorData, and ThermalSensorData from it to reduce redundant code.
  2. Adding more sensor MIB entry class: PSUTempSensor, PSUVoltageSensor, PSUCurrentSensor, PSUPowerSensor, FANSpeedSensor, and ThermalSensor.
  3. Separate MIB update to different functions according to different sensors types: update_xcvr_dom_data, update_psu_sensor_data, update_fan_sensor_data, and update_thermal_sensor_data.
  4. Add unit test cases to cover the new added MIB entries.
  5. Add lgtm.yaml to fix the LGTM checker.

- How to verify it

Manual test and run updated community SNMP test case(sonic-net/sonic-mgmt#3357).

- Description for the changelog

    Fan tachometers
    Thermal sensors
    PSU Voltage, Current, Power

Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
…tests/test_sensor.py

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
Copy link
Collaborator Author

keboliu commented Apr 27, 2021

@SuvarnaMeenakshi would you please help to review?

Copy link
Contributor

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

src/sonic_ax_impl/mibs/ietf/rfc3433.py Outdated Show resolved Hide resolved
src/sonic_ax_impl/mibs/ietf/rfc3433.py Outdated Show resolved Hide resolved
tests/namespace/test_sensor.py Show resolved Hide resolved
@keboliu
Copy link
Collaborator Author

keboliu commented May 5, 2021

@SuvarnaMeenakshi all your comments have been addressed, please check again.

@keboliu
Copy link
Collaborator Author

keboliu commented May 6, 2021

@SuvarnaMeenakshi please check my latest commit. and for the LGTM warnings, all of them are legacy code not touched in this PR.

Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi left a comment

Choose a reason for hiding this comment

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

LGTM

@keboliu
Copy link
Collaborator Author

keboliu commented May 8, 2021

@SuvarnaMeenakshi would you please help to restart the azure pipeline? seems one of the check is blocking.

@liat-grozovik
Copy link
Collaborator

@qiluo-msft do we need additional approval?

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 211 in repo Azure/sonic-snmpagent

@liat-grozovik
Copy link
Collaborator

@qiluo-msft can you please help to rerun azure pipeline? it is pending few days and i have no privilege to do so

@qiluo-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keboliu
Copy link
Collaborator Author

keboliu commented May 11, 2021

@qiluo-msft seems that the LGTM warning is not related to the change in the PR, and not even in the files that touched this time, what's your view? and also the "snmpagent" seems never going to be finished, it just waiting there after you started 2 days ago.

@qiluo-msft
Copy link
Contributor

I tested and fixed lgtm.yml. Please check and cherry-pick #216

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
@keboliu
Copy link
Collaborator Author

keboliu commented May 18, 2021

@SuvarnaMeenakshi @qiluo-msft Thanks for the review and fix for the LGTM checker. now everything passed after cherry-picking the LGTM checker fix, please help to proceed.

@qiluo-msft qiluo-msft merged commit fb6c940 into sonic-net:master May 18, 2021
@keboliu keboliu deleted the physical_entity_sensor_mib_extension branch May 18, 2021 05:24
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request May 18, 2021
…#3357)

Extend test_snmp_phy_entity to cover new add phyEntitySensor MIB entries in PR sonic-net/sonic-snmpagent#211

What is the motivation for this PR?
cover new added MIB entries in physical entity sensor MIB(RFC3433)

How did you do it?
Extend current test cases to check the correctness of the newly added MIB entries.

How did you verify/test it?
Run changed test_snmp_phy_entity

Signed-off-by: Kebo Liu <kebol@nvidia.com>
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
…sonic-net#3357)

Extend test_snmp_phy_entity to cover new add phyEntitySensor MIB entries in PR sonic-net/sonic-snmpagent#211

What is the motivation for this PR?
cover new added MIB entries in physical entity sensor MIB(RFC3433)

How did you do it?
Extend current test cases to check the correctness of the newly added MIB entries.

How did you verify/test it?
Run changed test_snmp_phy_entity

Signed-off-by: Kebo Liu <kebol@nvidia.com>
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.

4 participants