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

ASIC Thermal Monitoring HLD #557

Merged
merged 3 commits into from
Oct 22, 2020
Merged

ASIC Thermal Monitoring HLD #557

merged 3 commits into from
Oct 22, 2020

Conversation

padmanarayana
Copy link
Contributor

This document describes the high level design of a poller for ASIC thermal sensors.

@jleveque
Copy link
Contributor

@keboliu, @Junchao-Mellanox: Please review.

keboliu
keboliu reviewed Mar 8, 2020
View reviewed changes
doc/asic_thermal_monitoring_hld.md Show resolved Hide resolved
doc/asic_thermal_monitoring_hld.md Outdated
```

The ThermalBase() implementation for ASIC sensors will return the latest values exported by the SwitchOrch to the stateDB's TEMPERATURE_INFO table.
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks strange to me, the current idea is that platform API responsible to get the thermal sensor info from the lower layer and populate it to DB, and state DB info will feed CLI/SNMP or telemetry etc. but here you are suggesting platform API to read sensor info from DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The objective is to present ASIC internal sensors to any advanced thermal control algorithm. The reason it appears dissimilar to the standard platform sensors is because the intent was not to let pmon (or any platform daemon) make SAI calls (and create a new dependency) since this is a unique case. Hence letting the poller be part of SwitchOrch.

keboliu
keboliu suggested changes Mar 10, 2020
View reviewed changes
Copy link
Collaborator

@keboliu keboliu 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 left above.

@jleveque
Copy link
Contributor

@keboliu: Please review responses.

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox and @keboliu appreciate your feedback

  • Is there any reason to have an additional thermal monitoring while there is an already valid solution for thermal control and new CLIs for doing so?
  • If there is something missing I believe it should be added on top of the existing functionality and not create new and parallel one.

jleveque
jleveque approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please wait for other reviewers.

@liat-grozovik liat-grozovik merged commit f73b167 into sonic-net:master Oct 22, 2020
@sujinmkang
Copy link
Collaborator

sonic-net/sonic-buildimage#6556
sonic-net/sonic-swss#1517
sonic-net/sonic-swss-common#419

@judyjoseph judyjoseph mentioned this pull request Jun 11, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleveque jleveque jleveque approved these changes

@keboliu keboliu keboliu approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@padmanarayana @jleveque @liat-grozovik @sujinmkang @keboliu