DBus ObjectManager Interface usages within the project

DBus ObjectManager Interface usages within the project

[Ed Tanous]

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

We've had a couple cases in the project where patches have needed to be
slowed down because of inconsistencies in our use of object manager, so
IMO, the time has come to make our usage of ObjectManager consistent,
documented, and drop support for the (not upstream) daemons that don't
follow the rules.  As part of this, we will port forward all the upstream
daemons to be consistent, and do our best to avoid bugs, but this email is
intended to inform those that have non-upstream daemons about the change so
they can resolve their implementations.

The basics:
ObjectManager DBus interface will now be required for any daemon
implementing a Sensor.Value interface or Inventory.Item interface.

Daemons producing sensors will be required to place their ObjectManager at
/xyz/openbmc_project/sensors
Daemons producing inventory items will be required to place their
ObjectManager at /xyz/openbmc_project/inventory.

Both of these interfaces will be required to be published before claiming a
well known name on dbus, to allow for the possibility of caching
implementations at some point in future.

Functionally, this gets a little complicated because the
sdbusplus::asio::object_manager bindings in their default invocation create
an ObjectManager at the root object /, so we need to execute this in a few
steps.

0. Send this email out, outlining the problem, and warn the community that
if this is an assumption you make in your downstream daemons, those
assumptions will need to change.  Get consensus from maintainers.
1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value
and Inventory.Item to call out the required-ness of ObjectManager, and the
explicit dbus path where it's required to be placed.
2. Identify all the asio daemons that need changes, and publish changes
that move the object_manager to the appropriate path.  This is mostly going
to be a tree-wide grep for sdbusplus::asio::object_server, and look for
daemonst that don't make use of the add_manager() API.  Anyone directly
calling the Sensor or Invertory ObjectManager interfaces will need to port
to the new paths, but luckily, this isn't a very common operation, and I
beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In
bmcweb, there is actually a convoluted piece of code that uses the mapper
to sort out the location that the ObjectManager exists at so Redfish
sensors should remain consistent, and we don't yet have code that relies on
ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a
similar piece of code that should protect us in this case, but I defer to
those maintainers.

A starter list of daemons that need updating is:
All daemons in dbus-sensors
peci-pcie
entity-manager
libpeci
phosphor-u-boot-env-manager
pfr-manager
smbios-mdr
s2600-misc
dbus-sensors


4. Change the defaults of sdbusplus::asio::object_server to not init an
ObjectManager interface at the root path /.

5. Unwind the various complexities of searching for ObjectManager
interfaces, and rely on the assumptions above that should net us some
speedups in at least the sensors API, completely avoiding an extra mapper
call.

6. Get system owners to retest their platforms, with the tree changes in,
and fix any bugs we may have accidentally caused.

This was discussed pretty heavily on discord in the #development topic, and
some of the nitty gritty details on why this is needed is available there,
as well as I'm happy to discuss here.  This is one of those nasty
spaghetti-code things that we've lived with for a while;  Hopefully if we
can get this behind us, we can get some good real world performance
improvements.

Thoughts?

-Ed

