On Tue, Nov 10, 2020 at 10:35 PM Vijay Khemka <vijaykhemka@fb.com> wrote:

=EF=BB=BFOn 11/5/20, 3:55 PM, "Sui Chen" <suichen@google.com> wrote:

=C2=A0 =C2=A0 On Tue, Oct 6, 2020 at 6:43 PM Patrick Williams <patrick@stwcx.xyz> w= rote:
=C2=A0 =C2=A0 >
=C2=A0 =C2=A0 > On Tue, Oct 06, 2020 at 03:57:30PM -0700, Sui Chen wrote= :
=C2=A0 =C2=A0 > > On Fri, Oct 2, 2020 at 1:54 PM Vijay Khemka <vijaykhemka@fb.com= > wrote:
=C2=A0 =C2=A0 > > > If I understand correctly, protocol buffer wil= l be used by daemon who
=C2=A0 =C2=A0 > > > Is responding to the IPMI request and connecti= ng to this daemon via
=C2=A0 =C2=A0 > > > library call, then it is completely restricted= for the use of protocol buffer.
=C2=A0 =C2=A0 > > > If you are passing protocol buffer to this dae= mon then we have to define
=C2=A0 =C2=A0 > > > some policy here.
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > The Protocol buffer is only for serializing the dat= a to be sent
=C2=A0 =C2=A0 > > outside of the BMC. It is not used for communicatio= n inside
=C2=A0 =C2=A0 > > phosphor-health-monitor and will not be passed to t= he daemon.
=C2=A0 =C2=A0 >
=C2=A0 =C2=A0 > Why isn't this part done from within an existing IPM= I provider (ideally
=C2=A0 =C2=A0 > to me a google-ipmi-* repository at this time)?=C2=A0 I&= #39;m not especially keen
=C2=A0 =C2=A0 > on these details leaking out into other non-IPMI reposit= ories.
=C2=A0 =C2=A0 >
=C2=A0 =C2=A0 > > >
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0Other than these two things= I think adding new metrics to
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0phosphor-health-monitor sho= uld be manageable. I can start by trying to
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0add the IPMI blob handler t= o phosphor-health-monitor; my first attempt
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0might not look very elegant= , but if we find answers to the two
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0questions above, the merged= result will look a lot better. Hopefully
=C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0we can find a solution that= works well for everyone.
=C2=A0 =C2=A0 > > >
=C2=A0 =C2=A0 > > > I am looking forward to your patches
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > Please check out this WIP:
=C2=A0 =C2=A0 > > = https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/3709= 2
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > This WIP currently just adds the IPMI blob-based co= de to
=C2=A0 =C2=A0 > > phosphor-health-monitor almost as-is.
=C2=A0 =C2=A0 > > It also shows what we already have now.
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > There will be some work to merge the daemon and the= blob handler in an
=C2=A0 =C2=A0 > > organic way, and I am open to discussion with you h= ow to do that. The
=C2=A0 =C2=A0 > > first step I think I can do is to put the code for = extracting the
=C2=A0 =C2=A0 > > metrics (metrics.cpp, blob/metric.cpp) into a singl= e file and share
=C2=A0 =C2=A0 > > that between the daemon and the IPMI blob handler.<= br> =C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > Another issue I found is I am not using the latest = sdbusplus so I have
=C2=A0 =C2=A0 > > to comment out the usage of ValueIface::Unit::Perce= nt for now.
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > To build this requires 1) adding a pkgconfig file t= o
=C2=A0 =C2=A0 > > phosphor-ipmi-blobs (before
=C2=A0 =C2=A0 > > http= s://gerrit.openbmc-project.xyz/c/openbmc/phosphor-ipmi-blobs/+/37133 =C2=A0 =C2=A0 > > gets merged) and 2) adding phosphor-ipmi-blobs and = protobuf to DEPENDS
=C2=A0 =C2=A0 > > in phosphor-health-monitor's Bitbake recipe. =C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > Hope this WIP change illustrates our intention clea= rly.
=C2=A0 =C2=A0 > >
=C2=A0 =C2=A0 > > Thanks!
=C2=A0 =C2=A0 >
=C2=A0 =C2=A0 > --
=C2=A0 =C2=A0 > Patrick Williams

=C2=A0 =C2=A0 Hello Patrick and Vijay,

=C2=A0 =C2=A0 As far as I know, the only two "google-ipmi-*" repo= sitories are 1)
=C2=A0 =C2=A0 google-ipmi-sys and 2) google-ipmi-i2c, and neither seem to b= e related
=C2=A0 =C2=A0 to the health monitoring task we're doing right now.
=C2=A0 =C2=A0 In my understanding one similar library is phosphor-pid-contr= ol; its
=C2=A0 =C2=A0 IPMI handler is also in the repository rather than in a separ= ate
=C2=A0 =C2=A0 repository.

