<div dir="ltr"><br><br>On Fri, 22 Sept 2023 at 01:19, Ed Tanous <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br>><br>> On Thu, Sep 21, 2023 at 11:59 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com">dhruvaraj@gmail.com</a>> wrote:<br>> ><br>> > On Thu, 21 Sept 2023 at 23:22, Ed Tanous <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br>> > ><br>> > > On Thu, Sep 21, 2023 at 10:47 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com">dhruvaraj@gmail.com</a>> wrote:<br>> > > ><br>> > > > On Thu, 21 Sept 2023 at 22:59, Ed Tanous <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br>> > > > ><br>> > > > > On Thu, Sep 21, 2023 at 8:44 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com">dhruvaraj@gmail.com</a>> wrote:<br>> > > > > ><br>> > > > > > On Thu, 21 Sept 2023 at 21:09, Patrick Williams <<a href="mailto:patrick@stwcx.xyz">patrick@stwcx.xyz</a>> wrote:<br>> > > > > > ><br>> > > > > > > On Thu, Sep 21, 2023 at 08:52:15AM +0530, dhruvaraj S wrote:<br>> > > > > > > > Hi,<br>> > > > > > > ><br>> > > > > > > > In the current implementation, objects implementing the dump entry<br>> > > > > > > > interface implement both xyz.openbmc_project.Common.Progress (for dump<br>> > > > > > > > creation start time, end time, and status) and<br>> > > > > > > > xyz.openbmc_project.Time.Epoch (for dump creation time, which is<br>> > > > > > > > effectively the end time). This leads to a redundancy in specifying<br>> > > > > > > > the dump creation end time.<br>> > > > ><br>> > > > > Don't the two interfaces describe different things? Time.Epoch<br>> > > > > represents when the event occurred, not when the recording of that<br>> > > > > event was complete, Common.progress represents when the<br>> > > > > logging/processing of that event was complete. In a lot of on-bmc<br>> > > > > scenarios, they're going to be similar if not the same, but in the<br>> > > > > case of something like a remote processor flagging an error, they're<br>> > > > > going to be different. An error might not be completely processed<br>> > > > > until minutes later.<br>> > > > The progress interface contains both the start time and completed<br>> > > > time, the start time<br>> > ><br>> > > That's the time that the dump started. The dump could've been<br>> > > triggered by something that happened before it started, because of<br>> > > scheduling or resources. To be more clear, the timeline goes<br>> > > something like:<br>> > ><br>> > > Some hardware failure happens -> Time.Epoch<br>> > > Dump creation starts -> Common.Progress.Start<br>> > > Dump creation completes -> Common.Progress.End<br>> > ><br>> > > If the dumps are manually triggered, and there is no queuing time,<br>> > > yes, steps 1 and 2 will likely report the same value (which is the<br>> > > common case), but that doesn't mean they're the same in all cases.<br>> ><br>> > Based on the documentation<br>> > <a href="https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Dump/Entry.interface.yaml">https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Dump/Entry.interface.yaml</a><br>> > TimeEpoch is dump creation time, which is assumed as the time when the<br>> > final packaging<br>> > is completed.<br>> > The current implementation for BMC dump is like,<br>> > - Dump requested, an entry created with status set as InProgress and<br>> > Start Time is recorded<br>> > - Once the packaging is completed, the Status is changed to Completed,<br>> > Completed Time and TimeEpoch are set to the same value.<br>> ><br>> > But I agree some implementations can have the Time.Epoch set to the start time,<br>> > which may be close to the hardware or software failure in the system.<br>> > I think it is fine to keep TimeEpoch also to set the failure time, but<br>> > there can be a PEL<br>> > logged in such cases, and the failure time can be available from that right?<br>><br>> Lets see what it actually saves us in terms of code and patchsets to<br>> do this simplification (even if it's just a couple daemons that are<br>> representative), and continue from there. Unless I'm mistaken, this<br>> is going to change on the order of 10 lines of code diff in the<br>> daemons for this simplification, which is fine, but without seeing the<br>> patches, it's hard to assess impact to actual use cases, and whether<br>> those lines of diff are worth reworking and retesting everything.<br><br>Proposed changes to phosphor-debug-collector and bmcweb:<br><br>Although the number of lines of code changed is small, as you mentioned,<br>this change will remove the duplication of information in two places and the<br>dependency on an additional interface.<br><br>Links to the Gerrit changes:<br><br>bmcweb: <a href="https://gerrit.openbmc.org/c/openbmc/bmcweb/+/66732">https://gerrit.openbmc.org/c/openbmc/bmcweb/+/66732</a><br>Phosphor-debug-collector: <a href="https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/66606">https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/66606</a></div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Sept 2023 at 01:19, Ed Tanous <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Sep 21, 2023 at 11:59 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com" target="_blank">dhruvaraj@gmail.com</a>> wrote:<br>
><br>
> On Thu, 21 Sept 2023 at 23:22, Ed Tanous <<a href="mailto:ed@tanous.net" target="_blank">ed@tanous.net</a>> wrote:<br>
> ><br>
> > On Thu, Sep 21, 2023 at 10:47 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com" target="_blank">dhruvaraj@gmail.com</a>> wrote:<br>
> > ><br>
> > > On Thu, 21 Sept 2023 at 22:59, Ed Tanous <<a href="mailto:ed@tanous.net" target="_blank">ed@tanous.net</a>> wrote:<br>
> > > ><br>
> > > > On Thu, Sep 21, 2023 at 8:44 AM dhruvaraj S <<a href="mailto:dhruvaraj@gmail.com" target="_blank">dhruvaraj@gmail.com</a>> wrote:<br>
> > > > ><br>
> > > > > On Thu, 21 Sept 2023 at 21:09, Patrick Williams <<a href="mailto:patrick@stwcx.xyz" target="_blank">patrick@stwcx.xyz</a>> wrote:<br>
> > > > > ><br>
> > > > > > On Thu, Sep 21, 2023 at 08:52:15AM +0530, dhruvaraj S wrote:<br>
> > > > > > > Hi,<br>
> > > > > > ><br>
> > > > > > > In the current implementation, objects implementing the dump entry<br>
> > > > > > > interface implement both xyz.openbmc_project.Common.Progress (for dump<br>
> > > > > > > creation start time, end time, and status) and<br>
> > > > > > > xyz.openbmc_project.Time.Epoch (for dump creation time, which is<br>
> > > > > > > effectively the end time). This leads to a redundancy in specifying<br>
> > > > > > > the dump creation end time.<br>
> > > ><br>
> > > > Don't the two interfaces describe different things? Time.Epoch<br>
> > > > represents when the event occurred, not when the recording of that<br>
> > > > event was complete, Common.progress represents when the<br>
> > > > logging/processing of that event was complete. In a lot of on-bmc<br>
> > > > scenarios, they're going to be similar if not the same, but in the<br>
> > > > case of something like a remote processor flagging an error, they're<br>
> > > > going to be different. An error might not be completely processed<br>
> > > > until minutes later.<br>
> > > The progress interface contains both the start time and completed<br>
> > > time, the start time<br>
> ><br>
> > That's the time that the dump started. The dump could've been<br>
> > triggered by something that happened before it started, because of<br>
> > scheduling or resources. To be more clear, the timeline goes<br>
> > something like:<br>
> ><br>
> > Some hardware failure happens -> Time.Epoch<br>
> > Dump creation starts -> Common.Progress.Start<br>
> > Dump creation completes -> Common.Progress.End<br>
> ><br>
> > If the dumps are manually triggered, and there is no queuing time,<br>
> > yes, steps 1 and 2 will likely report the same value (which is the<br>
> > common case), but that doesn't mean they're the same in all cases.<br>
><br>
> Based on the documentation<br>
> <a href="https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Dump/Entry.interface.yaml" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Dump/Entry.interface.yaml</a><br>
> TimeEpoch is dump creation time, which is assumed as the time when the<br>
> final packaging<br>
> is completed.<br>
> The current implementation for BMC dump is like,<br>
> - Dump requested, an entry created with status set as InProgress and<br>
> Start Time is recorded<br>
> - Once the packaging is completed, the Status is changed to Completed,<br>
> Completed Time and TimeEpoch are set to the same value.<br>
><br>
> But I agree some implementations can have the Time.Epoch set to the start time,<br>
> which may be close to the hardware or software failure in the system.<br>
> I think it is fine to keep TimeEpoch also to set the failure time, but<br>
> there can be a PEL<br>
> logged in such cases, and the failure time can be available from that right?<br>
<br>
Lets see what it actually saves us in terms of code and patchsets to<br>
do this simplification (even if it's just a couple daemons that are<br>
representative), and continue from there. Unless I'm mistaken, this<br>
is going to change on the order of 10 lines of code diff in the<br>
daemons for this simplification, which is fine, but without seeing the<br>
patches, it's hard to assess impact to actual use cases, and whether<br>
those lines of diff are worth reworking and retesting everything.<br>
<br>
<br>
><br>
> ><br>
> > > is set when the request comes and completed time once the dump package<br>
> > > is completed, EpochTime is also set when the packaging is completed, so both<br>
> > > are representing the same value now.<br>
> > ><br>
> > > ><br>
> > > > FWIW, in terms of complexity reduction on DBus, I think there's a lot<br>
> > > > more impactful places to start for a lot less effort, but if this is<br>
> > > > something you really want to chase to conclusion, and it reduces the<br>
> > > > code complexity (measured by a net-negative diff patchset to OpenBMC)<br>
> > > > and you're willing to test all the scenarios, feel free to continue to<br>
> > > > chase it down.<br>
> > > ><br>
> > > > > > ><br>
> > > > > > > My proposed change updates the documentation of the interface,<br>
> > > > > > > removing the reference to xyz.openbmc_project.Time.Epoch and adding a<br>
> > > > > > > reference to xyz.openbmc_project.Common.Progress. This is to remove<br>
> > > > > > > the need for updating the creation time in multiple locations.<br>
> > > > > > ><br>
> > > > > > > You can review the change here:<br>
> > > > > > > <a href="https://gerrit.openbmc.org/c/openbmc/phosphor-dbus-interfaces/+/66680" rel="noreferrer" target="_blank">https://gerrit.openbmc.org/c/openbmc/phosphor-dbus-interfaces/+/66680</a><br>
> > > > > > ><br>
> > > > > > > Please note that this change will have an impact on any applications<br>
> > > > > > > that are currently reading the dump creation time from the EpochTime<br>
> > > > > > > interface. These applications will need to be updated to read the<br>
> > > > > > > creation time from the xyz.openbmc_project.Common.Progress interface<br>
> > > > > > > instead.<br>
> > > > > > > Link to the interface<br>
> > > > > > > <a href="https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Common/Progress.interface.yaml" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Common/Progress.interface.yaml</a><br>
> > > > > > ><br>
> > > > > > > I would appreciate it if you could take a look at the change and<br>
> > > > > > > provide any feedback you have.<br>
> > > > > > ><br>
> > > > > ><br>
> > > > > > It looks like the potential concern would be with bmcweb. There appears<br>
> > > > > > to maybe be some common code related to LogServices that expects all<br>
> > > > > > logs to have the Time.EpochTime interface. Are you going to add<br>
> > > > > > alternative code there to look at the Common.Progress interface instead?<br>
> > > > > > Is this acceptable to the bmcweb side?<br>
> > > > ><br>
> > > > > Common.Progress interface is already implemented in dump entry and<br>
> > > > > bmcweb reads that<br>
> > > > > for the status of the dump, now that needs to be extended to read the<br>
> > > > > CompletedTime also.<br>
> > > > > ><br>
> > > > > > --<br>
> > > > > > Patrick Williams<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > --<br>
> > > > > --------------<br>
> > > > > Dhruvaraj S<br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > --------------<br>
> > > Dhruvaraj S<br>
><br>
><br>
><br>
> --<br>
> --------------<br>
> Dhruvaraj S<br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature">--------------<br>Dhruvaraj S</div>