openbmc / technical-oversight-forum

5 stars 1 forks source link

Request to create a new repository for libpldm #16

Closed manojkiraneda closed 1 year ago

manojkiraneda commented 2 years ago

Hello Everyone,

This is a request for creating a new repository called libpldm, so that i can migrate the code from existing openbmc/pldm/libpldm to openbmc/libpldm

libpldm is a shared libary which is meant to be dealing with encoding, decoding support for various PLDM messages & also utility functions around the PLDM structures that can be used for other PLDM stacks . It does not depend on any openbmc specific dependencies ,so can be used by projects outside openbmc.

Moving this out of pldm repository would :

dkodihal commented 2 years ago

It would be good to elaborate on the complexities seen today with this being within the pldm repository.

williamspatrick commented 2 years ago

I would like to see a clear API definition (based on the present art) before we go this route. In general, we weren't suppose to have libraries (like libpldm) and this was created already without any kind of design presentation for approval.

A few example questions that come to my mind:

Who are going to be the maintainers of this new repository? How can we ensure fast response time to contributors outside of the core developer set?

I have a team that has been trying to propose extensions to the current "libpldm" implementation in order to be able to support additional transport layers, because the current implementation is tightly coupled with a particular MCTP API I believe, and they have had proposals since October. It has been very very slow to get any meaningful feedback on this proposal and we are 9 months in.

I'm not really inclined to give the PLDM-centric developers yet another repository as a playground for their own development until the maintainer / feedback situation improves and we have an adequate design discussion.

manojkiraneda commented 2 years ago

I would like to see a clear API definition (based on the present art) before we go this route. In general, we weren't suppose to have libraries (like libpldm) and this was created already without any kind of design presentation for approval.

I was not aware till now that this was added without any kind of approval, I recently started contributing to pldm repository and learnt that libpldm is meant to be helping hand for other pldm stacks (like openpower/hostboot, power hypervisor, skiboot) which are the remote endpoints to which bmc pldm stack on IBM would talk to.Probably other companies like intel, nvidia have similar stacks that consume this . Probably @dkodihal is the best person to answer why we had to choose this path .

A few example questions that come to my mind:

* What is the API?

I am not sure i understand the question, are you asking what is that we are exposing via the libpldm ? we expose unit tested encode, decode functions of all the pldm commands & wrapper functions to connect [to mctp socket], send & receive PLDM messages.and also util functions around the pldm structures like platform descriptor records.

* Who are the intended users outside of `openbmc/pldm`?  Does it have any utility within rest of the OpenBMC ecosystem?

Any pldm stack outside openbmc can leverage the libpldm library functions to encode, decode messages & send/receive messages. I may not be an expert in this area ,but i think as of the moment open-power/hostboot and open-power/skiboot are already using the libpldm library.

Although as per the design principle this library is not meant to be used inside openbmc , but i do see certain apps like dump, occ, pel code use that(again i dont know the reason nor i am the best person to answer why we had to choose that path).

* What are the environments we are targeting outside of the OpenBMC ecosystem?  How are we going to test utility in those environments?  (ie. who are these "people outside openbmc" and how do we know we satisfy their needs?)

Except the names of projects that i listed above, I have a very limited knowledge on what environments that these external projects use. Atleast my intention was to make it separate and easy for outsiders to consume and integrate into their project(i do feel that having that in pldm repo will be challenging for outsiders to consume due to the special meson feature flags like -Dbuild-libpldm-only and lack of it not being a meson subproject)

Who are going to be the maintainers of this new repository? How can we ensure fast response time to contributors outside of the core developer set?

I can work on doing the separation(if approved) and fix up any mess this creates but I don't have any intention of being a maintainer of this repository[as i dont think i have enough contributions to be a maintainer], I was added as a reviewer for pldm repository recently & i have been actively reviewing code since then.If maintainers don't have any problem i can copy the same maintainers & reviewers to this new repository as well.

Regarding the response time - i can only speak for myself in this - I am on top of most of the code reviews , and i would certainly try my best to be on top of reviews.

I have a team that has been trying to propose extensions to the current "libpldm" implementation in order to be able to support additional transport layers, because the current implementation is tightly coupled with a particular MCTP API I believe, and they have had proposals since October. It has been very very slow to get any meaningful feedback on this proposal and we are 9 months in.

