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

Add HLD for H/W capabilities fields in platform.json #768

Conversation

ArunSaravananBalachandran
Copy link
Contributor

Add H/W capabilities fields in platform.json

- Attribute specific fields:
- status led - "color" - A list of the supported colors.
- speed
- "minimum" - Minimum recommended fan speed that can be set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please elaborate on who will be the consumer of this info? Also as platform facts for the test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, these fields would be used as platform facts for platform API test suite.
But, these can also be consulted by any platform daemon before executing the respective APIs.

Copy link

@zzhiyuan zzhiyuan May 11, 2021

Choose a reason for hiding this comment

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

Since platform daemons should be using the platform API, I think some of these things should just be taken care of by vendor-side logic. Like the fan speeds are completely handled by platform API instead of thermalctld.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zzhiyuan: I'm not sure I understand your comment. The main usage of these values is for testing purposes to understand boundaries when testing the device. Can you please elaborate?

Choose a reason for hiding this comment

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

Ah, I meant more for the latter part, "these can also be consulted by any platform daemon before executing the respective APIs."

Since there is already the fantray abstraction, I believe it is better for platform vendor to control the min/max fan speed which may differ by model, via thermal manager. Having values in platform.json for purpose of testing is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Yes, I agree with you. Platform daemons really shouldn't be referring to these values.

@jleveque
Copy link
Contributor

@zzhiyuan, @andywongarista, @keboliu: Please review, as you may be able to utilize this to improve your platform API testing.


For each component's attribute, the defined `capabilities` fields are as follows:

- "controllable" : A boolean, 'true' if the given attribute can be controlled from the NOS, 'false' otherwise.

Choose a reason for hiding this comment

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

Can you elaborate in this context what controllable from the NOS mean? Most of these attributes would be managed by a daemon, either directly or via vendor code. So if say you change the fan speed, the thermalctld would change it back, and you'd have to stop the container in order to change it. Is this deemed controllable?

Choose a reason for hiding this comment

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

Can you maybe reword to:

- "controllable" : Boolean, 'true' if the given attribute can be controlled from the NOS, 'false' if it cannot. Defaults to 'true.'

So you don't need the note below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate in this context what controllable from the NOS mean? Most of these attributes would be managed by a daemon, either directly or via vendor code. So if say you change the fan speed, the thermalctld would change it back, and you'd have to stop the container in order to change it. Is this deemed controllable?

If a given platform component can be controlled from the NOS (software-controlled), it is considered to be 'controllable' and if a platform component is controlled by a dedicated platform controller (like BMC) and is not controllable from the NOS running on the CPU, it is considered as 'not controllable'.

@jleveque jleveque changed the title Add H/W capabilities fields in platform.json Add HLD for H/W capabilities fields in platform.json Jun 29, 2021
@jleveque jleveque merged commit bb70e89 into sonic-net:master Jun 29, 2021
sujinmkang pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Nov 10, 2021
…8820)

Why I did it
Include validation of chassis dict in platform.json unit test
Based on: sonic-net/SONiC#768

How I did it
Update platform_json_checker to validate fields in chassis dict.

How to verify it
Verified that the unit test reports success for correct values of existing and capabilities fields in platform.json
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