=C2=A0 =C2=A0 The "health monitoring IPMI Blob Handler" (that the= request in the
=C2=A0 =C2=A0 first email in this thread was indended for) was a monolithic= IPMI
=C2=A0 =C2=A0 blob handler; it used to both generate metrics and handle IPM= I
=C2=A0 =C2=A0 requests.
=C2=A0 =C2=A0 In the last month, I had de-coupled these two functions so th= e IPMI
=C2=A0 =C2=A0 blob handler does not generate metrics but reads metrics from= the
=C2=A0 =C2=A0 daemon in phosphor-health-monitor via DBus. In other words, t= he "monolithic"
=C2=A0 =C2=A0 handler has now become a thin layer. On the other hand,
=C2=A0 =C2=A0 phosphor-health-monitor will have to be significantly modifie= d to
=C2=A0 =C2=A0 generate the metrics that are in a different format from what= it's
=C2=A0 =C2=A0 generating right now, and Vijay and I are working on that. I = had create a chain
=C2=A0 =C2=A0 of changes https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/376= 59
=C2=A0 =C2=A0 to illustrate what I intend to do.
=C2=A0 =C2=A0 As a result, there comes the question of where the IPMI blob = handler
=C2=A0 =C2=A0 should live, and it appears I have the following choices:
=C2=A0 =C2=A0 1. in phosphor-health-monitor, or
=C2=A0 =C2=A0 2. some centralized location, along with many other IPMI blob= handlers, or
=C2=A0 =C2=A0 3. as a separate, new repository, or
=C2=A0 =C2=A0 4. something else?

=C2=A0 =C2=A0 I am facing a confusing situation as to where I should put th= e IPMI
=C2=A0 =C2=A0 blob handler, due to conflicting opinions:
=C2=A0 =C2=A0 1. The maintainers of phosphor-ipmi-blobs told me it's no= t desirable
=C2=A0 =C2=A0 to put all IPMI blob handlers into the same place.
=C2=A0 =C2=A0 2. By reading this email thread, I had the impression that it= 's not a
=C2=A0 =C2=A0 good idea to create too many repositories either.
=C2=A0 =C2=A0 3. Because of #1 and #2, I felt we should put the IPMI blob h= andler
=C2=A0 =C2=A0 into phosphor-health-monitor itself, just like phosphor-pid-c= ontrol
=C2=A0 =C2=A0 does.
=C2=A0 =C2=A0 4. In the last reply from Patrick it sounds it's a bad id= ea to put the
=C2=A0 =C2=A0 IPMI blob handler into phosphor-health-monitor because of IPM= I details
=C2=A0 =C2=A0 leaking out into non-IPMI repositories.
=C2=A0 =C2=A0 5. Vijay seemed to prefer to have all IPMI blob handlers in o= ne place
=C2=A0 =C2=A0 based on our discussion on IRC. However, according to #1 this= is going
=C2=A0 =C2=A0 to face pushback. As such, I created all my changes in
=C2=A0 =C2=A0 phosphor-health-monitor for review and for showing my intent = on how
=C2=A0 =C2=A0 the IPMI implementation is done.
=C2=A0 =C2=A0 6. Because of #4 and #5, it sounds like I can't put the I= PMI blob handler into
=C2=A0 =C2=A0 phosphor-health-monitor either.
=C2=A0 =C2=A0 So now, there is no place I can place this handler, and I am = now at a dead end.

I still feel that this should go to phosphor-ipmi-blobs, you can create a s= eparate
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.=

=C2=A0 =C2=A0 I need to find a way out and would greatly appreciate it if w= e can
=C2=A0 =C2=A0 reach a consensus here so that BMC health monitoring can move= forward.

=C2=A0 =C2=A0 Thanks!