I started to understand pldm only recently and i am still learning and trying to help others to learn what i know , and i don't think i am in a stage to give any design suggestions to anyone as MCTP and various transports are much bigger problems. I added myself to these reviews to understand and learn about the new requirements that are coming in . But beyond that i am not sure if i can give any valuable suggestion that could steer the designs.[I agree i am not active in giving feedback on the design documents]. Probably this would need an answer from the maintainers as well.

I'm not really inclined to give the PLDM-centric developers yet another repository as a playground for their own development until the maintainer / feedback situation improves and we have an adequate design discussion.

I can only speak for myself in this - I will try my best to be on top of reviews. Sure patrick , i am okay with what ever the tof forum decides.

manojkiraneda commented 2 years ago

It would be good to elaborate on the complexities seen today with this being within the pldm repository.

The problem that i have in mind is the lack of having this as a subproject, there by making this problematic to integrate this with other projects. And also my learning in the past from phosphor-logging is that having special meson options(-Dlibonly kind) are always pain to maintain , and it is much easier to have this as a general dependency/meson subproject in long run.

amboar commented 2 years ago

I would like to see a clear API definition (based on the present art) before we go this route.

It's not clear exactly what should be presented to satisfy this? A header file? A UML diagram? Some prose?

In general, we weren't suppose to have libraries (like libpldm) and this was created already without any kind of design presentation for approval.

I don't understand this position really. Arguably most function should be implemented in libraries and then we add applications developed on top. However, setting this aside for now.

Who are going to be the maintainers of this new repository? How can we ensure fast response time to contributors outside of the core developer set?

I can try to help out with maintenance, though I'm feeling pretty snowed under at the moment, which won't help with ...

I have a team that has been trying to propose extensions to the current "libpldm" implementation in order to be able to support additional transport layers, because the current implementation is tightly coupled with a particular MCTP API I believe, and they have had proposals since October. It has been very very slow to get any meaningful feedback on this proposal and we are 9 months in.

... this problem.

I'm not really inclined to give the PLDM-centric developers yet another repository as a playground for their own development until the maintainer / feedback situation improves and we have an adequate design discussion.

+1.

However, overall, I am in favour of splitting out the library to help with maintainability.

edtanous commented 2 years ago

I'm overall in favor of splitting this out, and I suspect the question of the API itself can be answered via patchsets implementing the behavior required. I would expect the target for this new library would be "any system trying to implement pldm" Maybe that's not the case?

If there's a problem with maintainer response times, lets get that understood outside the context of this repo request. Given that the maintainer list between the existing solution, and the separate libpldm repo is similar, and the code is the same, it seems likely that moving this code to a separate repo makes this no worse, and possibly better if we can get interested parties outside of OpenBMC interested in maintaining code and reviewing.

edtanous commented 2 years ago

Sorry, hit the wrong button. Didn’t mean to close.

manojkiraneda commented 2 years ago

I would expect the target for this new library would be "any system trying to implement pldm" Maybe that's not the case?

That should be the case ideally, but right now few apps like debug-collector , logging are using this library too - i think we have issues opened on those repo's as well [ i am not sure about why we choose to go that path, probably that could be fixed if we have a better dbus API's in place ] , that's some thing that i have in mind to fix too - but one thing at a time, wanna start with splitting this out first.

edtanous commented 2 years ago

I would expect the target for this new library would be "any system trying to implement pldm" Maybe that's not the case?

That should be the case ideally, but right now few apps like debug-collector , logging are using this library too - i think we have issues opened on those repo's as well [ i am not sure about why we choose to go that path, probably that could be fixed if we have a better dbus API's in place ] , that's some thing that i have in mind to fix too - but one thing at a time, wanna start with splitting this out first.

That's a little worrisome. Can we get some input from the maintainers of those repositories on why those are needed? I wouldn't expect debug collector to take ANY dependency directly on PLDM the protocol, and would expect them to be communicating via dbus.

manojkiraneda commented 2 years ago

I would expect the target for this new library would be "any system trying to implement pldm" Maybe that's not the case?

That should be the case ideally, but right now few apps like debug-collector , logging are using this library too - i think we have issues opened on those repo's as well [ i am not sure about why we choose to go that path, probably that could be fixed if we have a better dbus API's in place ] , that's some thing that i have in mind to fix too - but one thing at a time, wanna start with splitting this out first.

