<div dir="ltr">My 2c... We have plenty of blob handlers that have their own repos to keep maintainership and purposes separate. The phosphor-ipmi-blobs repository intends to provide a framework, not specific implementations.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 10, 2020 at 10:35 PM Vijay Khemka <<a href="mailto:vijaykhemka@fb.com">vijaykhemka@fb.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"><br>
<br>
On 11/5/20, 3:55 PM, "Sui Chen" <<a href="mailto:suichen@google.com" target="_blank">suichen@google.com</a>> wrote:<br>
<br>
On Tue, Oct 6, 2020 at 6:43 PM Patrick Williams <<a href="mailto:patrick@stwcx.xyz" target="_blank">patrick@stwcx.xyz</a>> wrote:<br>
><br>
> On Tue, Oct 06, 2020 at 03:57:30PM -0700, Sui Chen wrote:<br>
> > On Fri, Oct 2, 2020 at 1:54 PM Vijay Khemka <<a href="mailto:vijaykhemka@fb.com" target="_blank">vijaykhemka@fb.com</a>> wrote:<br>
> > > If I understand correctly, protocol buffer will be used by daemon who<br>
> > > Is responding to the IPMI request and connecting to this daemon via<br>
> > > library call, then it is completely restricted for the use of protocol buffer.<br>
> > > If you are passing protocol buffer to this daemon then we have to define<br>
> > > some policy here.<br>
> ><br>
> > The Protocol buffer is only for serializing the data to be sent<br>
> > outside of the BMC. It is not used for communication inside<br>
> > phosphor-health-monitor and will not be passed to the daemon.<br>
><br>
> Why isn't this part done from within an existing IPMI provider (ideally<br>
> to me a google-ipmi-* repository at this time)? I'm not especially keen<br>
> on these details leaking out into other non-IPMI repositories.<br>
><br>
> > ><br>
> > > Other than these two things I think adding new metrics to<br>
> > > phosphor-health-monitor should be manageable. I can start by trying to<br>
> > > add the IPMI blob handler to phosphor-health-monitor; my first attempt<br>
> > > might not look very elegant, but if we find answers to the two<br>
> > > questions above, the merged result will look a lot better. Hopefully<br>
> > > we can find a solution that works well for everyone.<br>
> > ><br>
> > > I am looking forward to your patches<br>
> ><br>
> > Please check out this WIP:<br>
> > <a href="https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/37092" rel="noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/37092</a> <br>
> ><br>
> > This WIP currently just adds the IPMI blob-based code to<br>
> > phosphor-health-monitor almost as-is.<br>
> > It also shows what we already have now.<br>
> ><br>
> > There will be some work to merge the daemon and the blob handler in an<br>
> > organic way, and I am open to discussion with you how to do that. The<br>
> > first step I think I can do is to put the code for extracting the<br>
> > metrics (metrics.cpp, blob/metric.cpp) into a single file and share<br>
> > that between the daemon and the IPMI blob handler.<br>
> ><br>
> > Another issue I found is I am not using the latest sdbusplus so I have<br>
> > to comment out the usage of ValueIface::Unit::Percent for now.<br>
> ><br>
> > To build this requires 1) adding a pkgconfig file to<br>
> > phosphor-ipmi-blobs (before<br>
> > <a href="https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-ipmi-blobs/+/37133" rel="noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-ipmi-blobs/+/37133</a> <br>
> > gets merged) and 2) adding phosphor-ipmi-blobs and protobuf to DEPENDS<br>
> > in phosphor-health-monitor's Bitbake recipe.<br>
> ><br>
> > Hope this WIP change illustrates our intention clearly.<br>
> ><br>
> > Thanks!<br>
><br>
> --<br>
> Patrick Williams<br>
<br>
<br>
Hello Patrick and Vijay,<br>
<br>
As far as I know, the only two "google-ipmi-*" repositories are 1)<br>
google-ipmi-sys and 2) google-ipmi-i2c, and neither seem to be related<br>
to the health monitoring task we're doing right now.<br>
In my understanding one similar library is phosphor-pid-control; its<br>
IPMI handler is also in the repository rather than in a separate<br>
repository.<br>
<br>
The "health monitoring IPMI Blob Handler" (that the request in the<br>
first email in this thread was indended for) was a monolithic IPMI<br>
blob handler; it used to both generate metrics and handle IPMI<br>
requests.<br>
In the last month, I had de-coupled these two functions so the IPMI<br>
blob handler does not generate metrics but reads metrics from the<br>
daemon in phosphor-health-monitor via DBus. In other words, the "monolithic"<br>
handler has now become a thin layer. On the other hand,<br>
phosphor-health-monitor will have to be significantly modified to<br>
generate the metrics that are in a different format from what it's<br>
generating right now, and Vijay and I are working on that. I had create a chain<br>
of changes <a href="https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/37659" rel="noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/37659</a> <br>
to illustrate what I intend to do.<br>
As a result, there comes the question of where the IPMI blob handler<br>
should live, and it appears I have the following choices:<br>
1. in phosphor-health-monitor, or<br>
2. some centralized location, along with many other IPMI blob handlers, or<br>
3. as a separate, new repository, or<br>
4. something else?<br>
<br>
I am facing a confusing situation as to where I should put the IPMI<br>
blob handler, due to conflicting opinions:<br>
1. The maintainers of phosphor-ipmi-blobs told me it's not desirable<br>
to put all IPMI blob handlers into the same place.<br>
2. By reading this email thread, I had the impression that it's not a<br>
good idea to create too many repositories either.<br>
3. Because of #1 and #2, I felt we should put the IPMI blob handler<br>
into phosphor-health-monitor itself, just like phosphor-pid-control<br>
does.<br>
4. In the last reply from Patrick it sounds it's a bad idea to put the<br>
IPMI blob handler into phosphor-health-monitor because of IPMI details<br>
leaking out into non-IPMI repositories.<br>
5. Vijay seemed to prefer to have all IPMI blob handlers in one place<br>
based on our discussion on IRC. However, according to #1 this is going<br>
to face pushback. As such, I created all my changes in<br>
phosphor-health-monitor for review and for showing my intent on how<br>
the IPMI implementation is done.<br>
6. Because of #4 and #5, it sounds like I can't put the IPMI blob handler into<br>
phosphor-health-monitor either.<br>
So now, there is no place I can place this handler, and I am now at a dead end.<br>
<br>
I still feel that this should go to phosphor-ipmi-blobs, you can create a separate<br>
directory (handler) under the same repo and it can become home for all the<br>
futures blob handler as these are going to interact with ipmi blobs anyway.<br>
<br>
I need to find a way out and would greatly appreciate it if we can<br>
reach a consensus here so that BMC health monitoring can move forward.<br>
<br>
Thanks!<br>
<br>
</blockquote></div>