Request to create repository google-ipmi-bmc-health

Request to create repository google-ipmi-bmc-health

[Sui Chen]

Hello OpenBMC community,

We are working on an IPMI blob-based implementation of BMC health
monitoring. We currently have an internal working prototype version
and would like to upload it to this newly proposed repository,
openbmc/google-ipmi-bmc-health .

We are aware of existing BMC health monitoring designs such as:
1. https://github.com/openbmc/phosphor-health-monitor and its
documentation https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/31957
2. https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/34766

Main differences between this implementation and existing ones are:
- google-ipmi-bmc-health is implemented with the IPMI blob handler
framework and exists as an IPMI blob handler, while
phosphor-health-monitor runs as a daemon and exposes BMC health
metrics on DBus in the same manner sensors are exposed.
- This implementation does not check health metric values against
thresholds or perform actions when thresholds are crossed.
- This implementation currently reports uptime, memory usage, free
disk space, CPU time consumed by processes, and file descriptor stats.
- This implementation does not read a configuration file yet. It
always reads the hard-coded set of health metrics listed above.
- This implementation does not post-process sensor readings such as
compute the average CPU usage over a certain time window.

As such, this implementation differs enough from existing ones such
that we believe we have enough reasons to have a separate repository
for it.

Thanks!

Re: Request to create repository google-ipmi-bmc-health

[Vijay Khemka]

Hi Sui,

On 9/30/20, 8:30 AM, "openbmc on behalf of Sui Chen" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of suichen@google.com> wrote:

    Hello OpenBMC community,

    We are working on an IPMI blob-based implementation of BMC health
    monitoring. We currently have an internal working prototype version
    and would like to upload it to this newly proposed repository,
    openbmc/google-ipmi-bmc-health .

In my opinion, we can enhance existing health-monitor and add your features.

    We are aware of existing BMC health monitoring designs such as:
    1. https://github.com/openbmc/phosphor-health-monitor and its
    documentation https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_31957&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=HTKEM8tcIgwzwL4OQVP1Kcve6ZfnhSTohdwPmIrjwe4&e= 
    2. https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_34766&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=EcxSrU1PC6Akfy1FR0wo-5TC_QvMld9SDT7pJAh5QcM&e= 

    Main differences between this implementation and existing ones are:
    - google-ipmi-bmc-health is implemented with the IPMI blob handler
    framework and exists as an IPMI blob handler, while
    phosphor-health-monitor runs as a daemon and exposes BMC health
    metrics on DBus in the same manner sensors are exposed.

Is this going to be a library or daemon, Same health-monitor daemon can 
Be enhanced to add these functionalities.

    - This implementation does not check health metric values against
    thresholds or perform actions when thresholds are crossed.

If you don't define threshold in configuration file, health-monitor will
also not monitor metrics defined.

    - This implementation currently reports uptime, memory usage, free
    disk space, CPU time consumed by processes, and file descriptor stats.

Same can be added as extra metrics. That was the goal of this repo as to
start with basic metrics and add more as required.

    - This implementation does not read a configuration file yet. It
    always reads the hard-coded set of health metrics listed above.

We can enable or disable certain metrics through this configuration file.

    - This implementation does not post-process sensor readings such as
    compute the average CPU usage over a certain time window.

Window size 1 can give latest data rather than averaged data.

    As such, this implementation differs enough from existing ones such
    that we believe we have enough reasons to have a separate repository
    for it.

I will strongly prefer to add all of the features in the existing repo.

    Thanks!

Re: Request to create repository google-ipmi-bmc-health

[Sui Chen]

Hi Vijay,

We can use whatever means that gets health monitoring done.
I have the following questions on how to merge the proposed IPMI
Blob-based implementation, google-ipmi-bmc-health (referred to as
"IPMI health blob") with phosphor-health-monitor. The intent of having
a separate "google-ipmi-bmc-health" was to avoid these questions:

1) The IPMI health blob is a library, not a daemon, so after the IPMI
health blob is added, phosphor-health-monitor will have both a library
and a daemon. The user needs to have a way to configure it. What is
the recommended way of doing this configuration?