That's a little worrisome. Can we get some input from the maintainers of those repositories on why those are needed? I wouldn't expect debug collector to take ANY dependency directly on PLDM the protocol, and would expect them to be communicating via dbus.

for debug-collector - i see an issue which was closed https://github.com/openbmc/openbmc/issues/3749, tagging @dhruvibm for debug collector & @spinler for logging.

williamspatrick commented 1 year ago

This issue was discussed by the TOF and the following direction was agreed to:

williamspatrick commented 1 year ago

Outstanding actions to close out this issue:

tomjoseph83 commented 1 year ago

If the intent of the proposal was to have this as a general purpose library and not specific to OpenBMC, did TOF consider moving this to DMTF org?

SPDM is another DMTF protocol. https://github.com/DMTF/libspdm

amboar commented 1 year ago

If the intent of the proposal was to have this as a general purpose library and not specific to OpenBMC, did TOF consider moving this to DMTF org?

This can be considered at another time. Let's first close on what we set out to achieve here.

amboar commented 1 year ago
* @amboar and @williamspatrick will act as initial maintainers with @edtanous, @bradbishop, and @jmbills being listed as reviewers in the `OWNERS` file.  @amboar and @jmbills are encouraged to find (at least) an individual from each of their companies to be mentored into taking over maintainership in a 3-6 month timeframe.

I intend to work with @manojkiraneda on this :slightly_smiling_face:

edtanous commented 1 year ago

If the intent of the proposal was to have this as a general purpose library and not specific to OpenBMC, did TOF consider moving this to DMTF org?

This can be considered at another time. Let's first close on what we set out to achieve here.

+1. If this were new code, it might be something to consider, but until it's abstracted in a way that makes it not specific to openbmc, I suspect there aren't many interested in it in its current state, and once that has happened, we can certainly engage DMTF to go add it at a later date.

tomjoseph83 commented 1 year ago

I see there are no technical objections to propose it to DMTF, other than the logistics.

but until it's abstracted in a way that makes it not specific to openbmc, I suspect there aren't many interested in it in its current state, and once that has happened,

I am surprised to hear that this is not generic enough, there are at least a handful of projects already consuming it. The only OpenBMC reference in libpldm is https://github.com/openbmc/pldm/tree/master/libpldm/requester which is used by a bunch of IBM specific apps, that too controlled by a flag.

edtanous commented 1 year ago

I am surprised to hear that this is not generic enough

To be more clear, the folder, as it exists in another repo is not generic enough because it's a folder within another repo. Once it's extracted, it very well might be generic enough, and someone can start the submission process for DMTF if they want. When and if that happens doesn't really have any bearing on this request for a repo, which I'm still of the opinion should be granted, with the last proposal above.

williamspatrick commented 1 year ago

I am surprised to hear that this is not generic enough, there are at least a handful of projects already consuming it.

Are these projects other repos within the openbmc org or elsewhere? If you are aware of these it would be good to have them listed so we can communicate to them the pending changes as well.

tomjoseph83 commented 1 year ago

To be more clear, the folder, as it exists in another repo is not generic enough because it's a folder within another repo. Once it's extracted, it very well might be generic enough, and someone can start the submission process for DMTF if they want. When and if that happens doesn't really have any bearing on this request for a repo, which I'm still of the opinion should be granted, with the last proposal above.

+1, thanks for clarifying.

Are these projects other repos within the openbmc org or elsewhere? If you are aware of these it would be good to have them listed so we can communicate to them the pending changes as well.

https://github.com/open-power/hostboot https://github.com/open-power/skiboot

These are the open ones, there are other internal projects as well.

bradbishop commented 1 year ago

I finally created the repository today. @amboar you are up.

@amboar prime history

amboar commented 1 year ago

Due to what we had to do to extract libpldm from the pldm repo the history is (unfortunately) minimal. However, I've primed the libpldm repo in gerrit and pushed the initial content to github directly in lieu of automated replication. I've requested the replication be setup. The content includes the initial OWNERS file as outlined by the comment from @williamspatrick above. Additionally, I've asked @manojkiraneda to add himself to reviewers

manojkiraneda commented 1 year ago

libpldm migration is completed, thank you everyone for all the help & support.