dbus interface redesign

[jk-ozlabs]

The PR #39 is precipitating a few other dbus interface changes that would need to happen at the same time. Just documenting some of my plans here.

The introduction of the links object directly in the /xyz/openbmc_project/mctp level breaks the existing convention that this object only hosts the tree of <net>/<eid> endpoint objects. Since we're doing that, we should probably only do this with a proper API revision, which then triggers some other changes. Those changes then come full-circle and may influence #39 too, so some discussion & planning here is definitely warranted.

The progression of those changes is approximately:

1. the bus name and object paths probably shouldn't reference the openbmc project namespace, as they're not OpenBMC specific. (OpenBMC does define the MCTP.Endpoint interface though, so that is suitable). So we should shift those to au.com.codeconstruct to avoid that. This is something that has been on the backburner for a while, so a revised API is the right time to introduce this change.
2. then, since we're defining new interfaces, we should version those, and apply the dbus standards for object & interface naming
3. which means that the top-level au.com.codeconstruct.MCTP interface is also up for redesign. Currently, it hosts the SetupEndpoint (and similar) methods. However, it's possible that those methods aren't appropriate for the a mctpd configuration, in the case where we're not a bus owner for anything (conditional on: would we still be running mctpd on that node? My assumption that there is still value in running some control protocol interactions there)
4. So, it's probably appropriate to rename au.com.codeconstruct.MCTP to au.com.codeconstruct.MCTP.BusOwner1, and only have it present on objects that are appropriate representations of a bus owner...
5. ... which would be the new links/<name> objects that we're introducing in #39 !

So, my proposal for a best-practices dbus layout would be:

/au/com/codeconstruct/mctp1

"entry point" object for the mctp tree. Currently hosts the top-level .MCTP interface, but that would be moved. So, no interfaces planned at present, but would be suitable for future top-level configuration

/au/com/codeconstruct/mctp1/networks/<net>

Top-level for each MCTP network. No interfaces at present.

/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid>

Endpoint object. Would host:

• xyz.openbmc_project.MCTP.Endpoint: as exists currently, for our generic endpoint properties: EID, Supported Message Types etc
• xyz.openbmc_project.Common.UUID: as exists currently, for UUIDs
• au.com.codeconstruct.MCTP.Endpoint1: as exists currently, for mctpd control of endpoints (Remove, Recover, etc)

/au/com/codeconstruct/mctp1/links/<name>

Link object, as proposed by #39 (but see my bikeshed suggestion below). Would host:

• au.com.codeconstruct.MCTP.Link1 (as proposed by Thu): generic link configuration, including Role property
• au.com.codeconstruct.MCTP.BusOwner1: our new interface that replaces au.com.codeconstruct.MCTP, for control-protocol bus-owner functions (SetupEndpoint etc). Only present if Role == BusOwner.

---

Some of my open design points:

• whether the BusOwner interface should be on the link object or somewhere else under the net
  • if it is on the link, we would drop the link name from the method arguments. That we already have that link name as an argument suggests that the link is the correct abstraction level for the dbus interface functionality.
• whether the link object location is suitable, or whether it should be under the networks/x object somehow.
  • I have a preference for the current proposal, otherwise links will be difficult to find
  • but each link is certainly specific one (and only one) net

And some options for bikeshed aficionados - there probably needs to be some tweaking of various names:

• Plural or singular collections? /mctp1/networks/<net-id>/endpoints/<eid> or /mctp1/network/x/endpoint/y ?
  • There is precedence for both, but plural seems a bit more common. I have a slight preference for singular, but common is probably better.
• /mctp1/links/x or /mctp/interfaces/x ?
  • "links" is a bit ambiguous, I prefer the latter

[ThuBaNguyen]

• Plural or singular collections? /mctp1/networks/<net-id>/endpoints/<eid> or /mctp1/network/x/endpoint/y ?

  • There is precedence for both, but plural seems a bit more common. I have a slight preference for singular, but common is probably better.

I prefer /mctp1/networks/<net-id>/endpoints/<eid>.