2) We are sending a protocol buffer through the IPMI interface to the
BMC, and the protocol buffer may be only used for the IPMI path and
not anywhere else. Would there be any concerns on the usage of a
protocol buffer here?

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.

Thanks,
Sui

On Thu, Oct 1, 2020 at 12:06 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
>
> Hi Sui,
>
> On 9/30/20, 8:30 AM, "openbmc on behalf of Sui Chen" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of suichen@google.com> wrote:
>
>     Hello OpenBMC community,
>
>     We are working on an IPMI blob-based implementation of BMC health
>     monitoring. We currently have an internal working prototype version
>     and would like to upload it to this newly proposed repository,
>     openbmc/google-ipmi-bmc-health .
>
> In my opinion, we can enhance existing health-monitor and add your features.
>
>     We are aware of existing BMC health monitoring designs such as:
>     1. https://github.com/openbmc/phosphor-health-monitor and its
>     documentation https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_31957&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=HTKEM8tcIgwzwL4OQVP1Kcve6ZfnhSTohdwPmIrjwe4&e=
>     2. https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_34766&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=EcxSrU1PC6Akfy1FR0wo-5TC_QvMld9SDT7pJAh5QcM&e=
>
>     Main differences between this implementation and existing ones are:
>     - google-ipmi-bmc-health is implemented with the IPMI blob handler
>     framework and exists as an IPMI blob handler, while
>     phosphor-health-monitor runs as a daemon and exposes BMC health
>     metrics on DBus in the same manner sensors are exposed.
>
> Is this going to be a library or daemon, Same health-monitor daemon can
> Be enhanced to add these functionalities.
>
>     - This implementation does not check health metric values against
>     thresholds or perform actions when thresholds are crossed.
>
> If you don't define threshold in configuration file, health-monitor will
> also not monitor metrics defined.
>
>     - This implementation currently reports uptime, memory usage, free
>     disk space, CPU time consumed by processes, and file descriptor stats.
>
> Same can be added as extra metrics. That was the goal of this repo as to
> start with basic metrics and add more as required.
>
>     - This implementation does not read a configuration file yet. It
>     always reads the hard-coded set of health metrics listed above.
>
> We can enable or disable certain metrics through this configuration file.
>
>     - This implementation does not post-process sensor readings such as
>     compute the average CPU usage over a certain time window.
>
> Window size 1 can give latest data rather than averaged data.
>
>     As such, this implementation differs enough from existing ones such
>     that we believe we have enough reasons to have a separate repository
>     for it.
>
> I will strongly prefer to add all of the features in the existing repo.
>
>     Thanks!
>

Re: Request to create repository google-ipmi-bmc-health

[Vijay Khemka]

Hi Sui,

On 10/1/20, 6:52 PM, "Sui Chen" <suichen@google.com> wrote:

    Hi Vijay,

    We can use whatever means that gets health monitoring done.
    I have the following questions on how to merge the proposed IPMI
    Blob-based implementation, google-ipmi-bmc-health (referred to as
    "IPMI health blob") with phosphor-health-monitor. The intent of having
    a separate "google-ipmi-bmc-health" was to avoid these questions:

    1) The IPMI health blob is a library, not a daemon, so after the IPMI
    health blob is added, phosphor-health-monitor will have both a library
    and a daemon. The user needs to have a way to configure it. What is
    the recommended way of doing this configuration?

