Proposal: Removing redundant EpochTime interface from dump entry

[dhruvaraj S]

On Thu, 21 Sept 2023 at 23:22, Ed Tanous <ed at tanous.net> wrote:
>
> On Thu, Sep 21, 2023 at 10:47 AM dhruvaraj S <dhruvaraj at gmail.com> wrote:
> >
> > 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
>
> That's the time that the dump started.  The dump could've been
> triggered by something that happened before it started, because of
> scheduling or resources.  To be more clear, the timeline goes
> something like:
>
> Some hardware failure happens -> Time.Epoch
> Dump creation starts -> Common.Progress.Start
> Dump creation completes -> Common.Progress.End
>
> If the dumps are manually triggered, and there is no queuing time,
> yes, steps 1 and 2 will likely report the same value (which is the
> common case), but that doesn't mean they're the same in all cases.

Based on the documentation
https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Dump/Entry.interface.yaml
TimeEpoch is dump creation time, which is assumed as the time when the
final packaging
is completed.
The current implementation for BMC dump is like,
- Dump requested, an entry created with status set as InProgress and
Start Time is recorded
- Once the packaging is completed, the Status is changed to Completed,
Completed Time and TimeEpoch are set to the same value.

But I agree some implementations can have the Time.Epoch set to the start time,
which may be close to the hardware or software failure in the system.
I think it is fine to keep TimeEpoch also to set the failure time, but
there can be a PEL
logged in such cases, and the failure time can be available from that right?

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



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