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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
May 18, 2021
Merged

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

merged 9 commits into from
May 18, 2021

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Apr 16, 2021 •
edited
Loading

- 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

@keboliu
Extend RFC3433 to support more sensors including:
6aa3dc6 
    Fan tachometers
    Thermal sensors
    PSU Voltage, Current, Power

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
reduce the redundant code
7b62632 
Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
Merge branch 'master' into physical_entity_sensor_mib_extension
ad2e66f
@keboliu
import TransceiverSensorData from new sensor_data.py
01313dc 
Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
Update test mock state_db.json for new added unit test
fddab17 
Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu keboliu mentioned this pull request Apr 18, 2021
Merged
3 tasks
@keboliu
Make the function names in tests/namespace/test_sensor.py align with …
a093bbc 
…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?

SuvarnaMeenakshi
SuvarnaMeenakshi requested changes Apr 28, 2021
View reviewed changes
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 •
edited
Loading

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

SuvarnaMeenakshi
SuvarnaMeenakshi previously approved these changes May 6, 2021
View reviewed changes
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 •
edited
Loading

@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 keboliu mentioned this pull request May 10, 2021
Merged
@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.

@keboliu keboliu closed this May 13, 2021
@keboliu keboliu reopened this May 13, 2021
@qiluo-msft qiluo-msft mentioned this pull request May 14, 2021
Closed
@qiluo-msft
Copy link
Contributor

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

@qiluo-msft @keboliu
Add lgtm.yml
3c19b14 
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
@keboliu
[SNMP] Extend snmp test to cover new added phyEntitySensor MIB change (…
4900b2e 
…#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>
@liorghub liorghub mentioned this pull request Aug 11, 2021
Open
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
@keboliu @vmittal-msft
[SNMP] Extend snmp test to cover new added phyEntitySensor MIB change (…
83768cc 
…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
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@SuvarnaMeenakshi SuvarnaMeenakshi SuvarnaMeenakshi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keboliu @liat-grozovik @qiluo-msft @SuvarnaMeenakshi