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

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

[Lei Yu]

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.
For a service that creates objects on different paths (e.g. sensors,
control, inventory), should it really create different object
managers?
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.

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

-- 
BRs,
Lei YU

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

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".

    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".

> 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.

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

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 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@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