Request to create repository google-ipmi-bmc-health

[Vijay Khemka]

On 11/5/20, 3:55 PM, "Sui Chen" <suichen at google.com> wrote:

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


    Hello Patrick and Vijay,

    As far as I know, the only two "google-ipmi-*" repositories are 1)
    google-ipmi-sys and 2) google-ipmi-i2c, and neither seem to be related
    to the health monitoring task we're doing right now.
    In my understanding one similar library is phosphor-pid-control; its
    IPMI handler is also in the repository rather than in a separate
    repository.

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

    I am facing a confusing situation as to where I should put the IPMI
    blob handler, due to conflicting opinions:
    1. The maintainers of phosphor-ipmi-blobs told me it's not desirable
    to put all IPMI blob handlers into the same place.
    2. By reading this email thread, I had the impression that it's not a
    good idea to create too many repositories either.
    3. Because of #1 and #2, I felt we should put the IPMI blob handler
    into phosphor-health-monitor itself, just like phosphor-pid-control
    does.
    4. In the last reply from Patrick it sounds it's a bad idea to put the
    IPMI blob handler into phosphor-health-monitor because of IPMI details
    leaking out into non-IPMI repositories.
    5. Vijay seemed to prefer to have all IPMI blob handlers in one place
    based on our discussion on IRC. However, according to #1 this is going
    to face pushback. As such, I created all my changes in
    phosphor-health-monitor for review and for showing my intent on how
    the IPMI implementation is done.
    6. Because of #4 and #5, it sounds like I can't put the IPMI blob handler into
    phosphor-health-monitor either.
    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 separate
directory (handler) under the same repo and it can become home for all the
futures blob handler as these are going to interact with ipmi blobs anyway.

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

    Thanks!