• /mctp1/links/x or /mctp/interfaces/x ?

  • "links" is a bit ambiguous, I prefer the latter

I used /mctp1/links/x in #39 because I saw that we are using mctp link to setup one interface. I think /mctp/interfaces/x is good too.

About 2. then, since we're defining new interfaces, we should version those, and apply the dbus standards for object & interface naming, Do we really need the version? I prefer to use au.com.codeconstruct.MCTP.Endpoint than au.com.codeconstruct.MCTP.Endpoint1. au.com.codeconstruct.MCTP.Endpoint is clearer.

[jk-ozlabs]

Hi @ThuBaNguyen ,

Thanks for taking a look. Some responses:

I prefer /mctp1/networks/<net-id>/endpoints/<eid>.

OK, cool. Let's take that approach then.

• "links" is a bit ambiguous, I prefer the latter

I used /mctp1/links/x in #39 because I saw that we are using mctp link to setup one interface. I think /mctp/interfaces/x is good too.

OK, I'll think about that one a bit more. The mctp link is for consistency with ip link.

Do we really need the version?

Yeah, it's necessary to prevent future API breakage (so we don't need a repeat of what we're doing now!).

It's fairly well-documented:

It is also a good idea to include the major version of the interface in the name, and increment it if incompatible changes are made