[-- Attachment #2: Type: text/html, Size: 3906 bytes --]

Re: DBus ObjectManager Interface usages within the project

[Nan Zhou]

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

Thanks Ed for bringing this up and writing a great summary.

I can provide some background information about how this was brought up and
how requiring ObjectManager (OM) can improve performance.

On Google hardware with more than 200 sensors, we found that fetching a
single sensor in the Redfish route is extremely slow (~0.6 seconds). On
the other hand ipmi sdr list gives a fair performance (returns all sensors
in seconds). We then root caused it and found that the BMCWeb sensor code
is inefficient. We introduced the query parameter into BMCWeb, and
submitted https://gerrit.openbmc.org/c/openbmc/bmcweb/+/52418 which
integrates OM's GetManagedObjects
<https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager>
with Redfish Expand
<https://redfish.dmtf.org/schemas/DSP0266_1.15.0.html#the-expand-query-parameter>
query parameters. With it, we got 40+ times speed up when enumerating 200+
sensors.

```
Before this change,
time wget -qO-
'http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)'
> /tmp/log_slow.json

real 0m33.786s
user 0m0.000s
sys 0m0.000s

After this change
time wget -qO-
'http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)'
> /tmp/log_fast.json

real 0m0.769s
user 0m0.010s
sys 0m0.010s
```

We found the same pattern (use OM for bulk query) can be applied to most
inventory resources (Processors, DIMMS, etc), where similar performance
improvements can be achieved.

Ed brought up this issue that OM is optional as of today in inventories and
sensors during the code review of
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53824. The path of the OM is
also inconsistent.

Please let us know what you think.

Best
Nan

On Tue, Jul 12, 2022 at 11:48 AM Ed Tanous <edtanous@google.com> wrote:

> We've had a couple cases in the project where patches have needed to be
> slowed down because of inconsistencies in our use of object manager, so
> IMO, the time has come to make our usage of ObjectManager consistent,
> documented, and drop support for the (not upstream) daemons that don't
> follow the rules.  As part of this, we will port forward all the upstream
> daemons to be consistent, and do our best to avoid bugs, but this email is
> intended to inform those that have non-upstream daemons about the change so
> they can resolve their implementations.
>
> The basics:
> ObjectManager DBus interface will now be required for any daemon
> implementing a Sensor.Value interface or Inventory.Item interface.
>
> Daemons producing sensors will be required to place their ObjectManager at
> /xyz/openbmc_project/sensors
> Daemons producing inventory items will be required to place their
> ObjectManager at /xyz/openbmc_project/inventory.
>
> Both of these interfaces will be required to be published before claiming
> a well known name on dbus, to allow for the possibility of caching
> implementations at some point in future.
>
> Functionally, this gets a little complicated because the
> sdbusplus::asio::object_manager bindings in their default invocation create
> an ObjectManager at the root object /, so we need to execute this in a few
> steps.
>
> 0. Send this email out, outlining the problem, and warn the community that
> if this is an assumption you make in your downstream daemons, those
> assumptions will need to change.  Get consensus from maintainers.
> 1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value
> and Inventory.Item to call out the required-ness of ObjectManager, and the
> explicit dbus path where it's required to be placed.
> 2. Identify all the asio daemons that need changes, and publish changes
> that move the object_manager to the appropriate path.  This is mostly going
> to be a tree-wide grep for sdbusplus::asio::object_server, and look for
> daemonst that don't make use of the add_manager() API.  Anyone directly
> calling the Sensor or Invertory ObjectManager interfaces will need to port
> to the new paths, but luckily, this isn't a very common operation, and I
> beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In
> bmcweb, there is actually a convoluted piece of code that uses the mapper
> to sort out the location that the ObjectManager exists at so Redfish
> sensors should remain consistent, and we don't yet have code that relies on
> ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a
> similar piece of code that should protect us in this case, but I defer to
> those maintainers.
>
> A starter list of daemons that need updating is:
> All daemons in dbus-sensors
> peci-pcie
> entity-manager
> libpeci
> phosphor-u-boot-env-manager
> pfr-manager
> smbios-mdr
> s2600-misc
> dbus-sensors
>
>
> 4. Change the defaults of sdbusplus::asio::object_server to not init an
> ObjectManager interface at the root path /.
>
> 5. Unwind the various complexities of searching for ObjectManager
> interfaces, and rely on the assumptions above that should net us some
> speedups in at least the sensors API, completely avoiding an extra mapper
> call.
>
> 6. Get system owners to retest their platforms, with the tree changes in,
> and fix any bugs we may have accidentally caused.
>
> This was discussed pretty heavily on discord in the #development topic,
> and some of the nitty gritty details on why this is needed is available
> there, as well as I'm happy to discuss here.  This is one of those nasty
> spaghetti-code things that we've lived with for a while;  Hopefully if we
> can get this behind us, we can get some good real world performance
> improvements.
>
> Thoughts?
>
> -Ed
>

[-- Attachment #2: Type: text/html, Size: 6574 bytes --]

Re: DBus ObjectManager Interface usages within the project

[Patrick Williams]

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

On Tue, Jul 12, 2022 at 01:02:37PM -0700, Nan Zhou wrote:
> Ed brought up this issue that OM is optional as of today in inventories and
> sensors during the code review of
> https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53824. The path of the OM is
> also inconsistent.

I don't think "OM is optional" is really a true statement.  It is
optional from a dbus perspective, true, but practically speaking all of
our daemons have it (just at an inconsistent location).  One of the
primary features of the object manager is to emit the signals for
managed objects, such as PropertiesChanged.  If you don't have an object
manager you don't get those signals.  We have to have that on Sensors,
at least, and I'm pretty sure everyone adds them for Inventory as well.

As you're seeing, there are typically two behaviors of implementation:

1. Anything using ASIO places OM at the root by default.
2. Anything using PDI-bindings places the OM at the lowest common part
   in their hierarchy.  (Something like /xyz/openbmc_project/sensors or
   /xyz/openbmc_project typically).

-- 
Patrick Williams

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

Re: DBus ObjectManager Interface usages within the project

[Lei Yu]

On Wed, Jul 13, 2022 at 2:49 AM Ed Tanous <edtanous@google.com> wrote:
>
> We've had a couple cases in the project where patches have needed to be slowed down because of inconsistencies in our use of object manager, so IMO, the time has come to make our usage of ObjectManager consistent, documented, and drop support for the (not upstream) daemons that don't follow the rules.  As part of this, we will port forward all the upstream daemons to be consistent, and do our best to avoid bugs, but this email is intended to inform those that have non-upstream daemons about the change so they can resolve their implementations.
>
> The basics:
> ObjectManager DBus interface will now be required for any daemon implementing a Sensor.Value interface or Inventory.Item interface.
>
> Daemons producing sensors will be required to place their ObjectManager at /xyz/openbmc_project/sensors
> Daemons producing inventory items will be required to place their ObjectManager at /xyz/openbmc_project/inventory.
>
> Both of these interfaces will be required to be published before claiming a well known name on dbus, to allow for the possibility of caching implementations at some point in future.
>
> Functionally, this gets a little complicated because the sdbusplus::asio::object_manager bindings in their default invocation create an ObjectManager at the root object /, so we need to execute this in a few steps.
>
> 0. Send this email out, outlining the problem, and warn the community that if this is an assumption you make in your downstream daemons, those assumptions will need to change.  Get consensus from maintainers.
> 1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value and Inventory.Item to call out the required-ness of ObjectManager, and the explicit dbus path where it's required to be placed.
> 2. Identify all the asio daemons that need changes, and publish changes that move the object_manager to the appropriate path.  This is mostly going to be a tree-wide grep for sdbusplus::asio::object_server, and look for daemonst that don't make use of the add_manager() API.  Anyone directly calling the Sensor or Invertory ObjectManager interfaces will need to port to the new paths, but luckily, this isn't a very common operation, and I beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In bmcweb, there is actually a convoluted piece of code that uses the mapper to sort out the location that the ObjectManager exists at so Redfish sensors should remain consistent, and we don't yet have code that relies on ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a similar piece of code that should protect us in this case, but I defer to those maintainers.

Is it better for the asio API to provide an extra constructor to take
a object_path?

E.g.
 object_server(const std::shared_ptr<sdbusplus::asio::connection>&
conn, const std::string& path)

Then the users could initialize its object_server at expected path directly:

 sdbusplus::asio::object_server objectServer(bus, "/my/path");

instead of two lines of code like:

 sdbusplus::asio::object_server objectServer(bus, true);
 objectServer.add_manager("/my/path");

-- 
BRs,
Lei YU

Re: DBus ObjectManager Interface usages within the project

[Ed Tanous]

On Tue, Jul 12, 2022 at 8:07 PM Lei Yu <yulei.sh@bytedance.com> wrote:
>
> On Wed, Jul 13, 2022 at 2:49 AM Ed Tanous <edtanous@google.com> wrote:
> >
> > We've had a couple cases in the project where patches have needed to be slowed down because of inconsistencies in our use of object manager, so IMO, the time has come to make our usage of ObjectManager consistent, documented, and drop support for the (not upstream) daemons that don't follow the rules.  As part of this, we will port forward all the upstream daemons to be consistent, and do our best to avoid bugs, but this email is intended to inform those that have non-upstream daemons about the change so they can resolve their implementations.
> >
> > The basics:
> > ObjectManager DBus interface will now be required for any daemon implementing a Sensor.Value interface or Inventory.Item interface.
> >
> > Daemons producing sensors will be required to place their ObjectManager at /xyz/openbmc_project/sensors
> > Daemons producing inventory items will be required to place their ObjectManager at /xyz/openbmc_project/inventory.
> >
> > Both of these interfaces will be required to be published before claiming a well known name on dbus, to allow for the possibility of caching implementations at some point in future.
> >
> > Functionally, this gets a little complicated because the sdbusplus::asio::object_manager bindings in their default invocation create an ObjectManager at the root object /, so we need to execute this in a few steps.
> >
> > 0. Send this email out, outlining the problem, and warn the community that if this is an assumption you make in your downstream daemons, those assumptions will need to change.  Get consensus from maintainers.
> > 1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value and Inventory.Item to call out the required-ness of ObjectManager, and the explicit dbus path where it's required to be placed.
> > 2. Identify all the asio daemons that need changes, and publish changes that move the object_manager to the appropriate path.  This is mostly going to be a tree-wide grep for sdbusplus::asio::object_server, and look for daemonst that don't make use of the add_manager() API.  Anyone directly calling the Sensor or Invertory ObjectManager interfaces will need to port to the new paths, but luckily, this isn't a very common operation, and I beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In bmcweb, there is actually a convoluted piece of code that uses the mapper to sort out the location that the ObjectManager exists at so Redfish sensors should remain consistent, and we don't yet have code that relies on ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a similar piece of code that should protect us in this case, but I defer to those maintainers.
>
> Is it better for the asio API to provide an extra constructor to take
> a object_path?
>
> E.g.
>  object_server(const std::shared_ptr<sdbusplus::asio::connection>&
> conn, const std::string& path)
>
> Then the users could initialize its object_server at expected path directly:
>
>  sdbusplus::asio::object_server objectServer(bus, "/my/path");
>
> instead of two lines of code like:
>
>  sdbusplus::asio::object_server objectServer(bus, true);
>  objectServer.add_manager("/my/path");

I like the thinking, but considering that ObjectPath is not required
in all cases, and there might be cases where they have multiple (if a
daemon supported sensors and inventory) I don't think we can have it
in the constructor, or at the very least we'd have to support the
non-path constructor as well, which basically amounts to the same as
we have (it would just call add_manager internally).  I don't think
the one line of code savings is worth it in that case.

>
> --
> BRs,
> Lei YU

dbus prefixes (was: DBus ObjectManager Interface usages within the project)

[Alexander Amelkin]

As a side note to this discussion, have we considered using shorter and 
more convenient paths for dbus?

I don't see any real reason for the project to be using this long and 
cumbersome /xyz/openbmc_project/ prefix.
Why not use just '/obmc/' or `/phosphor/` ? I believe that would be more 
than enough to separate all our services from any third-party ones on d-bus.

I understand why this prefixing is for in 'big' open desktop or server 
systems where there on d-bus can be any number of software from any 
number of vendors. In an embedded system, such as OpenBMC, on the 
contrary, the set of software using d-bus is strictly limited and we 
always know beforehand what prefixes are used. I'm pretty sure none of 
the software included into OpenBMC builds will ever use '/obmc' prefix. 
So why continue using the inconvenient long prefixes when it is safe to 
use short ones? I would even propose dropping all prefixes at all, but 
ok, let's pretend that there can be some other 3rd-party 'Inventory' 
than '/obmc/' ('/xyz/openbmc_project/').

Am I wrong or missing anything? What's stopping us from switching to a 
shorter prefix, aside from the existing code that will need to be 
changed to it?

The same proposal/question actually applies to service names (e.g. 
xyz.openbmc_project.ObjectMapper could easily become just 
obmc.ObjectMapper or phosphor.ObjectMapper), let alone just 'ObjectMapper'.

WBR, Alexander.

12.07.2022 21:48, Ed Tanous пишет:
> We've had a couple cases in the project where patches have needed to 
> be slowed down because of inconsistencies in our use of object 
> manager, so IMO, the time has come to make our usage of ObjectManager 
> consistent, documented, and drop support for the (not upstream) 
> daemons that don't follow the rules.  As part of this, we will port 
> forward all the upstream daemons to be consistent, and do our best to 
> avoid bugs, but this email is intended to inform those that have 
> non-upstream daemons about the change so they can resolve their 
> implementations.
>
> The basics:
> ObjectManager DBus interface will now be required for any daemon 
> implementing a Sensor.Value interface or Inventory.Item interface.
>
> Daemons producing sensors will be required to place their 
> ObjectManager at /xyz/openbmc_project/sensors
> Daemons producing inventory items will be required to place their 
> ObjectManager at /xyz/openbmc_project/inventory.
>

Re: dbus prefixes (was: DBus ObjectManager Interface usages within the project)

[Patrick Williams]

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

On Wed, Jul 13, 2022 at 01:17:10PM +0300, Alexander Amelkin wrote:
> As a side note to this discussion, have we considered using shorter and 
> more convenient paths for dbus?
> 
> I don't see any real reason for the project to be using this long and 
> cumbersome /xyz/openbmc_project/ prefix.
> Why not use just '/obmc/' or `/phosphor/` ? I believe that would be more 
> than enough to separate all our services from any third-party ones on d-bus.

FWIW, it isn't required but it is encouraged to use a reverse-domain prefix on
paths by the dbus spec itself.

>> Object paths are often namespaced by starting with a reversed domain
>> name...

> I understand why this prefixing is for in 'big' open desktop or server 
> systems where there on d-bus can be any number of software from any 
> number of vendors. In an embedded system, such as OpenBMC, on the 
> contrary, the set of software using d-bus is strictly limited and we 
> always know beforehand what prefixes are used. I'm pretty sure none of 
> the software included into OpenBMC builds will ever use '/obmc' prefix. 
> So why continue using the inconvenient long prefixes when it is safe to 
> use short ones? I would even propose dropping all prefixes at all, but 
> ok, let's pretend that there can be some other 3rd-party 'Inventory' 
> than '/obmc/' ('/xyz/openbmc_project/').

Generally speaking, isn't the content in a single process pretty well
known no matter the type of system the daemon is installed on?  I think
it is pretty rare for a shared library to create its own dbus objects
and if it did it'd probably create its own bus-connection too.  I'm not
sure what is unique about a desktop vs embedded system in this
discussion.

What are we saving by switching to something shorter?  Can you
elaborate?  It seems like it is mostly just typing...

> 
> Am I wrong or missing anything? What's stopping us from switching to a 
> shorter prefix, aside from the existing code that will need to be 
> changed to it?
> 
> The same proposal/question actually applies to service names (e.g. 
> xyz.openbmc_project.ObjectMapper could easily become just 
> obmc.ObjectMapper or phosphor.ObjectMapper), let alone just 'ObjectMapper'.

Service names and interface names have a strong statement w.r.t
reverse-domain prefixes.

>> Interface names should start with the reversed DNS domain name of the
>> author of the interface (in lower-case), like interface names in
>> Java.

>> Like interface names, well-known bus names should start with the
>> reversed DNS domain name of the author of the interface (in
>> lower-case)...

Assuming the typical use of "should" it means "this should be done
unless you have a good reason not to".

-- 
Patrick Williams

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

Re: dbus prefixes (was: DBus ObjectManager Interface usages within the project)

[Alexander Amelkin]

13.07.2022 14:39, Patrick Williams пишет:
> On Wed, Jul 13, 2022 at 01:17:10PM +0300, Alexander Amelkin wrote:
>> I don't see any real reason for the project to be using this long and
>> cumbersome /xyz/openbmc_project/ prefix.
>> Why not use just '/obmc/' or `/phosphor/` ? I believe that would be more
>> than enough to separate all our services from any third-party ones on d-bus.
> FWIW, it isn't required but it is encouraged to use a reverse-domain prefix on
> paths by the dbus spec itself.

Yes, I'm aware of that. But do we actually need to follow that 
recommendation? What for?
I believe, it exists and is encouraged for big open systems to avoid 
conflicts between different pieces of software using d-bus simultaneously.

As I said, OpenBMC is a closed system where the set of software is well 
controlled. We know beforehand what software uses which prefix. I find 
this just inconvenient to use the long prefixes in these circumstances.

>
>>> Object paths are often namespaced by starting with a reversed domain
>>> name...
>> I understand why this prefixing is for in 'big' open desktop or server
>> systems where there on d-bus can be any number of software from any
>> number of vendors. In an embedded system, such as OpenBMC, on the
>> contrary, the set of software using d-bus is strictly limited and we
>> always know beforehand what prefixes are used. I'm pretty sure none of
>> the software included into OpenBMC builds will ever use '/obmc' prefix.
>> So why continue using the inconvenient long prefixes when it is safe to
>> use short ones? I would even propose dropping all prefixes at all, but
>> ok, let's pretend that there can be some other 3rd-party 'Inventory'
>> than '/obmc/' ('/xyz/openbmc_project/').
> Generally speaking, isn't the content in a single process pretty well
> known no matter the type of system the daemon is installed on?  I think
> it is pretty rare for a shared library to create its own dbus objects
> and if it did it'd probably create its own bus-connection too.  I'm not
> sure what is unique about a desktop vs embedded system in this
> discussion.

It's rather that embedded system is unique compared to desktop.

Desktops, if not enforced to use reverse domain prefixes, may end up 
using the same name for a d-bus service if two different software 
authors decide to use the same word. The probability of such a 
coincidence in a desktop is quite high. That's why d-bus spec encourages 
use of reverse domain notation (relying on uniqueness of domain names 
and their binding through registration to physical bodies or legal 
entities).

In embedded systems the probability of that coincidence is negligible on 
the other hand.

> What are we saving by switching to something shorter?  Can you
> elaborate?  It seems like it is mostly just typing...

I guess you're right that it's mostly just typing that we save. That's 
actually a lot when we're talking debugging and running busctl 
repeatedly with changing parameters from a command line.

It may be also that we are achieving some better code readability by 
dropping the identical long prefixes and only leaving what's essential.

At least for me that is important.

>
>> Am I wrong or missing anything? What's stopping us from switching to a
>> shorter prefix, aside from the existing code that will need to be
>> changed to it?
>>
>> The same proposal/question actually applies to service names (e.g.
>> xyz.openbmc_project.ObjectMapper could easily become just
>> obmc.ObjectMapper or phosphor.ObjectMapper), let alone just 'ObjectMapper'.
> Service names and interface names have a strong statement w.r.t
> reverse-domain prefixes.
>>> Interface names should start with the reversed DNS domain name of the
>>> author of the interface (in lower-case), like interface names in
>>> Java.
>>> Like interface names, well-known bus names should start with the
>>> reversed DNS domain name of the author of the interface (in
>>> lower-case)...
> Assuming the typical use of "should" it means "this should be done
> unless you have a good reason not to".

Again, why (for what purpose) do we have to obey those statements?
What exactly are we achieving by obeying them? Do we need to achieve that?

We're creating a closed system that does not interact with any external 
software using d-bus.

IMO, we're completely free to choose the notation of service and interface
names as we see fit. We won't be introducing any disturbance into the
outside world by not using the general purpose notation. No one would 
actually
even notice us doing that.

WBR, Alexander.

Re: dbus prefixes (was: DBus ObjectManager Interface usages within the project)

[Brad Bishop]

On Wed, Jul 27, 2022 at 02:15:34PM +0300, Alexander Amelkin wrote:
>
>Again, why (for what purpose) do we have to obey those statements?
>What exactly are we achieving by obeying them? Do we need to achieve that?
>
>We're creating a closed system that does not interact with any 
>external software using d-bus.

I agree.  The spec also recommends that we version our service names and 
interfaces, but we don't do that either.

Further, outside of the OpenBMC community, DBus programming conventions 
are such that the service name defines the interface schema for a given 
service.  We don't adhere to that convention either.

The way I look at it is, our use of DBus is mostly an implementation 
detail of our tightly coupled system.  We don't use it as an interface 
to the outside world.  All of these conventions being discussed exist to 
aid with the latter.

Thanks,
Brad

Re: DBus ObjectManager Interface usages within the project

[Patrick Williams]

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

On Tue, Jul 12, 2022 at 11:48:31AM -0700, Ed Tanous wrote:
> We've had a couple cases in the project where patches have needed to be
> slowed down because of inconsistencies in our use of object manager, so
> IMO, the time has come to make our usage of ObjectManager consistent,
> documented, and drop support for the (not upstream) daemons that don't
> follow the rules.  As part of this, we will port forward all the upstream
> daemons to be consistent, and do our best to avoid bugs, but this email is
> intended to inform those that have non-upstream daemons about the change so
> they can resolve their implementations.

There isn't much in the dbus spec that puts requirements on where the
object manager is, but that certainly doesn't mean we can't add our own
design requirements on top of it.  Thanks for starting this.

> The basics:
> ObjectManager DBus interface will now be required for any daemon
> implementing a Sensor.Value interface or Inventory.Item interface.
> 
> Daemons producing sensors will be required to place their ObjectManager at
> /xyz/openbmc_project/sensors
> Daemons producing inventory items will be required to place their
> ObjectManager at /xyz/openbmc_project/inventory.
> 
> Both of these interfaces will be required to be published before claiming a
> well known name on dbus, to allow for the possibility of caching
> implementations at some point in future.

This means we can end up having N object managers in a single daemon if
it is hosting multiple namespaces?  Why not just host it at
/xyz/openbmc_project?

> This was discussed pretty heavily on discord in the #development topic, and
> some of the nitty gritty details on why this is needed is available there,
> as well as I'm happy to discuss here.  This is one of those nasty
> spaghetti-code things that we've lived with for a while;  Hopefully if we
> can get this behind us, we can get some good real world performance
> improvements.

I see the background being discussed when I read through the history on
#development, but I don't see the rationale on why this was chosen.  I see one
comment that placing the OM at / is "wrong" but I don't see any justification.
Why is ".../sensors" right but "/" or "/xyz/openbmc_project" is not?

We had a good chunk of this discussion about 6 months back in
phosphor-virtual-sensors where some Redfish code was broken against that
daemon because it _was_ using "/xyz/openbmc_project/sensors" and there
was a patch to move it to "/" which ended up getting merged.
Fundamentally, I think it boiled down to neither being in opposition to
the standard and there was a bunch of code that already implied "/" so
it was the simplest way forward to achieve compatibility.

> 
> Thoughts?

I do think that moving the OM lower in the hierarchy is probably better
because it allows us to have parts of the hierarchy which do not emit
signals, where having it on "/" does not.  I'm just trying to understand
(and hopefully document more) the rationale on why this choice was made.

-- 
Patrick Williams

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

Re: DBus ObjectManager Interface usages within the project

[Ed Tanous]

On Wed, Jul 13, 2022 at 4:24 AM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Tue, Jul 12, 2022 at 11:48:31AM -0700, Ed Tanous wrote:
> > We've had a couple cases in the project where patches have needed to be
> > slowed down because of inconsistencies in our use of object manager, so
> > IMO, the time has come to make our usage of ObjectManager consistent,
> > documented, and drop support for the (not upstream) daemons that don't
> > follow the rules.  As part of this, we will port forward all the upstream
> > daemons to be consistent, and do our best to avoid bugs, but this email is
> > intended to inform those that have non-upstream daemons about the change so
> > they can resolve their implementations.
>
> There isn't much in the dbus spec that puts requirements on where the
> object manager is, but that certainly doesn't mean we can't add our own
> design requirements on top of it.  Thanks for starting this.
>
> > The basics:
> > ObjectManager DBus interface will now be required for any daemon
> > implementing a Sensor.Value interface or Inventory.Item interface.
> >
> > Daemons producing sensors will be required to place their ObjectManager at
> > /xyz/openbmc_project/sensors
> > Daemons producing inventory items will be required to place their
> > ObjectManager at /xyz/openbmc_project/inventory.
> >
> > Both of these interfaces will be required to be published before claiming a
> > well known name on dbus, to allow for the possibility of caching
> > implementations at some point in future.
>
> This means we can end up having N object managers in a single daemon if
> it is hosting multiple namespaces?  Why not just host it at
> /xyz/openbmc_project?

Because rarely is the question being asked "What are all the openbmc
interfaces and values for those interfaces that this application
supports".  Usually the question is more specific, like "What sensors
does this application support".  If we had to get all the inventory
items, nics, and other stuff at the same time that we got all the
sensors, it would make the responses larger.

>
> > This was discussed pretty heavily on discord in the #development topic, and
> > some of the nitty gritty details on why this is needed is available there,
> > as well as I'm happy to discuss here.  This is one of those nasty
> > spaghetti-code things that we've lived with for a while;  Hopefully if we
> > can get this behind us, we can get some good real world performance
> > improvements.
>
> I see the background being discussed when I read through the history on
> #development, but I don't see the rationale on why this was chosen.  I see one
> comment that placing the OM at / is "wrong" but I don't see any justification.
> Why is ".../sensors" right but "/" or "/xyz/openbmc_project" is not?

GetManagedObjects doesn't have a way to filter down further, and for
daemons that might return both, it's advantageous to be able to filter
down more and not return every interface in one response.

>
> We had a good chunk of this discussion about 6 months back in
> phosphor-virtual-sensors where some Redfish code was broken against that
> daemon because it _was_ using "/xyz/openbmc_project/sensors" and there
> was a patch to move it to "/" which ended up getting merged.

Yep, this is essentially the continuation of that discussion.

> Fundamentally, I think it boiled down to neither being in opposition to
> the standard and there was a bunch of code that already implied "/" so
> it was the simplest way forward to achieve compatibility.
>
> >
> > Thoughts?
>
> I do think that moving the OM lower in the hierarchy is probably better
> because it allows us to have parts of the hierarchy which do not emit
> signals, where having it on "/" does not.  I'm just trying to understand
> (and hopefully document more) the rationale on why this choice was made.

Sounds good.

>
> --
> Patrick Williams

Re: DBus ObjectManager Interface usages within the project

[Nan Zhou]

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

>
> I don't think "OM is optional" is really a true statement.  It is
> optional from a dbus perspective, true, but practically speaking all of
> our daemons have it (just at an inconsistent location).  One of the
> primary features of the object manager is to emit the signals for
> managed objects, such as PropertiesChanged.  If you don't have an object
> manager you don't get those signals.  We have to have that on Sensors,
> at least, and I'm pretty sure everyone adds them for Inventory as well.


As you're seeing, there are typically two behaviors of implementation:


1. Anything using ASIO places OM at the root by default.
> 2. Anything using PDI-bindings places the OM at the lowest common part
>    in their hierarchy.  (Something like /xyz/openbmc_project/sensors or
>    /xyz/openbmc_project typically).


Is this documented somewhere formally so that when people ask or question
about it, we can have a pointer.

If not, is PDI a good place to place this requirement?

On Wed, Jul 13, 2022 at 7:36 AM Ed Tanous <edtanous@google.com> wrote:

> On Wed, Jul 13, 2022 at 4:24 AM Patrick Williams <patrick@stwcx.xyz>
> wrote:
> >
> > On Tue, Jul 12, 2022 at 11:48:31AM -0700, Ed Tanous wrote:
> > > We've had a couple cases in the project where patches have needed to be
> > > slowed down because of inconsistencies in our use of object manager, so
> > > IMO, the time has come to make our usage of ObjectManager consistent,
> > > documented, and drop support for the (not upstream) daemons that don't
> > > follow the rules.  As part of this, we will port forward all the
> upstream
> > > daemons to be consistent, and do our best to avoid bugs, but this
> email is
> > > intended to inform those that have non-upstream daemons about the
> change so
> > > they can resolve their implementations.
> >
> > There isn't much in the dbus spec that puts requirements on where the
> > object manager is, but that certainly doesn't mean we can't add our own
> > design requirements on top of it.  Thanks for starting this.
> >
> > > The basics:
> > > ObjectManager DBus interface will now be required for any daemon
> > > implementing a Sensor.Value interface or Inventory.Item interface.
> > >
> > > Daemons producing sensors will be required to place their
> ObjectManager at
> > > /xyz/openbmc_project/sensors
> > > Daemons producing inventory items will be required to place their
> > > ObjectManager at /xyz/openbmc_project/inventory.
> > >
> > > Both of these interfaces will be required to be published before
> claiming a
> > > well known name on dbus, to allow for the possibility of caching
> > > implementations at some point in future.
> >
> > This means we can end up having N object managers in a single daemon if
> > it is hosting multiple namespaces?  Why not just host it at
> > /xyz/openbmc_project?
>
> Because rarely is the question being asked "What are all the openbmc
> interfaces and values for those interfaces that this application
> supports".  Usually the question is more specific, like "What sensors
> does this application support".  If we had to get all the inventory
> items, nics, and other stuff at the same time that we got all the
> sensors, it would make the responses larger.
>
> >
> > > This was discussed pretty heavily on discord in the #development
> topic, and
> > > some of the nitty gritty details on why this is needed is available
> there,
> > > as well as I'm happy to discuss here.  This is one of those nasty
> > > spaghetti-code things that we've lived with for a while;  Hopefully if
> we
> > > can get this behind us, we can get some good real world performance
> > > improvements.
> >
> > I see the background being discussed when I read through the history on
> > #development, but I don't see the rationale on why this was chosen.  I
> see one
> > comment that placing the OM at / is "wrong" but I don't see any
> justification.
> > Why is ".../sensors" right but "/" or "/xyz/openbmc_project" is not?
>
> GetManagedObjects doesn't have a way to filter down further, and for
> daemons that might return both, it's advantageous to be able to filter
> down more and not return every interface in one response.
>
> >
> > We had a good chunk of this discussion about 6 months back in
> > phosphor-virtual-sensors where some Redfish code was broken against that
> > daemon because it _was_ using "/xyz/openbmc_project/sensors" and there
> > was a patch to move it to "/" which ended up getting merged.
>
> Yep, this is essentially the continuation of that discussion.
>
> > Fundamentally, I think it boiled down to neither being in opposition to
> > the standard and there was a bunch of code that already implied "/" so
> > it was the simplest way forward to achieve compatibility.
> >
> > >
> > > Thoughts?
> >
> > I do think that moving the OM lower in the hierarchy is probably better
> > because it allows us to have parts of the hierarchy which do not emit
> > signals, where having it on "/" does not.  I'm just trying to understand
> > (and hopefully document more) the rationale on why this choice was made.
>
> Sounds good.
>
> >
> > --
> > Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 6620 bytes --]

Re: DBus ObjectManager Interface usages within the project

[Nan Zhou]

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

I searched all the DIMM daemons in the org.
https://github.com/openbmc/openpower-vpd-parser/search?q=DIMM

Seems to me smbios-sdr is the only public repo. Are there any other repos
that implement the DIMM interface?

On Wed, Jul 13, 2022 at 5:49 PM Nan Zhou <nanzhou@google.com> wrote:

> I don't think "OM is optional" is really a true statement.  It is
>> optional from a dbus perspective, true, but practically speaking all of
>> our daemons have it (just at an inconsistent location).  One of the
>> primary features of the object manager is to emit the signals for
>> managed objects, such as PropertiesChanged.  If you don't have an object
>> manager you don't get those signals.  We have to have that on Sensors,
>> at least, and I'm pretty sure everyone adds them for Inventory as well.
>
>
> As you're seeing, there are typically two behaviors of implementation:
>
>
> 1. Anything using ASIO places OM at the root by default.
>> 2. Anything using PDI-bindings places the OM at the lowest common part
>>    in their hierarchy.  (Something like /xyz/openbmc_project/sensors or
>>    /xyz/openbmc_project typically).
>
>
> Is this documented somewhere formally so that when people ask or question
> about it, we can have a pointer.
>
> If not, is PDI a good place to place this requirement?
>
> On Wed, Jul 13, 2022 at 7:36 AM Ed Tanous <edtanous@google.com> wrote:
>
>> On Wed, Jul 13, 2022 at 4:24 AM Patrick Williams <patrick@stwcx.xyz>
>> wrote:
>> >
>> > On Tue, Jul 12, 2022 at 11:48:31AM -0700, Ed Tanous wrote:
>> > > We've had a couple cases in the project where patches have needed to
>> be
>> > > slowed down because of inconsistencies in our use of object manager,
>> so
>> > > IMO, the time has come to make our usage of ObjectManager consistent,
>> > > documented, and drop support for the (not upstream) daemons that don't
>> > > follow the rules.  As part of this, we will port forward all the
>> upstream
>> > > daemons to be consistent, and do our best to avoid bugs, but this
>> email is
>> > > intended to inform those that have non-upstream daemons about the
>> change so
>> > > they can resolve their implementations.
>> >
>> > There isn't much in the dbus spec that puts requirements on where the
>> > object manager is, but that certainly doesn't mean we can't add our own
>> > design requirements on top of it.  Thanks for starting this.
>> >
>> > > The basics:
>> > > ObjectManager DBus interface will now be required for any daemon
>> > > implementing a Sensor.Value interface or Inventory.Item interface.
>> > >
>> > > Daemons producing sensors will be required to place their
>> ObjectManager at
>> > > /xyz/openbmc_project/sensors
>> > > Daemons producing inventory items will be required to place their
>> > > ObjectManager at /xyz/openbmc_project/inventory.
>> > >
>> > > Both of these interfaces will be required to be published before
>> claiming a
>> > > well known name on dbus, to allow for the possibility of caching
>> > > implementations at some point in future.
>> >
>> > This means we can end up having N object managers in a single daemon if
>> > it is hosting multiple namespaces?  Why not just host it at
>> > /xyz/openbmc_project?
>>
>> Because rarely is the question being asked "What are all the openbmc
>> interfaces and values for those interfaces that this application
>> supports".  Usually the question is more specific, like "What sensors
>> does this application support".  If we had to get all the inventory
>> items, nics, and other stuff at the same time that we got all the
>> sensors, it would make the responses larger.
>>
>> >
>> > > This was discussed pretty heavily on discord in the #development
>> topic, and
>> > > some of the nitty gritty details on why this is needed is available
>> there,
>> > > as well as I'm happy to discuss here.  This is one of those nasty
>> > > spaghetti-code things that we've lived with for a while;  Hopefully
>> if we
>> > > can get this behind us, we can get some good real world performance
>> > > improvements.
>> >
>> > I see the background being discussed when I read through the history on
>> > #development, but I don't see the rationale on why this was chosen.  I
>> see one
>> > comment that placing the OM at / is "wrong" but I don't see any
>> justification.
>> > Why is ".../sensors" right but "/" or "/xyz/openbmc_project" is not?
>>
>> GetManagedObjects doesn't have a way to filter down further, and for
>> daemons that might return both, it's advantageous to be able to filter
>> down more and not return every interface in one response.
>>
>> >
>> > We had a good chunk of this discussion about 6 months back in
>> > phosphor-virtual-sensors where some Redfish code was broken against that
>> > daemon because it _was_ using "/xyz/openbmc_project/sensors" and there
>> > was a patch to move it to "/" which ended up getting merged.
>>
>> Yep, this is essentially the continuation of that discussion.
>>
>> > Fundamentally, I think it boiled down to neither being in opposition to
>> > the standard and there was a bunch of code that already implied "/" so
>> > it was the simplest way forward to achieve compatibility.
>> >
>> > >
>> > > Thoughts?
>> >
>> > I do think that moving the OM lower in the hierarchy is probably better
>> > because it allows us to have parts of the hierarchy which do not emit
>> > signals, where having it on "/" does not.  I'm just trying to understand
>> > (and hopefully document more) the rationale on why this choice was made.
>>
>> Sounds good.
>>
>> >
>> > --
>> > Patrick Williams
>>
>

[-- Attachment #2: Type: text/html, Size: 7306 bytes --]

Re: DBus ObjectManager Interface usages within the project

[Brad Bishop]

> On Sep 12, 2022, at 12:54 PM, Nan Zhou <nanzhou@google.com> wrote:
> 
> I searched all the DIMM daemons in the org. https://github.com/openbmc/openpower-vpd-parser/search?q=DIMM

For what it is worth, this link is not searching the entire org, just the openpower-vpd-parser repository.

Re: DBus ObjectManager Interface usages within the project

[Nan Zhou]

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

Oh, yeah, wrong URL. I meant
https://github.com/search?l=C%2B%2B&q=org%3Aopenbmc+DIMM&type=Code.

Thanks for your correction.

On Tue, Sep 13, 2022 at 8:46 AM Brad Bishop <bradleyb@fuzziesquirrel.com>
wrote:

>
>
> > On Sep 12, 2022, at 12:54 PM, Nan Zhou <nanzhou@google.com> wrote:
> >
> > I searched all the DIMM daemons in the org.
> https://github.com/openbmc/openpower-vpd-parser/search?q=DIMM
>
> For what it is worth, this link is not searching the entire org, just the
> openpower-vpd-parser repository.
>

[-- Attachment #2: Type: text/html, Size: 1123 bytes --]