search

openbmc / bmcweb

A do everything Redfish, KVM, GUI, and DBus webserver for OpenBMC
Apache License 2.0
154 stars 131 forks source link

Dead virtual media code in bmcweb #188

Closed edtanous closed 1 month ago

edtanous commented 3 years ago

The nbd proxy implementation present within bmcweb never had it's backend upstreamed. As such, this is dead code to us, and could be removed.

Code in question is here: https://github.com/openbmc/bmcweb/blob/master/include/nbd_proxy.hpp

And I suspect could be completely removed with no impact to the openbmc project.

gtmills commented 3 years ago

https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/40766 disables this code

Kitsok commented 3 years ago

Just to confirm that this code is used by us (Yandex).

gtmills commented 3 years ago

Just to confirm that this code is used by us (Yandex).

Can you help upstream the backend? Because without an upstreamed backend we will eventually remove from bmcweb.

Kitsok commented 3 years ago

Just to confirm that this code is used by us (Yandex).

Can you help upstream the backend? Because without an upstreamed backend we will eventually remove from bmcweb.

It's Intel code (while in public repository), I've asked them to upstream.

edtanous commented 3 years ago

Just to confirm that this code is used by us (Yandex).

Can you help upstream the backend? Because without an upstreamed backend we will eventually remove from bmcweb.

It's Intel code (while in public repository), I've asked them to upstream.

Please ask them to start by writing a design doc; There's already a virtual media implementation, so we need to understand why this one is better, and needs to diverge from the already upstreamed one. I'm sure there are good reasons, but we need to get those written down.

Kitsok commented 3 years ago

There's already a virtual media implementation

I think Intel implementation fits well to what is described here as legacy mode: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md I couldn't find other implementation in OpenBMC repository, could you please point its' location?

edtanous commented 3 years ago

https://github.com/openbmc/bmcweb/blob/90e97e1d26b78d899a543831a8051dacbbdde71a/meson_options.txt#L5

edtanous commented 3 years ago

Several months after disabling this, and no progress on upstreaming the backend or a design doc. If no one is interested in this code, it would help the complexity to delete it, which I'm currently intending on doing. At such time as a backend is upstreamed, we can open a new patchset to re-add the feature.

edtanous commented 1 year ago

We're now several years into disabling this, and some progress has been made on getting the backend upstreamed, but not much.

Recent changes have gone in that make me far less worried about the impact of changes in that module, and to improve the reliability, but it still has no backend.

NguyenTanNhutQuang commented 1 year ago

@edtanous and @Kitsok, I try to understand the whole picture here. Virtural media has two modes, legacy and proxy mode. There are several components for each mode to enable them to work. Legacy mode components:

Kitsok commented 1 year ago

So, what is the backend of proxy mode as you mentioned?

Not sure if this answers your question, but... We use two modes of operation of VM:

edtanous commented 1 year ago

I wasn't a part of the discussion on virtual media when the bmcweb backend was merged. You should likely look to the people that pushed and/or approved it.

NguyenTanNhutQuang commented 11 months ago

I found that these two commits were added to enable BMCWEB_ENABLE_VM_NBDPROXY https://github.com/openbmc/bmcweb/commit/107077def176ad4a29557fae353de9bb00381ca9 https://github.com/openbmc/bmcweb/commit/c0a1c8a0ecc55aef54e6f44ea89a4dd232e265a2

But the build flag BMCWEB_ENABLE_VM_NBDPROXY is a bit missleading since the code of these commits are for implementing Redfish schema of virtual media or the front end of the Redfish virtual media. The backend of these code are implemented by Intel at this repo https://github.com/Intel-BMC/virtual-media which is discontinued.

Now, if we remove the flag BMCWEB_ENABLE_VM_NBDPROXY and its related code (the two commits mentioned above), the feature virtual media via redfish will not work anymore.

I wonder if nobody is using Redfish virtual media?

Kitsok commented 11 months ago
edtanous commented 11 months ago

So, what is the backend of proxy mode as you mentioned?

I missed this question the first time around. The virtual media service that you mention in your list. The other things in the list are already well-maintained open source projects. As you note, Intels project is completely discontinued and unmaintained, and given the lack of interest in getting it upstreamed and maintained tells me that we can remove this code from bmcweb.

We use it in production .... I'll have to include it back using local patches.

If that's the case, it would help a lot if you could please volunteer in getting it reviewed, tested, coded in line with the coding standard, and getting a maintainer that's interested in it added to the reviewers. As-is, it is dead, untestable and unmaintainable code that I'm not sure if works. Patches like this https://gerrit.openbmc.org/c/openbmc/bmcweb/+/66298 get very few reviewers, and no test results, which doesn't seem sustainable. With that said, apparently it works in your environment, so clearly it's of some use.

the feature virtual media via redfish will not work anymore

The feature never worked in an upstream build. As you say, it only worked in the Intel-BMC fork.

NguyenTanNhutQuang commented 11 months ago

The nbd proxy implementation present within bmcweb never had it's backend upstreamed. As such, this is dead code to us, and could be removed.

Code in question is here: https://github.com/openbmc/bmcweb/blob/master/include/nbd_proxy.hpp

And I suspect could be completely removed with no impact to the openbmc project.

@edtanous I misunderstand your first comment, you mean that the code in nbd_proxy.hpp will be removed only, is it?

thangqn-ampere commented 11 months ago

I wonder if we really need to have backend application. As I understand, the InsertMedia action has information needed for the ISO file from SMB or NFS. Then, we just need to mount the ISO file to the RootFS (not sure about current mount point now) and then execute the /etc/nbd-proxy/state script (stored at meta-phosphor/recipes-connectivity/jsnbd/jsnbd/state_hook) with start option to create virtual USB device, similar to the WebUI virtual-media.

thangqn-ampere commented 11 months ago

I wonder if we really need to have backend application. As I understand, the InsertMedia action has information needed for the ISO file from SMB or NFS. Then, we just need to mount the ISO file to the RootFS (not sure about current mount point now) and then execute the /etc/nbd-proxy/state script (stored at meta-phosphor/recipes-connectivity/jsnbd/jsnbd/state_hook) with start option to create virtual USB device, similar to the WebUI virtual-media.

Let ignore my comment. This issue discusses about proxy mode and my comment is for legacy.

NguyenTanNhutQuang commented 11 months ago

The nbd proxy implementation present within bmcweb never had it's backend upstreamed. As such, this is dead code to us, and could be removed. Code in question is here: https://github.com/openbmc/bmcweb/blob/master/include/nbd_proxy.hpp And I suspect could be completely removed with no impact to the openbmc project.

@edtanous I misunderstand your first comment, you mean that the code in nbd_proxy.hpp will be removed only, is it?

Hi @edtanous, Good day! Just a soft remind, could you please confirm my question above? Thank you.

edtanous commented 1 month ago

Relevant virtual media code has been combined to the point where this dead code is no longer as much of a problem, and effort seems to be happening to get the virtual media daemon into upstream.

Closing this bug out.