DBus ObjectManager Interface usages within the project

[Ed Tanous]

On Wed, Jul 13, 2022 at 4:24 AM Patrick Williams <patrick at 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