Proposal: Removing redundant EpochTime interface from dump entry

[dhruvaraj S]

On Thu, 21 Sept 2023 at 22:59, Ed Tanous <ed at tanous.net> wrote:
>
> 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.
The progress interface contains both the start time and completed
time, the start time
is set when the request comes and completed time once the dump package
is completed, EpochTime is also set when the packaging is completed, so both
are representing the same value now.

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



-- 
--------------
Dhruvaraj S