Yes the same repo can generate library as well as daemon. Currently it is
configuring 2 metrics cpu and memory, we can add another entry like
IPMI blob and if it is there then only it will build ipmi blobs.

    2) We are sending a protocol buffer through the IPMI interface to the
    BMC, and the protocol buffer may be only used for the IPMI path and
    not anywhere else. Would there be any concerns on the usage of a
    protocol buffer here?

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. 

    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

    Thanks,
    Sui

    On Thu, Oct 1, 2020 at 12:06 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
    >
    > Hi Sui,
    >
    > On 9/30/20, 8:30 AM, "openbmc on behalf of Sui Chen" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of suichen@google.com> wrote:
    >
    >     Hello OpenBMC community,
    >
    >     We are working on an IPMI blob-based implementation of BMC health
    >     monitoring. We currently have an internal working prototype version
    >     and would like to upload it to this newly proposed repository,
    >     openbmc/google-ipmi-bmc-health .
    >
    > In my opinion, we can enhance existing health-monitor and add your features.
    >
    >     We are aware of existing BMC health monitoring designs such as:
    >     1. https://github.com/openbmc/phosphor-health-monitor and its
    >     documentation https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_31957&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=HTKEM8tcIgwzwL4OQVP1Kcve6ZfnhSTohdwPmIrjwe4&e=
    >     2. https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_docs_-2B_34766&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=Z-_Rsue1ZHBD_TgPw7EDIc8dh8E8o8dlUe8aKr7I5VA&s=EcxSrU1PC6Akfy1FR0wo-5TC_QvMld9SDT7pJAh5QcM&e=
    >
    >     Main differences between this implementation and existing ones are:
    >     - google-ipmi-bmc-health is implemented with the IPMI blob handler
    >     framework and exists as an IPMI blob handler, while
    >     phosphor-health-monitor runs as a daemon and exposes BMC health
    >     metrics on DBus in the same manner sensors are exposed.
    >
    > Is this going to be a library or daemon, Same health-monitor daemon can
    > Be enhanced to add these functionalities.
    >
    >     - This implementation does not check health metric values against
    >     thresholds or perform actions when thresholds are crossed.
    >
    > If you don't define threshold in configuration file, health-monitor will
    > also not monitor metrics defined.
    >
    >     - This implementation currently reports uptime, memory usage, free
    >     disk space, CPU time consumed by processes, and file descriptor stats.
    >
    > Same can be added as extra metrics. That was the goal of this repo as to
    > start with basic metrics and add more as required.
    >
    >     - This implementation does not read a configuration file yet. It
    >     always reads the hard-coded set of health metrics listed above.
    >
    > We can enable or disable certain metrics through this configuration file.
    >
    >     - This implementation does not post-process sensor readings such as
    >     compute the average CPU usage over a certain time window.
    >
    > Window size 1 can give latest data rather than averaged data.
    >
    >     As such, this implementation differs enough from existing ones such
    >     that we believe we have enough reasons to have a separate repository
    >     for it.
    >
    > I will strongly prefer to add all of the features in the existing repo.
    >
    >     Thanks!
    >

Re: Request to create repository google-ipmi-bmc-health

[Sui Chen]

On Fri, Oct 2, 2020 at 1:54 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
>
> Hi Sui,
>
> On 10/1/20, 6:52 PM, "Sui Chen" <suichen@google.com> wrote:
>
>     Hi Vijay,
>
>     We can use whatever means that gets health monitoring done.
>     I have the following questions on how to merge the proposed IPMI
>     Blob-based implementation, google-ipmi-bmc-health (referred to as
>     "IPMI health blob") with phosphor-health-monitor. The intent of having
>     a separate "google-ipmi-bmc-health" was to avoid these questions:
>
>     1) The IPMI health blob is a library, not a daemon, so after the IPMI
>     health blob is added, phosphor-health-monitor will have both a library
>     and a daemon. The user needs to have a way to configure it. What is
>     the recommended way of doing this configuration?
>
> Yes the same repo can generate library as well as daemon. Currently it is
> configuring 2 metrics cpu and memory, we can add another entry like
> IPMI blob and if it is there then only it will build ipmi blobs.
>
>     2) We are sending a protocol buffer through the IPMI interface to the
>     BMC, and the protocol buffer may be only used for the IPMI path and
>     not anywhere else. Would there be any concerns on the usage of a
>     protocol buffer here?
>
> 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.

>
>     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!

Re: Request to create repository google-ipmi-bmc-health

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2515 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Request to create repository google-ipmi-bmc-health

