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

Support Name field as primary thermal zone identifier and deemphasize Zone Index number #42

Open
Krellan opened this issue Nov 13, 2023 · 0 comments

Comments

@Krellan
Copy link
Member

Krellan commented Nov 13, 2023

It would be nice if thermal zones were primarily identified by the Name string, using the Zone Index number merely as a fallback.

Currently, it is the other way around.

When IPMI was the only external interface, zone index numbers were required, and the only way to access our IPMI thermal zone extensions: https://github.com/openbmc/phosphor-pid-control/blob/master/ipmi.md

However, now that we have Redfish, this limitation no longer exists, and we should be using names, not numbers, with Redfish.

  • The old config.json JSON format does not support the Name field, so thermal zones can not be named when using that format.
  • The new Entity Manager JSON format treats the ZoneIndex field, the thermal zone number, as optional. This behavior is correct, but the absence of the zone number can lead to problems elsewhere.
  • Because Entity Manager JSON parsing is in a random unpredictable order, not top to bottom through the file as it is with the old config.json parsing, zone numbers will not be consistent from run to run, and they will frequently be different than what the user expected them to be. This causes problems, and this is a good enough reason on its own to want to deprecate index numbers in favor of consistent names.
  • The D-Bus objects are named zone0, zone1, and so on, using only the zone number to influence their naming. They should instead have used the Name field to influence their naming, however, this would break many configurations that people already have in the field. Maybe we can instantiate two copies, to avoid breaking anybody. Name one copy after the zone number, as before, and name one copy after the Name string.
  • When instantiating D-Bus objects from user-given names, as always, be careful. Apply D-Bus object name escaping rules. Watch for duplicates, watch for nonexistent names (they are optional in the old config.json format), and watch for names that would conflict with the legacy zone0 automatic naming format.
  • There needs to be an easy and reliable way to look up the zone number given the name, and look up the name given the zone number. This will make the conversion process much easier. More fields should be added to the existing D-Bus objects, to add this.
  • Other programs need to have changes made similarly, and be audited to ensure that they are referencing zone names instead of zone index numbers, whenever possible. The program bmcweb is one example, as it sets up Redfish endpoints named Zone_0, Zone_1, and so on, again using only the zone number to influence their naming.
  • When making these fixes, to this and other programs, there may be D-Bus interface changes necessary, which would have a ripple effect.

The good news is that phosphor-pid-control cleanly takes care of a conflicting or missing zone number, and already auto-assigns the next available free zone number in sequence, starting with zero and going up from there:

// Auto-assign next unused zone number, using 0-based numbering

As an example of how the reliance on zone numbers causes problems elsewhere, I made this fix to bmcweb in order to avoid an error when the ZoneIndex field did not appear in the Entity Manager JSON:

https://gbmc-review.googlesource.com/c/gbmcweb/+/13314

Being able to use zone names, instead of relying on zone index numbers, will go a long way towards gaining acceptance of a standard Redfish equivalent to our existing IPMI thermal zone extensions.

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

No branches or pull requests

1 participant