-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
S3IP sysfs specification and S3IP sysfs framework HLD #1068
Conversation
Can you please include the full name of S3IP in the HLD? Thanks. |
@Pettershao want to be the reviewer of this HLD. |
/sys_switch/curr_sensor/curr[n]/max|R/W|int|Alarm threshold, unit: mA | ||
/sys_switch/curr_sensor/curr[n]/min|R/W|int|Alarm recovery threshold, unit: mA | ||
/sys_switch/curr_sensor/curr[n]/value|RO|int|current unit: mA | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a Power Sensor sysfs
section. This would provide the power in milliwatts from various power-sensing devices throughout the platform. Most on-board DC-DC converters (also called PWM or pulse-width modulator chips) provide voltage, current and calculated power. They often provide power In and Power out, allowing easy measurement of efficiency. It might be more reliable to read this directly rather than compute it from current * voltage, which might obtain readings at different times and result in invalid computation. It could also be more accurate because multiplying current * power in the controller results in compound loss of precision (rounded value * rounded value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to this, the requirement to directly collect the Power sensor was not collected (because it can be obtained normally via Current *voltage), so it was not covered in the first phase.
This is a good point, we can probably add this sysfs definition in the next stage.
|/sys_switch/fan/fan[n]/status |RO| enum|Fan states are defined as follows:<br>0: not present<br>1: present and normal<br>2: present and abnormal | ||
|/sys_switch/fan/fan[n]/led_status |R/W| enum| The fan status lights are defined as follows:<br>See the definition of enumeration value of LED status light for details | ||
|
||
### 7. Power information sysfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better title would be PSU Information sysfs
, since the data comprise many individual readings including power, current, voltage, inventory info, etc. all related to system-level PSUs. Changing the title would also more clearly differentiate between the proposed Power Information sysfs
in the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, PSU Information sysfs would be more suitable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great proposal, please see comments.
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|
||
The S3IP sysfs specification is represented as an organized sysfs directory structure on white-box devices. Device management software and debugging tools need to access hardware through this interface. | ||
|
||
Both vendors and users should comply with the S3IP sysfs specification. Device vendors focus on the specification implementation, while users verify the usability of the device against this specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if stronger language should be used to set expectations about how thoroughly vendors implement this specification. For example, a switch main board might have 10-20 sensors; language in this document might specify that "Vendors SHOULD provide software access to every sensor capable of being read. For any available sensor, vendors SHALL provide S3IP sysfs access."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, accept, I'll add it
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|
||
#### 2.2 Design of the S3ip sysfs specification | ||
Design Principles | ||
- Unified, specificationized behavior, consistent experience among all SONiC devices, regardless of underlying platform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the 2.0 platform APIs does the same thing, isn't it? These APIs are platform independent and platform vendors can implement them as they need i.e.
- Either read them directly from the SysFS created by device drivers, or
- Read them raw and then apply some filters/masks/pre-processing steps to get the final value
What is the reason/motivation behind the S3IP SysFS?
With S3IP, I see you are able to manage 1 but what about point#2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already discussed the motivations of S3IP in separate meetings.
The S3IP SYSFS specification was proposed. because
-
For ODM users, hardware data conforming to S3IP SYSFS specification is conducive to in-depth processing and intelligent fault identification and prediction.
-
For ODM vendors, S3IP SYSFS is easy to understand and implement
The S3IP SYSFS framework is light and has low learning and maintenance costs
I have added a section explaining the differences between S3IP SYSfs and PDDF. Some contents are marked TBD, please help to complete them. thanks
|
||
``` | ||
|
||
Sample code exposes watchdog register to sysfs, This code should be implemented by the vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vendors can have drivers which are common to various NOS. Isn't the S3IP framework mandating them to create SONiC specific drivers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We consider the case of the common driver, so by the configuration file, the path of the common driver, soft link to S3IP sysfs directory, do not need to create SONiC specific drivers
|
||
SONiC is designed to be portable to a variety of network devices. Many devices share the same ASIC platform and only differ in device-specific hardware components, such as PSUs, fan modules, and environment sensors. Currently, ODM vendor provides drivers to expose the device-specific hardware through sysfs, allowing SONiC to communicate with them, as described in the [Porting Guide](https://github.com/sonic-net/SONiC/wiki/Porting-Guide). However, many inefficient porting works are still required for SONiC developers due to different drivers from different devices. | ||
|
||
The S3IP sysfs specification defines a unified interface to access peripheral hardware on devices from different vendors, making it easier for SONiC to support different devices and platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By my count, this is the third proposal to unify vendor device implementations for SONiC. Two others are:
- PDDF: https://github.com/sonic-net/SONiC/blob/master/doc/platform/brcm_pdk_pddf.md
- PIT: https://github.com/sonic-net/SONiC/pull/1014/files
How is this differentiated from the other two, and can this feature leverage anything from the other two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a section explaining the differences between S3IP SYSfs and PDDF. Please see this update.
3. Porting S3IP sysfs framework to platform project | ||
- Create the S3IP sysfs service configuration file. | ||
- Modify /s3ip/s3ip_sysfs_service/s3ip_sysfs_conf.json to create a configuration file | ||
- This file should be installed to /etc/s3ip/s3ip_sysfs_conf.json on the device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned in the review for PIT:
https://github.com/sonic-net/SONiC/pull/1014/files#r932428427
I am concerned that this will be yet another configuration file that platform vendors will have to generate and maintain when bringing platforms in to SONiC. Can the platform configuration be derived from already-existing platform configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We plan to be two-in-one with the PDDF framework. but in two steps. The first step was to integrate the S3IP SYSFS framework into the Sonic community.
At present, there are many vendors in china that comply with S3IP framework. The S3IP SYSFS Framework is integrated into the SONiC community so that Chinese ODM vendors can more easily contribute their existing platforms to the community, and other vendors can also have the opportunity to adapt their platforms according to S3IP specifications and expand their business opportunities in China. This is good for SONiC community ecology;
|/sys_switch/fan/fan[n]/led_status |R/W| enum| The fan status lights are defined as follows:<br>See the definition of enumeration value of LED status light for details | ||
|
||
### 7. PSU information sysfs | ||
Power Information The Sysfs path must be /sys_switch/psu/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend "Power Supply Unit" information for clarity, especially if you add a "Power Sensor" section in the future per #1068 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding #1068 (comment) how do intend to track this? Could you add a placeholder in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,I will update this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
| Item | S3IP sysfs|PDDF| | ||
|-|-|-| | ||
|Requirements| The requirements are put forward from the user's perspective, and the sysfs node is summarized from the actual operation experience. The goal is to unify the interface for device management | The requirements are proposed from the technical point of view.TBD | ||
|Ecological | Devices compliant with S3IP SYSFS specification have been widely used in data centers.<br> The [S3IP](http://www.s3ip.org/) project involved both vendors and users[S3IP] (including Tencent, Alibaba, Baidu, Kuaishou, Meituan, Jingdong and more than a dozen ODM vendors). Vendors and users complete a closed-loop of requirements. standards and debugging tools that have the ability to iterate continuously. | TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ecological -> Ecosystem is probably what you meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,accept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EcoSystem: (PDDF) PDDF is a new framework and it is developed in the SONiC context. Some ODM platforms are already using PDDF. PDDF is an underlying framework which ODMs can use for faster development but it does not exposes any fixed SysFS nodes to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this comment in the PDDF column for the row "EcoSystem"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|
||
#### 2.2 Design of the S3ip sysfs specification | ||
Design Principles | ||
- Unified, specificationized behavior, consistent experience among all SONiC devices, regardless of underlying platform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"specificationized" is not a real word, I suggest "well-specified"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,accept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor grammar suggestions, otherwise looks good.
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|
||
| Item | S3IP sysfs|PDDF| | ||
|-|-|-| | ||
|Requirements| The requirements are put forward from the user's perspective, and the sysfs node is summarized from the actual operation experience. The goal is to unify the interface for device management | The requirements are proposed from the technical point of view.TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*** Requirements: (PDDF) The requirements are proposed from technical point of view aimed at platform driver and APIs development in SONiC. Only those SysFS which are required by SONiC platform APIs are exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this comment in the PDDF column for the row "Requirements"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|-|-|-| | ||
|Requirements| The requirements are put forward from the user's perspective, and the sysfs node is summarized from the actual operation experience. The goal is to unify the interface for device management | The requirements are proposed from the technical point of view.TBD | ||
|Ecological | Devices compliant with S3IP SYSFS specification have been widely used in data centers.<br> The [S3IP](http://www.s3ip.org/) project involved both vendors and users[S3IP] (including Tencent, Alibaba, Baidu, Kuaishou, Meituan, Jingdong and more than a dozen ODM vendors). Vendors and users complete a closed-loop of requirements. standards and debugging tools that have the ability to iterate continuously. | TBD | ||
|Development Mode | Regular development model,<br>Programming is required to implement the requirements. <br>ODM venders need to provide professional driver support for customers,Customers validating device with sysfs| Data driven development model.TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Development Mode: (PDDF) ODM vendors can use common PDDF kernel drivers and user space common platform APIs. Only some platform specific device data needs to be provided by the ODMs in the form of JSON files. Validation is via usual SONiC CLIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this comment in the PDDF column for the row "Development Mode"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|Requirements| The requirements are put forward from the user's perspective, and the sysfs node is summarized from the actual operation experience. The goal is to unify the interface for device management | The requirements are proposed from the technical point of view.TBD | ||
|Ecological | Devices compliant with S3IP SYSFS specification have been widely used in data centers.<br> The [S3IP](http://www.s3ip.org/) project involved both vendors and users[S3IP] (including Tencent, Alibaba, Baidu, Kuaishou, Meituan, Jingdong and more than a dozen ODM vendors). Vendors and users complete a closed-loop of requirements. standards and debugging tools that have the ability to iterate continuously. | TBD | ||
|Development Mode | Regular development model,<br>Programming is required to implement the requirements. <br>ODM venders need to provide professional driver support for customers,Customers validating device with sysfs| Data driven development model.TBD | ||
|Flexible | 1.Bus independent, The hardware support:<br>Fan<br>PSU<br>System EEPROM<br>Transceivers<br>CPLD<br>FPGA<br>System LED<br>Temperature sensors<br>Current sensors<br>Voltage sensors<br>Slot<br>Watchdog<br><br>2.Support scenarios with many customization requirements, such as FPGA-Polling, BMC management hardware and firmware upgrades <br><br>3.Normalized SYSFS is easy for hardware fault identification and prediction <br><br>4.Easy to debug for ODM users, and they need not care about the bus topology |Support I2C bus, other bus support requires development work <br>Fan<br>PSU<br>System EEPROM<br>CPLD<br>Optic Transceivers<br>System LED control via CPLD<br>System Status Registers in CPLD<br>Temp Sensors <br><br> TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flexibility: (PDDF) PDDF can be used on the platforms which use I2C bus to communicate with the peripheral devices. Platforms which use BMC can also be brought up using PDDF. In future PDDF would be supported on platform using PCIE FPGA devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this comment in the PDDF column for the row "Flexibility"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already updated
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
- This specification defines the path name, permissions, data type, and data unit for each hardware | ||
- The sysfs path defined in this specification must exist, and the file content should be "NA" if no hardware is available. | ||
|
||
S3IP sysfs specifiction : [specifiction](/doc/s3ip_sysfs/s3ip_sysfs_specification.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling error, should be:
S3IP sysfs specification : specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
| Item | S3IP sysfs|PDDF| | ||
|-|-|-| | ||
|Requirements| The requirements are put forward from the user's perspective, and the sysfs node is summarized from the actual operation experience. The goal is to unify the interface for device management | The requirements are proposed from technical point of view aimed at platform driver and APIs development in SONiC. Only those SysFS which are required by SONiC platform APIs are exposed. | ||
|Ecosystem | Devices compliant with S3IP SYSFS specification have been widely used in data centers.<br> The [S3IP](http://www.s3ip.org/) project involved both vendors and users[S3IP] (including Tencent, Alibaba, Baidu, Kuaishou, Meituan, Jingdong and more than a dozen ODM vendors). Vendors and users complete a closed-loop of requirements. standards and debugging tools that have the ability to iterate continuously. | PDDF is a new framework and it is developed in the SONiC context. Some ODM platforms are already using PDDF. PDDF is an underlying framework which ODMs can use for faster development but it does not exposes any fixed SysFS nodes to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
complete a closed-loop of requirements. standards
Should the period be a comma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted
doc/s3ip_sysfs/s3ip_sysfs_hld.md
Outdated
|
||
PDDF is not incompatible with S3IP SYSFS, we will combine the two parts into two Phases: | ||
|
||
Phase 1: The S3IP SYSFS Framework is integrated into the SONiC community so that Chinese ODM vendors can more easily contribute their existing platforms to the community, and other vendors can also have the opportunity to adapt their platforms according to S3IP specifications and expand their business opportunities in China. This is good for SONiC community ecology; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecology -> ecosystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted
I'm not sure I fully understand your question. LGTM = "Looks good to me," sorry if my use of this acronym was ambiguous. I am OK with the PR. GitHub is saying someone with write access needs to approve, in other words a repo maintainer. |
OK,github's code review tool is also called LGTM (refer to LGTM.com), So there was a misunderstanding. |
@lguohan I have submitted the code PR for some time, and no further comments were received. I hope the 202211 version supports this feature, Please help to review it. Thanks |
@lguohan can you please check if this one can be merged? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me
@lguohan can you please help to check if this PR can be merged? Reviewers have approved and build passed. Thanks. |
@lguohan please help to review this HLD |
@lguohan would you please help to review and merge this PR? Thanks. |
@lguohan , all the code PRs are merged, can you please check if this PR can be merged as well? Thanks. |
@tianshangfei can you please rebase this PR? Seems like the info is out of sync with master branch |
|
@lguohan can you please help to review and merge this HLD PR? The code PRs have been merged. |
|/sys_switch/fan/fan[n]/motor_number |RO| int| Number of fan motors | ||
|/sys_switch/fan/fan[n]/direction |RO| enum| The duct types are defined as follows:<br>0: F2B, forward air duct <br>1: B2F, rear duct | ||
|/sys_switch/fan/fan[n]/ratio |R/W| int| Motor speed percentage, value range 0-100 | ||
|/sys_switch/fan/fan[n]/motor[n]/speed |RO| int| Speed value,unit: RPM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tianshangfei
Are you considering a Fantray as one fan unit? Or Fan[n] refers to individual fans inside each Fantray?
I have some confusion about the "Number of Fan Motors" field. If a Fantray is one fan unit, then "Front" and "Rear" fans can be marked as two motors which have different speed, max & min speeds, spped tolerance etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definitions of fans and motors are as follows: a device contains n fan units, and a fan unit contains 1-2 motors. Each motor may have a fault, so the speed, maximum and minimum speed, speed tolerance, etc. are collected by the motor. "F2B " and "B2F " are used to identify the direction of the fan unit. F2B is the abbreviation for front-to-back airflow.
272e86a
SONiC is designed to be portable to a variety of network devices. Many devices share the same ASIC platform and only differ in device-specific hardware components, such as PSUs, fan modules, and environment sensors. Currently, ODM vendor provides drivers to expose the device-specific hardware through sysfs, allowing SONiC to communicate with them, as described in the Porting Guide. However, many inefficient porting works are still required for SONiC developers due to different drivers from different devices.
The S3IP sysfs specification defines a unified interface to access peripheral hardware on devices from different vendors, making it easier for SONiC to support different devices and platforms.