Missing fan present object in fansensor (and expected usage of object manager)

[Lei Yu]

On Fri, Dec 2, 2022 at 2:07 AM Patrick Williams <patrick at stwcx.xyz> wrote:
>
> On Thu, Dec 01, 2022 at 09:38:17PM +0800, Lei Yu wrote:
> > This email is to discuss an issue found in fansensor service, and
> > about the expected usage of object manager.
> >
> > # The issue
> > With upstream dbus-sensors (7627c86), the fan-present objects are
> > missing on DBus.
> > The fansensor service should create objects like
> > `/xyz/openbmc_project/inventory/fan0` to represent the fan presence
> > status [1].
> > However, with the changes of "fixing the ObjectManager in the right
> > place"[2], it creates an object manager with
> > `/xyz/openbmc_project/sensors` instead of `/`, and the objects created
> > on different object paths are actually not created anymore.
> >
> > I see there is a fix for the "control" objects in the fanensor[3], but
> > the fan-present objects are still missing.
> >
> > The fix is simple that we could add an extra object manager on
> > `/xyz/openbmc_project/inventory/`.
> >
> > # Expected usage of object manager
> > But that involves extra object managers.
>
> There isn't really any harm in extra object managers.  They are "cheap"
> as they're effectively just a string held in the sd-bus library.
>
> > For a service that creates objects on different paths (e.g. sensors,
> > control, inventory), should it really create different object
> > managers?
>
> We've had some ambiguity in where the object manager should be hosted.
> From a dbus perspective there is no requirement, but it makes it
> difficult to call GetManagedObjects.  The direction that we've been
> going is that:
>
>     1. Some hierarchies are now explicitly documenting where an ObjectManager
>        is required (like sensors does now).  This should generally go at
>        the level 1 deeper than "openbmc_project".

I guess it's better to document more requirements to help daemon to
host the objects on expected paths. E.g. for
inventories/control/settings

>
>     2. Daemons can still optionally host an ObjectManager at the root if
>        this is somehow helpful.
>
> > The caller of such service (e.g. bmcweb) usually calls
> > `GetManagedObjects` to the object manager interface to get the
> > objects. In the above case, it has to call `GetManagedObjects`
> > multiple times on different paths.
>
> The reason for having the ObjectManager lower in the hierarchy is so
> that the GetManagedObjects call is cheaper and doesn't give you every
> object hosted by that service.  In something like the PLDM daemon this
> might be lots of objects that the particular call has no interest in.
>
> I don't suspect there is really any case, with the current
> implementation, where bmcweb is going to attempt to prune the calls by
> grabbing objects from two different hierarchies in a single call.  We've
> decided to simplify the "where do I find the ObjectManager" instead of
> optimizing the performance of "give me exactly what I want with this specific
> query".

Agreed.

>
> > So the question is, what is the *expected* and *proper* way to use
> > object manager for such case?
> >
> > [1]: https://github.com/openbmc/dbus-sensors/blob/master/src/TachSensor.cpp#L77
> > [2]: https://github.com/openbmc/dbus-sensors/commit/14ed5e99
> > [3]: https://github.com/openbmc/dbus-sensors/commit/d9067251
>
> FWIW, Nan fixed[1] the same issue you're seeing in dbus-sensors in the
> phosphor-health-monitor by adding the ObjectManager at the root.  We
> don't currently require one at `/xyz/openbmc_project/inventory` but I
> wouldn't be surprised if we do in the not-too-distant future.

So for this fansensor case, let's add the object that hosts
/xyz/openbmc_project/inventory
Will post a patch.

>
> 1. https://github.com/openbmc/phosphor-health-monitor/commit/af109947dad9c6dbf496c4111c625e5ae407dd81
>
> --
> Patrick Williams



-- 
BRs,
Lei YU