[Sui Chen]

On Tue, Oct 6, 2020 at 6:43 PM Patrick Williams <patrick@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@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 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!

Re: Request to create repository google-ipmi-bmc-health

[Vijay Khemka]

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

    On Tue, Oct 6, 2020 at 6:43 PM Patrick Williams <patrick@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@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!

Re: Request to create repository google-ipmi-bmc-health

[William Kennington]

[-- Attachment #1: Type: text/plain, Size: 6857 bytes --]

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.

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

>
>
> On 11/5/20, 3:55 PM, "Sui Chen" <suichen@google.com> wrote:
>
>     On Tue, Oct 6, 2020 at 6:43 PM Patrick Williams <patrick@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@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!
>
>

[-- Attachment #2: Type: text/html, Size: 8509 bytes --]

Re: Request to create repository google-ipmi-bmc-health

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

On Tue, Nov 10, 2020 at 10:38:55PM -0800, William Kennington wrote:
> 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.

Thanks William for the background on phosphor-ipmi-blobs intentions.

> On Tue, Nov 10, 2020 at 10:35 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
> > <11/5/20, 3:55 PM, "Sui Chen" <suichen@google.com> wrote:
> >     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?

Sui,

Now that the design has been separated so that the majority of the
metric implementation is in p-h-m and the protobuf-ipmi-specific parts
just do light-weight dbus operations, it seems reasonable to me to
create a new repository to hold that part.  That part seems fairly
unique to what Google intends to do and I don't think we should burden
the maintainers of another repository with that effort.

I don't have a strong opinion on the IPMI blob handlers being all in one
vs spread out in individual repositories, as long as those repositories
are light-weight translations from the dbus APIs to the specific IPMI
blob format.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Request to create repository google-ipmi-bmc-health

[Sui Chen]

On Wed, Nov 11, 2020 at 4:14 AM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Tue, Nov 10, 2020 at 10:38:55PM -0800, William Kennington wrote:
> > 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.
>
> Thanks William for the background on phosphor-ipmi-blobs intentions.
>
> > On Tue, Nov 10, 2020 at 10:35 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
> > > <11/5/20, 3:55 PM, "Sui Chen" <suichen@google.com> wrote:
> > >     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?
>
> Sui,
>
> Now that the design has been separated so that the majority of the
> metric implementation is in p-h-m and the protobuf-ipmi-specific parts
> just do light-weight dbus operations, it seems reasonable to me to
> create a new repository to hold that part.  That part seems fairly
> unique to what Google intends to do and I don't think we should burden
> the maintainers of another repository with that effort.
>
> I don't have a strong opinion on the IPMI blob handlers being all in one
> vs spread out in individual repositories, as long as those repositories
> are light-weight translations from the dbus APIs to the specific IPMI
> blob format.
>
> --
> Patrick Williams

Hello Patrick,

Thanks for your understanding for our request to create a new repository.

Our team had also met last Friday for a discussion on where the
implementation of the blob handler should go, and we also agreed it is
preferable to create a new repository compared to putting its
implementation in phosphor-health-monitor or phosphor-ipmi-blobs.

Now that the IPMI blob handler lives in its own separate repo, it
seems to me that the design does not have to be separated right now;
the new repo could, for now, hold the monolithic IPMI blob handler
where the metric implementation is entirely in the handler.

In the meantime, we will continue to work on the separated design
where the blob handler does light-weight dbus operations against the
daemon, starting from addressing the comments. This might take some
time but we are invested in its design proposal and we are determined
to finish implementing it.

If this plan sounds reasonable, can we request to create the
repository now? If the word "health" in the name is a concern, how
about "google-ipmi-bmc-metrics"?

Thanks!
Sui

Re: Request to create repository google-ipmi-bmc-health

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Mon, Nov 16, 2020 at 04:00:47PM -0800, Sui Chen wrote:
> On Wed, Nov 11, 2020 at 4:14 AM Patrick Williams <patrick@stwcx.xyz> wrote:
> > Sui,
> >
> > Now that the design has been separated so that the majority of the
> > metric implementation is in p-h-m and the protobuf-ipmi-specific parts
> > just do light-weight dbus operations, it seems reasonable to me to
> > create a new repository to hold that part.  That part seems fairly
> > unique to what Google intends to do and I don't think we should burden
> > the maintainers of another repository with that effort.
> 
> Our team had also met last Friday for a discussion on where the
> implementation of the blob handler should go, and we also agreed it is
> preferable to create a new repository compared to putting its
> implementation in phosphor-health-monitor or phosphor-ipmi-blobs.
> 
> Now that the IPMI blob handler lives in its own separate repo, it
> seems to me that the design does not have to be separated right now;
> the new repo could, for now, hold the monolithic IPMI blob handler
> where the metric implementation is entirely in the handler.

I don't really agree with going this direction if I understand
correctly.  We started this discussion because people felt there were
bits that were useful to others in a more generic repository and bits
that were only useful to Google.  Now that we've come to agreement that
the Google-bits belong in a separate repository, why would we go down
the path that all the bits belong in a separate repository where nobody
else can usefully interact with them?

Given some of the code review comments I left in the
phosphor-health-monitor proposal, I'm not sure we've really come to a
consensus on how metrics like this should be handled architecturally.
If you continue doing the Google-specific parts, I think it is going to
be difficult to unravel the design into something that can be globally
applicable.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Request to create repository google-ipmi-bmc-health

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

>
> Hello Patrick,
>
> Thanks for your understanding for our request to create a new repository.
>
> Our team had also met last Friday for a discussion on where the
> implementation of the blob handler should go, and we also agreed it is
> preferable to create a new repository compared to putting its
> implementation in phosphor-health-monitor or phosphor-ipmi-blobs.
>
> Now that the IPMI blob handler lives in its own separate repo, it
> seems to me that the design does not have to be separated right now;
> the new repo could, for now, hold the monolithic IPMI blob handler
> where the metric implementation is entirely in the handler.
>

I completely disagree with this approach of having a platform specific
implementation, I will still prefer to have a generic design in health
monitor
for metrics collection and blob handler can be in a separate repo.


> In the meantime, we will continue to work on the separated design
> where the blob handler does light-weight dbus operations against the
> daemon, starting from addressing the comments. This might take some
> time but we are invested in its design proposal and we are determined
> to finish implementing it.
>
> If this plan sounds reasonable, can we request to create the
> repository now? If the word "health" in the name is a concern, how
> about "google-ipmi-bmc-metrics"?
>
> Thanks!
> Sui
>

[-- Attachment #2: Type: text/html, Size: 1884 bytes --]

Re: Request to create repository google-ipmi-bmc-health

[Sui Chen]

On Wed, Nov 18, 2020 at 12:48 AM Vijay Khemka
<vijaykhemkalinux@gmail.com> wrote:
>
>
>
>>
>>
>> Hello Patrick,
>>
>> Thanks for your understanding for our request to create a new repository.
>>
>> Our team had also met last Friday for a discussion on where the
>> implementation of the blob handler should go, and we also agreed it is
>> preferable to create a new repository compared to putting its
>> implementation in phosphor-health-monitor or phosphor-ipmi-blobs.
>>
>> Now that the IPMI blob handler lives in its own separate repo, it
>> seems to me that the design does not have to be separated right now;
>> the new repo could, for now, hold the monolithic IPMI blob handler
>> where the metric implementation is entirely in the handler.
>
>
> I completely disagree with this approach of having a platform specific
> implementation, I will still prefer to have a generic design in health monitor
> for metrics collection and blob handler can be in a separate repo.
>

Thanks for the explanation, we will keep the platform-specific
implementation (blob handler doing everything) down-stream, while
working on the generic design (blob handler only translating DBus
messages to IPMI blob) upstream in the meantime.

I think we can focus on working out how the added metrics should be
made available on DBus in phosphor-health-monitor; this has to be
resolved no matter where the blob handler lives.

Sui