<div dir="ltr">Hi Artem,<div><br></div><div>I listed the potential changes below (to the best of my understand of phosphor-hostlogger),</div><div><br></div><div>1. log_buffer.xpp: </div><div>will be removed; we propose to persist a chunk into the log file as soon as we receive it; newline will be logged as is, so logs are still splitted by newlines; we can potentially set the maximum length of a log as well (force split a long line into smaller lines)</div><div><br></div><div>2. host_console.xpp: stay unchanged;</div><div><br></div><div>3. zlib_file.xpp, zlib_exception.xpp: </div><div>will be removed or slightly changed; we can potentially use the linux logrotate which has built-in compression and file rotation (in this case these compression utilities will be removed). </div><div>The latest log file isn't compressed any more. History log files are still compressed.</div><div>We need to implement some codes to deal with the race condition in log rotations (we should reopen the writing fd after the latest log file is renamed; inotify might do the trick).</div><div>We might also need to rotate according to host boot cycles (like what postcodes are doing right now).</div><div><br></div><div>3. dbus_loop.xpp: stay unchanged;</div><div><br></div><div>4. service.xpp: </div><div>will be slightly changed; socket IO callback and host state watcher are kept; manual flash or flash upon service restart will be removed</div><div><br></div><div>5. config.xpp:</div><div>will be slightly changed; options for flash policy will be removed;</div><div><br></div><div>We might split the implementation into several phases. We might not jump to the final version at one iteration. But the above are what we eventually want at this moment.</div><div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 25, 2021 at 8:03 AM Nan Zhou <<a href="mailto:nanzhou@google.com">nanzhou@google.com</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"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="color:rgb(80,0,80)">"we propose to remove the ring buffer and write to the log file as soon<br></span>as some characters are received. This implicitly removes the needs of<br><span style="color:rgb(80,0,80)">the ring buffer, and the persistence triggering (host reboot, sigterm,<br></span><span style="color:rgb(80,0,80)">etc) in hostlogger"</span><span style="color:rgb(80,0,80)"><br></span>I would like to get a more detailed description of further changes in order<br>to see the whole picture of the solution.</blockquote><div> Originally I didn't consider including changing the log file according to the boot cycle; we will keep part of the dbus/signal watcher to make that different reboot posts to a different log file.</div><div><br></div><div>We will work out some more detailed descriptions for future changes soon.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 24, 2021 at 11:41 PM Artem Senichev <<a href="mailto:artemsen@gmail.com" target="_blank">artemsen@gmail.com</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">Sorry guys, maybe this is a misunderstanding on my part.<br>
<br>
I was confused with the following line in the proposal:<br>
<br>
"we propose to remove the ring buffer and write to the log file as soon<br>
as some characters are received. This implicitly removes the needs of<br>
the ring buffer, and the persistence triggering (host reboot, sigterm,<br>
etc) in hostlogger"<br>
<br>
I would like to get a more detailed description of further changes in order<br>
to see the whole picture of the solution. <br>
<br>
--<br>
Regards,<br>
Artem Senichev<br>
Software Engineer, YADRO.<br>
<br>
<br>
On Mon, May 24, 2021 at 09:27:39AM -0700, Ed Tanous wrote:<br>
> On Mon, May 24, 2021 at 12:52 AM Artem Senichev <<a href="mailto:artemsen@gmail.com" target="_blank">artemsen@gmail.com</a>> wrote:<br>
> ><br>
> > I'll try to convey the main idea that we tried to implement in this service.<br>
> ><br>
> > Hostlogger was originally designed to work with OpenPOWER systems, which<br>
> > generate a very detailed log at boot time.<br>
> <br>
> There are definitely other non OpenPOWER systems that have this same behavior.<br>
> <br>
> > It is important to save these logs and the host console output just before<br>
> > rebooting for further investigation of incidents when hardware errors occur.<br>
> > So, we have two log files for each server session (boot log + last OS messages).<br>
> > That's why we need a D-bus watcher.<br>
> > BMC flash has around 3MiB of free RW space, this force us to use compression<br>
> > and file rotation.<br>
> ><br>
> > All of these features are unnecessary for "streaming" real-time log recording.<br>
> <br>
> I disagree with you there.  Rotation and compression are still useful<br>
> in a "streaming" case.  Because of the ways the APIs are defined,<br>
> LogService in redfish provides both a "historical" version of past<br>
> logs.  It's useful to have those logs rotated and compressed.<br>
> <br>
> > You don't need DBus watchers, rotation can be done with native Linux utilities,<br>
> > you don't even need to split the input stream into lines.<br>
> <br>
> I'm not following why those now wouldn't be needed.  Redfish LogEntry<br>
> would separate per line, so we'd have to do the splitting somewhere.<br>
> There's already code to do that in hostlogger.  Wouldn't you still<br>
> want to separate log per boot, and have lines split between log files?<br>
>  I'm not following why those would go away just because there's a<br>
> desire to poll for logs and get up to date information.<br>
> <br>
> > Just redirect obmc-console.log: `tail -f /var/log/obmc-console.log > my.log`.<br>
> <br>
> This doesn't solve the problem presented.  First of all, log rotation<br>
> and compression are still needed.  Also, it's desirable to have dbus<br>
> watchers and separate the logs per boot, such that they can end up<br>
> separated in the Redfish API.  Also, in what you presented, my.log<br>
> would quickly and easily overflow the available space, as there's no<br>
> log rotation.<br>
> <br>
> ><br>
> > I understand your desire to add a new mode to the existing project instead of<br>
> > creating a new one. But there is very little in common between these modes.<br>
> <br>
> I'm not following how they're all that different, see above about<br>
> needing very similar features.  For the sake of argument, lets say we<br>
> went with a totally different implementation, would it be able to live<br>
> in the hostlogger repo to be able to reuse code where needed?  There's<br>
> a lot of code that I suspect will be identical.<br>
> <br>
> > Even reading the socket will have to be done separately, since it is buffered<br>
> > for line splitting in the current implementation.<br>
> > In the end, only bb-recipe and the `main` function will remain in the common.<br>
> ><br>
> > --<br>
> > Regards,<br>
> > Artem Senichev<br>
> > Software Engineer, YADRO.<br>
> ><br>
> > On Fri, May 21, 2021 at 10:51:45AM -0700, Nan Zhou wrote:<br>
> ><br>
> > > ><br>
> > > > ><br>
> > > > > > we propose to remove the ring buffer and write to the log file<br>
> > > > > > as soon as some characters are received.<br>
> > > > ><br>
> > > > > I am not sure it is a good idea.<br>
> > > > > The host can generate a lot of logs, but we have very limited free space.<br>
> > > > This is a fair concern, but wouldn't the rollover limits make this not<br>
> > > > an issue?  They seem like they could be easily configured.<br>
> > ><br>
> > > Right. Logrotate will be able to handle the rotation. Maximum size, # log<br>
> > > files, and compression can be easily configured.<br>
> > ><br>
> > > > In<br>
> > > > > addition, such heavy traffic will lead to a quick breakdown of the flash<br>
> > > > (most<br>
> > > > > available products are guaranteed to withstand around 100,000 P/E cycles<br>
> > > > only).<br>
> > > > JFFS2 is wear leveled, and there are other BMC stacks that I know of<br>
> > > > that implement this without any ill effects to flash longevity, with<br>
> > > > that said, if Nan made the "last log on disk" feature configurable,<br>
> > > > would that alleviate your concerns?<br>
> > ><br>
> > > We also noticed that the obmc-server itself will buffer the log a bit. Will<br>
> > > it still be a problem if we don't write a character at once but a block of<br>
> > > them?<br>
> > > And as Ed said, we can also make this feature configurable. I would imagine<br>
> > > the log buffer will remain if the "last log on disk" feature is disabled.<br>
> > ><br>
> > ><br>
> > > > ><br>
> > > > > > This implicitly removes the needs<br>
> > > > > > of the ring buffer, and the persistence triggering (host reboot,<br>
> > > > sigterm,<br>
> > > > > > etc) in hostlogger. We believe this keeps the same functionality but<br>
> > > > saves<br>
> > > > > > hundreds of lines of codes in phosphor-hostlogger.<br>
> > > > Difference of opinion here, I don't think this removes the need for<br>
> > > > the host reboot event;  Having each reboot post to a different log<br>
> > > > needs to be maintained, and I have to imagine that there's some sort<br>
> > > > of sigterm handler still, although it becomes a lot smaller.<br>
> > ><br>
> > ><br>
> > > ><br>
> > > > > You are suggesting to delete the buffer, DBus watcher, log rotate. How<br>
> > > > are you<br>
> > > > > going to keep the same functionality if you remove everything related to<br>
> > > > it?<br>
> > > > +1.  In the initial thought I didn't think we were removing any<br>
> > > > functionality with this.  I had assumed the dbus watcher would remain,<br>
> > > > and we would still have the log rotation behavior.  In reading through<br>
> > > > Nans proposal I don't think these are getting removed;  Maybe I<br>
> > > > misunderstood?<br>
> > ><br>
> > ><br>
> > > Yes, if we want to keep different reboot posts to a different log file, we<br>
> > > can keep part of the dbus/signal watcher.<br>
> > ><br>
> > > On Fri, May 21, 2021 at 10:24 AM Ed Tanous <<a href="mailto:edtanous@google.com" target="_blank">edtanous@google.com</a>> wrote:<br>
> > ><br>
> > > > On Thu, May 20, 2021 at 11:10 PM Artem Senichev <<a href="mailto:artemsen@gmail.com" target="_blank">artemsen@gmail.com</a>><br>
> > > > wrote:<br>
> > > > ><br>
> > > > > On Thu, May 20, 2021 at 04:29:09PM -0700, Nan Zhou wrote:<br>
> > > > > > Hi all,<br>
> > > > > ><br>
> > > > > > In the previous thread (<br>
> > > > > > <a href="https://lists.ozlabs.org/pipermail/openbmc/2021-March/025234.html" rel="noreferrer" target="_blank">https://lists.ozlabs.org/pipermail/openbmc/2021-March/025234.html</a>), we<br>
> > > > > > (engineers from Google and Quanta) discussed our attempt to share host<br>
> > > > > > serial logs via Redfish, which includes polling logs via LogService and<br>
> > > > > > streaming log bytes via EventService (e.g. SSE). We went with the<br>
> > > > event log<br>
> > > > > > architecture<br>
> > > > > > <<br>
> > > > <a href="https://github.com/openbmc/docs/blob/master/architecture/redfish-logging-in-bmcweb.md" rel="noreferrer" target="_blank">https://github.com/openbmc/docs/blob/master/architecture/redfish-logging-in-bmcweb.md</a><br>
> > > > ><br>
> > > > > > and did the implementation.<br>
> > > > > ><br>
> > > > > > We still want to reuse the phosphor-hostlogger and do some<br>
> > > > modification. We<br>
> > > > > > believe it is better to try to reuse existing codes if possible and<br>
> > > > improve<br>
> > > > > > them rather than creating new things that have similar functionalities<br>
> > > > (in<br>
> > > > > > our case, it is a daemon that could collect logs and persist them).<br>
> > > > ><br>
> > > > > I agree, reusing code is a right choice, but only when it is really<br>
> > > > possible.<br>
> > > > > For now it looks like you want to remove most of the Hostlogger features<br>
> > > > to<br>
> > > > > transform it from buffer-like to stream-like service.<br>
> > > ><br>
> > > > I'm not quite following this statement.  Which features are getting<br>
> > > > removed?  From what I can see, he's suggesting making<br>
> > > > phosphor-hostlogger look more like a well-behaved linux logging<br>
> > > > daemon, but I don't think any features got omitted (or I'm missing<br>
> > > > something critical).<br>
> > > ><br>
> > > > ><br>
> > > > > > We want to do the following modification in phosphor-hostlogger (from<br>
> > > > the<br>
> > > > > > input and output point of view, just very few things will be changed)<br>
> > > > > ><br>
> > > > > > 1. One limitation of phosphor-hostlogger is that it will lose data in<br>
> > > > the<br>
> > > > > > memory (the ring buffer maintained by hostlogger) when BMC gets force<br>
> > > > > > restarted;<br>
> > > > ><br>
> > > > > When BMC goes to reboot it stops all services, at that moment hostlogger<br>
> > > > gets<br>
> > > > > a signal and flushes all gathered logs to a file.<br>
> > > ><br>
> > > > Only if the reboot is planned.  If the BMC loses power (which is<br>
> > > > "normal" for a bmc) there isn't time to persist to flash before the<br>
> > > > power goes down and the logs are most likely lost.<br>
> > > ><br>
> > > > ><br>
> > > > > > we propose to remove the ring buffer and write to the log file<br>
> > > > > > as soon as some characters are received.<br>
> > > > ><br>
> > > > > I am not sure it is a good idea.<br>
> > > > > The host can generate a lot of logs, but we have very limited free space.<br>
> > > ><br>
> > > > This is a fair concern, but wouldn't the rollover limits make this not<br>
> > > > an issue?  They seem like they could be easily configured.<br>
> > > ><br>
> > > > > In<br>
> > > > > addition, such heavy traffic will lead to a quick breakdown of the flash<br>
> > > > (most<br>
> > > > > available products are guaranteed to withstand around 100,000 P/E cycles<br>
> > > > only).<br>
> > > ><br>
> > > > JFFS2 is wear leveled, and there are other BMC stacks that I know of<br>
> > > > that implement this without any ill effects to flash longevity, with<br>
> > > > that said, if Nan made the "last log on disk" feature configurable,<br>
> > > > would that alleviate your concerns?<br>
> > > ><br>
> > > > ><br>
> > > > > > This implicitly removes the needs<br>
> > > > > > of the ring buffer, and the persistence triggering (host reboot,<br>
> > > > sigterm,<br>
> > > > > > etc) in hostlogger. We believe this keeps the same functionality but<br>
> > > > saves<br>
> > > > > > hundreds of lines of codes in phosphor-hostlogger.<br>
> > > ><br>
> > > > Difference of opinion here, I don't think this removes the need for<br>
> > > > the host reboot event;  Having each reboot post to a different log<br>
> > > > needs to be maintained, and I have to imagine that there's some sort<br>
> > > > of sigterm handler still, although it becomes a lot smaller.<br>
> > > ><br>
> > > > ><br>
> > > > > You are suggesting to delete the buffer, DBus watcher, log rotate. How<br>
> > > > are you<br>
> > > > > going to keep the same functionality if you remove everything related to<br>
> > > > it?<br>
> > > ><br>
> > > > +1.  In the initial thought I didn't think we were removing any<br>
> > > > functionality with this.  I had assumed the dbus watcher would remain,<br>
> > > > and we would still have the log rotation behavior.  In reading through<br>
> > > > Nans proposal I don't think these are getting removed;  Maybe I<br>
> > > > misunderstood?<br>
> > > ><br>
> > > > ><br>
> > > > > > 2. We propose not to compress the latest log file. This saves us the<br>
> > > > > > overhead of doing decompression when BMCWeb just needs to retrieve the<br>
> > > > most<br>
> > > > > > recent logs. There are still going to be log rotations in the file<br>
> > > > level.<br>
> > > > > > Files other than the latest log file are still going to be compressed.<br>
> > > > We<br>
> > > > > > can modify existing codes to achieve this or use the linux logrotate<br>
> > > > > > directly.<br>
> > > > > ><br>
> > > > > > Furthermore, we will add host serial logs into BMCWeb, both LogService<br>
> > > > and<br>
> > > > > > EventService. In LogService, we will teach BMCWeb how to read the<br>
> > > > latest<br>
> > > > > > log file that is not compressed and the other compressed old logs, and<br>
> > > > how<br>
> > > > > > to assemble LogEntries out of raw serial logs. We will discuss<br>
> > > > EventService<br>
> > > > > > in future threads but the very initial idea is to setup inotify on log<br>
> > > > > > files and SSE to stream out new bytes to clients (like what the<br>
> > > > existing<br>
> > > > > > event logging is doing).<br>
> > > > > ><br>
> > > > > > As we said above, for phosphor-hostlogger, the input is still the<br>
> > > > > > obmc-server unix socket, and the output are still persisted log files.<br>
> > > > But<br>
> > > > > > the functionality will get improved (less data loss), code gets<br>
> > > > simplified<br>
> > > > > > (no ring buffer and persistence triggering), and it will become easier<br>
> > > > and<br>
> > > > > > more performant to get BMCWeb hooked up for log streaming via Redfish.<br>
> > > > > ><br>
> > > > > > Please let us know what you think. We appreciate any feedback. Thank<br>
> > > > you<br>
> > > > > > very much!<br>
> > > > > ><br>
> > > > > > Sincerely,<br>
> > > > > > Nan<br>
> > > > ><br>
> > > > > --<br>
> > > > > Regards,<br>
> > > > > Artem Senichev<br>
> > > > > Software Engineer, YADRO.<br>
> > > ><br>
</blockquote></div>
</blockquote></div>