(https://dbus.freedesktop.org/doc/dbus-specification.html)

This is achieved by including a version number in each interface name, service name and object path which is incremented on every backwards-incompatible change.

(https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning)

and all of: http://0pointer.de/blog/projects/versioning-dbus.html

I prefer to use au.com.codeconstruct.MCTP.Endpoint than au.com.codeconstruct.MCTP.Endpoint1. au.com.codeconstruct.MCTP.Endpoint is clearer.

The names are certainly neater, but we do need the versioning for future compat.

[jk-ozlabs]

However, .Endpoint might be a special case here; it exists already, and we're not making any changes to the interface definition...

[jk-ozlabs]

although, we are standardising on the lower-case format for prefix component of the interfaces, so that probably warrants changing to stay uniform.

[jk-ozlabs]

Have just pushed a proposed (ie., draft) implementation up at https://github.com/CodeConstruct/mctp/compare/main...dev/dbus-rework.

This uses the configuration branch as a base, as I intend to merge that beforehand.

[jk-ozlabs]

The plan from there would be:

• merge #39
• move the BusOwner1.DRAFT interface to the new interface objects introduced by that merge (and hence changing that dbus interface, as we would no longer need the MCTP interface name as an argument)
• remove the .DRAFT suffix

[ThuBaNguyen]

The plan from there would be:

• merge mctpd: Add mctp/links/<linkName> D-Bus object #39
• move the BusOwner1.DRAFT interface to the new interface objects introduced by that merge (and hence changing that dbus interface, as we would no longer need the MCTP interface name as an argument)
• remove the .DRAFT suffix

I'm planing to update the mctp:

1. Remove au.com.codeconstruct.MCTP from /xyz/openbmc_project/mctp, move it to /au/com/codeconstruct/mctp1/links/ and rename that interface to to au.com.codeconstruct.MCTP.BusOwner1
2. Change /xyz/openbmc_project/mctp to /au/com/codeconstruct/mctp1
3. /au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid> will include xyz.openbmc_project.MCTP.Endpoint, xyz.openbmc_project.Common.UUID,au.com.codeconstruct.MCTP.Endpoint` as current implementation.
4. These change will be added to #39 as the first commit.

For #39, I will

1. Name the DBus object as /au/com/codeconstruct/mctp1/links/<name>
2. Rename au.com.codeconstruct.MCTP.Link to au.com.codeconstruct.MCTP.Link1
3. Add au.com.codeconstruct.MCTP.BusOwner1 to /au/com/codeconstruct/mctp1/links/<name>

Let me know your idea.

[jk-ozlabs]

Not sure I totally understand there, but:

I'm planing to update the mctp:

1. Remove au.com.codeconstruct.MCTP from /xyz/openbmc_project/mctp, move it to /au/com/codeconstruct/mctp1/links/ and rename that interface to to au.com.codeconstruct.MCTP.BusOwner1

The rename to BusOwner1 is already present in my branch. I'll handle moving that to the MCTP interface/x objects once your change adds those objects

2. Change /xyz/openbmc_project/mctp to /au/com/codeconstruct/mctp1

Also already done in my branch, but may need updating in your changes too

3. /au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid> will include xyz.openbmc_project.MCTP.Endpoint, xyz.openbmc_project.Common.UUID,au.com.codeconstruct.MCTP.Endpoint` as current implementation.

This is done too.

4. These change will be added to mctpd: Add mctp/links/<linkName> D-Bus object #39 as the first commit.

Just rebase on top of my proposed branch for these.

For #39, I will

1. Name the DBus object as /au/com/codeconstruct/mctp1/links/<name>

2. Rename au.com.codeconstruct.MCTP.Link to au.com.codeconstruct.MCTP.Link1

Sounds good!

3. Add au.com.codeconstruct.MCTP.BusOwner1 to /au/com/codeconstruct/mctp1/links/<name>

I'll handle this as a change (ie, moving the dbus interface implementation) after #39 is merged.

[ThuBaNguyen]

I don't know that you already update the mctp code as your proposal in branch dev/dbus-rework. I will update #39 base on this branch.

[jk-ozlabs]

Ah, and maybe interfaces rather than links. While both have some degree of namespace conflict, I think the "interfaces" term is more common for the actual network interface.

[ThuBaNguyen]

I don't know that you already update the mctp code as your proposal in branch dev/dbus-rework. I will update #39 base on this branch.

I'm in trouble with verifying branch dev/dbus-rework. Below is my step to apply dev/dbus-rework

As commit mctpd: attempt a default configuration from /etc/mctpd.conf, I tried to create mctpd.conf and override the /etc/mctpd.conf with my configuration file.

# Mode: either bus-owner or endpoint
mode = "bus-owner"

# MCTP protocol configuration. Used for both endpoint and bus-owner modes.
[mctp]
message_timeout_ms = 250

After flash the OpenBMC image, I tried to cat /etc/mctpd.conf to confirm about the correctness of step 1. I also tried to print ctx->mctp_timeout to confirm about the configuration is applied.

Check the journal log I saw below message

May 31 16:40:17 board systemd[1]: mctpd.service: start operation timed out. Terminating.
May 31 16:40:17 board systemd[1]: mctpd.service: Failed with result 'timeout'.
May 31 16:40:17 board systemd[1]: Failed to start MCTP control protocol daemon.

I don't change the mctpd.service

cat /lib/systemd/system/mctpd.service

[Unit]
Description=MCTP control protocol daemon
Wants=mctp-local.target
After=mctp-local.target

[Service]
Type=dbus
BusName=xyz.openbmc_project.MCTP
ExecStart=/usr/sbin/mctpd

[Install]
WantedBy=mctp.target

[ThuBaNguyen]

If I roll back to commit mctpd: attempt a default configuration from /etc/mctpd.conf(https://github.com/CodeConstruct/mctp/commit/947205f0b8e364c1b00abdbdf0966ef3cd21ba85)

There is no error regardless I don't add mctpd.conf.

[jk-ozlabs]

OK, sounds like I have a bug in the conf changes. I'll try with your config shortly.

[ThuBaNguyen]

OK, sounds like I have a bug in the conf changes. I'll try with your config shortly.

Any update @jk-ozlabs ?

[jk-ozlabs]

nope, it was the weekend! I'll check it out today.

[jk-ozlabs]

oh, looks like it isn't related to the config changes.

You have:

[Service] Type=dbus BusName=xyz.openbmc_project.MCTP ExecStart=/usr/sbin/mctpd

You'll need to update BusName to match the new bus owner name.

I assume this is coming from the example in conf/mctpd.service, so I'll update that in the dbus-rework branch.

[jk-ozlabs]

so I'll update that in the dbus-rework branch.

ok, done.

[ThuBaNguyen]

New code fix the issue. Thanks.