Proposal: Removing redundant EpochTime interface from dump entry

[Ed Tanous]

On Thu, Sep 21, 2023 at 8:44 AM dhruvaraj S <dhruvaraj at gmail.com> wrote:
>
> On Thu, 21 Sept 2023 at 21:09, Patrick Williams <patrick at stwcx.xyz> wrote:
> >
> > On Thu, Sep 21, 2023 at 08:52:15AM +0530, dhruvaraj S wrote:
> > > Hi,
> > >
> > > In the current implementation, objects implementing the dump entry
> > > interface implement both xyz.openbmc_project.Common.Progress (for dump
> > > creation start time, end time, and status) and
> > > xyz.openbmc_project.Time.Epoch (for dump creation time, which is
> > > effectively the end time). This leads to a redundancy in specifying
> > > the dump creation end time.

Don't the two interfaces describe different things?  Time.Epoch
represents when the event occurred, not when the recording of that
event was complete, Common.progress represents when the
logging/processing of that event was complete.  In a lot of on-bmc
scenarios, they're going to be similar if not the same, but in the
case of something like a remote processor flagging an error, they're
going to be different.  An error might not be completely processed
until minutes later.

FWIW, in terms of complexity reduction on DBus, I think there's a lot
more impactful places to start for a lot less effort, but if this is
something you really want to chase to conclusion, and it reduces the
code complexity (measured by a net-negative diff patchset to OpenBMC)
and you're willing to test all the scenarios, feel free to continue to
chase it down.

> > >
> > > My proposed change updates the documentation of the interface,
> > > removing the reference to xyz.openbmc_project.Time.Epoch and adding a
> > > reference to xyz.openbmc_project.Common.Progress. This is to remove
> > > the need for updating the creation time in multiple locations.
> > >
> > > You can review the change here:
> > > https://gerrit.openbmc.org/c/openbmc/phosphor-dbus-interfaces/+/66680
> > >
> > > Please note that this change will have an impact on any applications
> > > that are currently reading the dump creation time from the EpochTime
> > > interface. These applications will need to be updated to read the
> > > creation time from the xyz.openbmc_project.Common.Progress interface
> > > instead.
> > > Link to the interface
> > > https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Common/Progress.interface.yaml
> > >
> > > I would appreciate it if you could take a look at the change and
> > > provide any feedback you have.
> > >
> >
> > It looks like the potential concern would be with bmcweb.  There appears
> > to maybe be some common code related to LogServices that expects all
> > logs to have the Time.EpochTime interface.  Are you going to add
> > alternative code there to look at the Common.Progress interface instead?
> > Is this acceptable to the bmcweb side?
>
> Common.Progress interface is already implemented in dump entry and
> bmcweb reads that
> for the status of the dump, now that needs to be extended to read the
> CompletedTime also.
> >
> > --
> > Patrick Williams
>
>
>
> --
> --------------
> Dhruvaraj S