Update BMC HLD with Redis communication for SONiC BMC - #2453
Conversation
Signed-off-by: Ben Levi <belevi@nvidia.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
Signed-off-by: Ben Levi <belevi@nvidia.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
|
Hi @judyjoseph This PR includes the design updates I presented in the "SONiC BMC WG Meeting". Thanks! |
|
|
||
| In summary, when the BMC runs OpenBMC, the Switch-Host NOS interacts with the BMC through the Redfish RESTful API. | ||
|
|
||
| When the BMC runs SONiC OS, the Switch-Host NOS interacts with the BMC through Redis over the host–BMC link (`usb0`). The host connects to the Redis instance on the BMC and reads platform data. |
There was a problem hiding this comment.
Why can't we implement the API's in SONiC Redfish too (barring the dump)? Should be fairly straightforward to implement. This way we keep the API layer clean and do away with the bmc os config too?
There was a problem hiding this comment.
As part of the SONiC BMC project architecture, the agreed-upon design was that communication between the host side and the BMC side would be via Redis. Dedicated Redfish endpoints will be added for specific use cases, but those are outside the scope of this PR.
Most of the existing BMC APIs (bmc_base.py) are OpenBMC-specific and are therefore not relevant for SONiC BMC. Examples include session management, resetting the root password, and firmware updates, which in our case will be handled directly on the BMC side.
However, the host still needs access to the BMC EEPROM data so that we can continue supporting the existing show platform bmc eeprom CLI. For that reason, the EEPROM information is retrieved from the SONiC BMC Redis database.
|
|
||
| In summary, when the BMC runs OpenBMC, the Switch-Host NOS interacts with the BMC through the Redfish RESTful API. | ||
|
|
||
| When the BMC runs SONiC OS, the Switch-Host NOS interacts with the BMC through Redis over the host–BMC link (`usb0`). The host connects to the Redis instance on the BMC and reads platform data. |
There was a problem hiding this comment.
I think this design needs another look. It exposes the BMCs internal OS details to every hostside consumer instead of encapsulating them behind a stable API abstraction. this way sharing implementation details across layers and tightly couples consumers(of BMC) to the BMC OS internals, which could make maintenance and future changes significantly harder. please think about it
| config bmc os openbmc | ||
| ``` | ||
|
|
||
| `config bmc os` writes `DEVICE_METADATA|bmc.os`. Subsequent BMC CLI/API calls on the Switch-Host use Redfish or Redis according to this setting. |
There was a problem hiding this comment.
why an operator need this API at all ? exposing config bmc os as a configurable option suggests the BMC OS is a runtime choice, which it is not.
I would recommend removing it, or at most exposing it as read only platform metadata. a get is ok but not set . please think about it.
|
|
||
| ### 6.3 Errors Handling: | ||
| - generate_dump check whether BMC is suppported (via bmc.json file). If not, BMC logic is skipped. | ||
| - `generate_dump` checks whether BMC Redfish dump is supported: Switch-Host, `bmc.json` present, and `get_bmc_os() == openbmc`. If not, host-side BMC dump logic is skipped. |
There was a problem hiding this comment.
get_bmc_os() == openbmcis not looking good .
ideally , this check should be be in only one place something like "ChassisBase.get_bmc()" ( from bmc.json) , all implementation of subclass creation like vendor specific custom code and transport strategy can grow independently
| | `get_serial()` | Yes | Yes | | ||
| | `get_revision()` | Yes | Yes | | ||
| | `get_status()` | Yes | Yes | | ||
| | `is_replaceable()` | Yes | Yes | |
There was a problem hiding this comment.
OpenBMC and SONiC BMC already provide almost all of the capabilities we need (EEPROM, presence, model/serial, status, version, etc.). For those common capabilities, we should expose a single interface that hides the underlying BMC OS.
The current design seems to push the BMC OS decision into the caller and we need to hard code like this
if (bmc.os == OpenBMC)
use OpenBMC-specific API
else
use SONiC BMC API
That means every consumer now needs to know which BMC OS is running, which creates unnecessary tight coupling.
My proposal :
- I think we should separate this by capability, not by BMC OS. Common functionality should always be accessed through Redfish (or another common API), regardless of whether the platform is running OpenBMC or SONiC BMC.
- only genuinely platform specific features should require a platform extension.
The underlying BMC OS should remain an implementation detail. hostside consumers should nt need to know or care about which BMC OS is running.
|
@benle7 thanks for this proposal, appreciate the effort here. @oleksandrivantsiv @benle7 , when you get a chance, could you take another look at some of the points above? I think considering them would help make the design cleaner and more maintainable in future . |
5 participants
This PR updates the existing BMC HLD, which was originally designed for communication with OpenBMC via Redfish.
The document has been extended to describe the required changes for supporting communication with a SONiC BMC through the BMC's Redis database.