Proposal: Removing redundant EpochTime interface from dump entry
dhruvaraj S
dhruvaraj at gmail.com
Fri Sep 22 19:08:07 AEST 2023
On Fri, 22 Sept 2023 at 01:19, Ed Tanous <ed at tanous.net> wrote:
>
> On Thu, Sep 21, 2023 at 11:59 AM dhruvaraj S <dhruvaraj at gmail.com> wrote:
> >
> > 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?
>
> Lets see what it actually saves us in terms of code and patchsets to
> do this simplification (even if it's just a couple daemons that are
> representative), and continue from there. Unless I'm mistaken, this
> is going to change on the order of 10 lines of code diff in the
> daemons for this simplification, which is fine, but without seeing the
> patches, it's hard to assess impact to actual use cases, and whether
> those lines of diff are worth reworking and retesting everything.
Proposed changes to phosphor-debug-collector and bmcweb:
Although the number of lines of code changed is small, as you mentioned,
this change will remove the duplication of information in two places and the
dependency on an additional interface.
Links to the Gerrit changes:
bmcweb: https://gerrit.openbmc.org/c/openbmc/bmcweb/+/66732
Phosphor-debug-collector:
https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/66606
On Fri, 22 Sept 2023 at 01:19, Ed Tanous <ed at tanous.net> wrote:
> On Thu, Sep 21, 2023 at 11:59 AM dhruvaraj S <dhruvaraj at gmail.com> wrote:
> >
> > 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?
>
> Lets see what it actually saves us in terms of code and patchsets to
> do this simplification (even if it's just a couple daemons that are
> representative), and continue from there. Unless I'm mistaken, this
> is going to change on the order of 10 lines of code diff in the
> daemons for this simplification, which is fine, but without seeing the
> patches, it's hard to assess impact to actual use cases, and whether
> those lines of diff are worth reworking and retesting everything.
>
>
> >
> > >
> > > > 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
>
--
--------------
Dhruvaraj S
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20230922/3c769714/attachment-0001.htm>
More information about the openbmc
mailing list