Virtual Media repository request

Virtual Media repository request

[Hawrylewicz Czarnowski, Przemyslaw]

Hi.

I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).

The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.

And additional question: is there anything to do in order to enable CI for this repo?

-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Patrick Williams]

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

On Tue, Dec 07, 2021 at 03:50:47PM +0000, Hawrylewicz Czarnowski, Przemyslaw wrote:

Hello Przemyslaw,

Thank you for wanting to work at getting this code mainlined.  I know there has
been quite a bit of interest from various people outside Intel.

> I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).

I know you've got a pending commit to update some pieces of this design.  Since
none of the code has been submitted since the design was originally written, do
we need anyone to re-read it and see if anything has changed in the rest of the
codebase that needs design updates?

> The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.
> 

What did you have in mind for maintainer structure on this?  I'd ideally like to
see someone outside of Intel be a co-maintainer with you since:

  - This code was initially written as experimental Intel-only repository
    without any community feedback and
  - The current code hasn't been touched in 2 years and best practices have
    likely changed.
  - You're not currently a maintainer on any other repositories.

> And additional question: is there anything to do in order to enable CI for this repo?

Once the repository is set up, Andrew G can enable CI on it fairly quickly.

-- 
Patrick Williams

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

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 08.12.2021 17:56, Patrick Williams wrote:
> On Tue, Dec 07, 2021 at 03:50:47PM +0000, Hawrylewicz Czarnowski, Przemyslaw wrote:
> 
> Hello Przemyslaw,
> 
> Thank you for wanting to work at getting this code mainlined.  I know there has
> been quite a bit of interest from various people outside Intel.
> 
>> I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).
> 
> I know you've got a pending commit to update some pieces of this design.  Since
> none of the code has been submitted since the design was originally written, do
> we need anyone to re-read it and see if anything has changed in the rest of the
> codebase that needs design updates?

The code base for VM is "live" at the moment and besides 
asynchronousness nothing needs to be updated at the moment.

There is a limitation for DVD iso's which lies in USB gadget and how it 
is implemented, so maybe there could be some kind of note about that.

> 
>> The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.
>>
> 
> What did you have in mind for maintainer structure on this?  I'd ideally like to
> see someone outside of Intel be a co-maintainer with you since:
> 
>    - This code was initially written as experimental Intel-only repository
>      without any community feedback and
>    - The current code hasn't been touched in 2 years and best practices have
>      likely changed.
 >    - You're not currently a maintainer on any other repositories.

The code base exposed in provingground was under the development at that 
moment and actually shouldn't be submitted. The code is updated now 
(still needs some polishing I suppose) but I am ready to push updated 
sources and ask the community to review it.

Right now I am the main person for VM in Intel, but actually I was 
thinking about some co-maintainership in case there are other parties 
willing to have contribution in the code. This could be worked out 
during review process.

> 
>> And additional question: is there anything to do in order to enable CI for this repo?
> 
> Once the repository is set up, Andrew G can enable CI on it fairly quickly.
> 

Glad to hear that.


-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[i.kononenko]

Hello!

Here I have some points about the Virtual Media feature.
Some time ago I have worked to improve virtual media, in the YADRO we implement 
the feature based on the `Intel-BMC/provingground/virtual-media` implementation.

Since the project was not be upstreamed, that was our private changes. 
Today we have the good news, the project will be published and I guess our changes
could be provided too.

On 09.12.2021 11:56, Czarnowski, Przemyslaw wrote:
> On 08.12.2021 17:56, Patrick Williams wrote:
>> On Tue, Dec 07, 2021 at 03:50:47PM +0000, Hawrylewicz Czarnowski, Przemyslaw wrote:
>>
>> Hello Przemyslaw,
>>
>> Thank you for wanting to work at getting this code mainlined.  I know there has
>> been quite a bit of interest from various people outside Intel.
>>
>>> I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).
>>
>> I know you've got a pending commit to update some pieces of this design.  Since
>> none of the code has been submitted since the design was originally written, do
>> we need anyone to re-read it and see if anything has changed in the rest of the
>> codebase that needs design updates?
> 
> The code base for VM is "live" at the moment and besides asynchronousness nothing needs to be updated at the moment.
> 
> There is a limitation for DVD iso's which lies in USB gadget and how it is implemented, so maybe there could be some kind of note about that.
> 

Some months ago I published changes for the kernel:usb-gadget:mass-storage that aims
to support CD/DVD/BD media types (based on the image size).
The work has not been completed, the changes require to be properly formed. Looks 
like the publishing virtual media repository is a good point to actualize that patch
series.

Please, feel free to suggest helpful notes to improve a VM consumer interface; e.g. 
I still don't know how to better determine VM-image type - by the image size, how I 
did, or by sysfs interface(like the `cdrom` is specified).

>>
>>> The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.
>>>
>>
>> What did you have in mind for maintainer structure on this?  I'd ideally like to
>> see someone outside of Intel be a co-maintainer with you since:
>>
>>    - This code was initially written as experimental Intel-only repository
>>      without any community feedback and
>>    - The current code hasn't been touched in 2 years and best practices have
>>      likely changed.
>>    - You're not currently a maintainer on any other repositories.
> 
> The code base exposed in provingground was under the development at that moment and actually shouldn't be submitted. The code is updated now (still needs some polishing I suppose) but I am ready to push updated sources and ask the community to review it.
> 
> Right now I am the main person for VM in Intel, but actually I was thinking about some co-maintainership in case there are other parties willing to have contribution in the code. This could be worked out during review process.

Since I have some work to support VM and have examined the virtual-media implementation
by Intel I could contribute them.

> 
>>
>>> And additional question: is there anything to do in order to enable CI for this repo?
>>
>> Once the repository is set up, Andrew G can enable CI on it fairly quickly.
>>
> 
> Glad to hear that.
> 
> 

-- 
Best regards,

Igor Kononenko

Re: Virtual Media repository request

[Patrick Williams]

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

On Thu, Dec 09, 2021 at 09:56:55AM +0100, Czarnowski, Przemyslaw wrote:
> On 08.12.2021 17:56, Patrick Williams wrote:
> > On Tue, Dec 07, 2021 at 03:50:47PM +0000, Hawrylewicz Czarnowski, Przemyslaw wrote:

> > What did you have in mind for maintainer structure on this?  I'd ideally like to
> > see someone outside of Intel be a co-maintainer with you since:
> > 
> >    - This code was initially written as experimental Intel-only repository
> >      without any community feedback and
> >    - The current code hasn't been touched in 2 years and best practices have
> >      likely changed.
>  >    - You're not currently a maintainer on any other repositories.
> 
> The code base exposed in provingground was under the development at that 
> moment and actually shouldn't be submitted. The code is updated now 
> (still needs some polishing I suppose) but I am ready to push updated 
> sources and ask the community to review it.
> 
> Right now I am the main person for VM in Intel, but actually I was 
> thinking about some co-maintainership in case there are other parties 
> willing to have contribution in the code. This could be worked out 
> during review process.

This response feels like a bit of a chicken-and-egg.  Who is going to review and
approve the commits to the repository that assign a maintainer to the
repository?

Maybe Ed's proposal of using an existing repository solves that.  We would need
to make sure the current maintainer is accepting of whatever design direction
you've decided to go though.

-- 
Patrick Williams

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

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 13.12.2021 17:10, Patrick Williams wrote:
> On Thu, Dec 09, 2021 at 09:56:55AM +0100, Czarnowski, Przemyslaw wrote:
>> On 08.12.2021 17:56, Patrick Williams wrote:
>>> On Tue, Dec 07, 2021 at 03:50:47PM +0000, Hawrylewicz Czarnowski, Przemyslaw wrote:
> 
>>> What did you have in mind for maintainer structure on this?  I'd ideally like to
>>> see someone outside of Intel be a co-maintainer with you since:
>>>
>>>     - This code was initially written as experimental Intel-only repository
>>>       without any community feedback and
>>>     - The current code hasn't been touched in 2 years and best practices have
>>>       likely changed.
>>   >    - You're not currently a maintainer on any other repositories.
>>
>> The code base exposed in provingground was under the development at that
>> moment and actually shouldn't be submitted. The code is updated now
>> (still needs some polishing I suppose) but I am ready to push updated
>> sources and ask the community to review it.
>>
>> Right now I am the main person for VM in Intel, but actually I was
>> thinking about some co-maintainership in case there are other parties
>> willing to have contribution in the code. This could be worked out
>> during review process.
> 
> This response feels like a bit of a chicken-and-egg.  Who is going to review and
> approve the commits to the repository that assign a maintainer to the
> repository?

I just posted a response to Ed, then noticed new email in the thread.

The code for service is already created and reviewed and used 
internally. The I wanted to expose the code base to wide audience and 
incorporate any valuable input from the community - from people already 
using the old code base (from both jsnbd and provingground) and others 
willing to have impact on the form of the service.

> 
> Maybe Ed's proposal of using an existing repository solves that.  We would need
> to make sure the current maintainer is accepting of whatever design direction
> you've decided to go though.

As I mentioned in the other email, new service is much more complex. We 
have used it for some time and got an expertise on the whole stack.

Also as authors we want to have a key role in the future of this part of 
the code.

But still, please use all above as my input to make the best decision 
for the project.

-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Jeremy Kerr]

Hi all,

> Maybe Ed's proposal of using an existing repository solves that.  We
> would need to make sure the current maintainer is accepting of
> whatever design direction you've decided to go though.

I'm fine with replacing the jsnbd code with a newer implementation,
provided there's general community acceptance for doing so. If that's
the case, I'm happy to use the existing repo, or replacing openbmc/jsnbd
entirely - whatever suits best.

[Perhaps in your design document, you can expand the Alternatives
Considered section, to provide some motivation to change over]

However, I'm *not* OK with just introducing a completely alternate VM
implementation and leaving jsnbd as-is, where some platforms use one,
and some the other. We have way too many instances where there are two
separate implementations and/or repos that deliver the same
functionality. I would like to avoid making that problem worse.

So, either:

 - submit these as updates to jsnbd, which implement the new structure as
   you like. I'd be happy to hand over the repo to the new maintainers.

 or

 - provide the new VM implementation as a new repo, propose to change
   platforms to use the new implementation, and we can delete jsnbd.

Cheers,


Jeremy

Re: Virtual Media repository request

[Ed Tanous]

On Mon, Dec 13, 2021 at 6:11 PM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi all,
>
> > Maybe Ed's proposal of using an existing repository solves that.  We
> > would need to make sure the current maintainer is accepting of
> > whatever design direction you've decided to go though.
>
> I'm fine with replacing the jsnbd code with a newer implementation,
> provided there's general community acceptance for doing so. If that's
> the case, I'm happy to use the existing repo, or replacing openbmc/jsnbd
> entirely - whatever suits best.
>
> [Perhaps in your design document, you can expand the Alternatives
> Considered section, to provide some motivation to change over]
>
> However, I'm *not* OK with just introducing a completely alternate VM
> implementation and leaving jsnbd as-is, where some platforms use one,
> and some the other. We have way too many instances where there are two
> separate implementations and/or repos that deliver the same
> functionality. I would like to avoid making that problem worse.

+1

>
> So, either:
>
>  - submit these as updates to jsnbd, which implement the new structure as
>    you like. I'd be happy to hand over the repo to the new maintainers.
>
>  or
>
>  - provide the new VM implementation as a new repo, propose to change
>    platforms to use the new implementation, and we can delete jsnbd.
>
> Cheers,
>
>
> Jeremy

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 15.12.2021 20:26, Ed Tanous wrote:
> On Mon, Dec 13, 2021 at 6:11 PM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>>
>> Hi all,
>>
>>> Maybe Ed's proposal of using an existing repository solves that.  We
>>> would need to make sure the current maintainer is accepting of
>>> whatever design direction you've decided to go though.
>>
>> I'm fine with replacing the jsnbd code with a newer implementation,
>> provided there's general community acceptance for doing so. If that's
>> the case, I'm happy to use the existing repo, or replacing openbmc/jsnbd
>> entirely - whatever suits best.
>>
>> [Perhaps in your design document, you can expand the Alternatives
>> Considered section, to provide some motivation to change over]
>>
>> However, I'm *not* OK with just introducing a completely alternate VM
>> implementation and leaving jsnbd as-is, where some platforms use one,
>> and some the other. We have way too many instances where there are two
>> separate implementations and/or repos that deliver the same
>> functionality. I would like to avoid making that problem worse.
> 
> +1

I am ok with that approach, but I just wanted to separate service code 
and JS nbd server, as they are quite distinct entities from my perspective.
So yes for taking over VM functionality by the new service in separate 
repo, leaving only nbd server implementation on this repo.
What do you think?

> 
>>
>> So, either:
>>
>>   - submit these as updates to jsnbd, which implement the new structure as
>>     you like. I'd be happy to hand over the repo to the new maintainers.
>>
>>   or
>>
>>   - provide the new VM implementation as a new repo, propose to change
>>     platforms to use the new implementation, and we can delete jsnbd.
>>
>> Cheers,
>>
>>
>> Jeremy


-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Jeremy Kerr]

Hi Przemyslaw,

> I am ok with that approach, but I just wanted to separate service code
> and JS nbd server, as they are quite distinct entities from my
> perspective.

The actual nbd server code is tiny; only around 260 lines of javascript.
I don't think it's worth keeping a whole repo for that, given we would
not be using the jsnbd/nbd-proxy code.

So, I'd suggest just including this with the new VM implementation, or
moving it alongside the rest of the web ui.

Cheers,


Jeremy

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 17.12.2021 10:45, Jeremy Kerr wrote:
> Hi Przemyslaw,
> 
>> I am ok with that approach, but I just wanted to separate service code
>> and JS nbd server, as they are quite distinct entities from my
>> perspective.
> 
> The actual nbd server code is tiny; only around 260 lines of javascript.
> I don't think it's worth keeping a whole repo for that, given we would
> not be using the jsnbd/nbd-proxy code.
> 
> So, I'd suggest just including this with the new VM implementation, or
> moving it alongside the rest of the web ui.

Ok, got it.

I plan to start pushing changes here this week.

There is just a one thing to be determined.
Currently, the "proxy" mode handler for websocket in bmcweb is defined 
as /vm/0/0 (see include/vm_websocket.hpp:161).
New service handler (to be found in include/nbd_proxy.hpp) requires 
websocket defined as /nbd/<str> as more slots can be available.
This breaks the old API.
I believe there should be a kind of migration period for applications 
that use old location and format before the old one is turned off.
For /nbd location there is already implemented UI module to handle that 
(but needs to be enabled).

Who is aware of such applications?


> 
> Cheers,
> 
> 
> Jeremy
> 


-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 20.12.2021 13:54, Czarnowski, Przemyslaw wrote:
> 
> I plan to start pushing changes here this week.
> 
Replying to myself, but just pushed a series of first 4 patches to review.

It is just a skeleton (infrastructure to build main flows on) but wanted 
to get the first reviews for initial changes (transition from old 
content to the new one in particular).

-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 23.12.2021 02:01, Czarnowski, Przemyslaw wrote:
> On 20.12.2021 13:54, Czarnowski, Przemyslaw wrote:
>>
>> I plan to start pushing changes here this week.
>>
> Replying to myself, but just pushed a series of first 4 patches to review.
> 
> It is just a skeleton (infrastructure to build main flows on) but wanted 
> to get the first reviews for initial changes (transition from old 
> content to the new one in particular).
> 

I would like to rise the request for new service of Virtual Media 
repository once again.

Recently I've made an attempt to push VM service patches once again 
after UT has been added. In the meantime, I've noticed that in order to 
make a graceful transition between old and new solution the final switch 
between the old and new code should be made at the moment when the last 
patch is accepted.

There are two main reason why I would like to recall the discussion.
First is the voice of the maintainer of the old service. Second - 
problems with linting and CI which wants to build and test both projects 
simultaneously.

Of course the decision does not belong to me. I just do not want mess 
with current CI to support this transitional state.

-- 
Best regards,
Przemyslaw Czarnowski

Re: Virtual Media repository request

[Ed Tanous]

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

On Mon, Dec 19, 2022 at 6:02 AM Czarnowski, Przemyslaw <
przemyslaw.hawrylewicz.czarnowski@linux.intel.com> wrote:

> On 23.12.2021 02:01, Czarnowski, Przemyslaw wrote:
> > On 20.12.2021 13:54, Czarnowski, Przemyslaw wrote:
> >>
> >> I plan to start pushing changes here this week.
> >>
> > Replying to myself, but just pushed a series of first 4 patches to
> review.
> >
> > It is just a skeleton (infrastructure to build main flows on) but wanted
> > to get the first reviews for initial changes (transition from old
> > content to the new one in particular).
> >
>
> I would like to rise the request for new service of Virtual Media
> repository once again.
>
> Recently I've made an attempt to push VM service patches once again
> after UT has been added. In the meantime, I've noticed that in order to
> make a graceful transition between old and new solution the final switch
> between the old and new code should be made at the moment when the last
> patch is accepted.
>
> There are two main reason why I would like to recall the discussion.
> First is the voice of the maintainer of the old service.


What does "voice" mean in the above?  Can you elaborate on the problem, and
give some specific examples (ideally links to the gerrit reviews)?


> Second -
> problems with linting and CI which wants to build and test both projects
> simultaneously.
>

Same thing here, can you give specific examples?  The linting is just
enforcing the coding standard, and testing two executables at the same time
is something many other repositories do.


>
> Of course the decision does not belong to me. I just do not want mess
> with current CI to support this transitional state.
>

My opinion still remains the same, let's keep everything in one
repository.  If you need help in CI to do that, let's talk about the
specific things that are blocking, and we can go from there.


>
> --
> Best regards,
> Przemyslaw Czarnowski
>
>

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

Re: Virtual Media repository request

[Jeremy Kerr]

Hi Przemyslaw,
> I would like to rise the request for new service of Virtual Media 
> repository once again.

Sure! I've seen a lot of gerrit updates happen recently, but it looks
like they're failing verification. Is there a particular challenge that
you're facing there?

> Recently I've made an attempt to push VM service patches once again 
> after UT has been added.

(what does UT refer to here?)

> In the meantime, I've noticed that in order to make a graceful
> transition between old and new solution the final
> switch between the old and new code should be made at the moment when
> the last patch is accepted.

In this case, I figure the approach is to just make a wholesale switch-
over to the new codebase; so, an incremental patch series may be a bit
onerous on you for not much community benefit. Since you'd be the
maintainer of the new code, I'm happy with whatever approach gets you
there :)

There was an earlier decision to use the same (jsnbd) repo for this,
rather than add a new repo and do a switch over. That wouldn't be my
preferred approach, but still seems workable - is that the issue you're
looking to re-review? Or something else?

Cheers,


Jeremy

Re: Virtual Media repository request

[Ed Tanous]

On Tue, Dec 7, 2021 at 7:52 AM Hawrylewicz Czarnowski, Przemyslaw
<przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
>
> Hi.
>
> I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).

Considering that the virtual media already uses pieces of
functionality from the old virtual media, why wouldn't this just go in
https://github.com/openbmc/jsnbd

Ideally we shouldn't need two different virtual media implementations,
and my understanding is that the "new" one is a complete replacement
for jsnbd, while still using the javascript portions of it;  Moving
the implementation there will simplify when people look for virtual
media, and will promote the reuse of code, so I think that's what we
should do.

>
> The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.
>
> And additional question: is there anything to do in order to enable CI for this repo?

I believe CI is already enabled for jsnbd, so I think we're already
good to go there.

>
> --
> Best regards,
> Przemyslaw Czarnowski
>

Re: Virtual Media repository request

[Czarnowski, Przemyslaw]

On 11.12.2021 22:13, Ed Tanous wrote:
> On Tue, Dec 7, 2021 at 7:52 AM Hawrylewicz Czarnowski, Przemyslaw
> <przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
>>
>> Hi.
>>
>> I would like to request for new Virtual Media service repository (based on the design document located here: https://github.com/openbmc/docs/blob/master/designs/virtual-media.md).
> 
> Considering that the virtual media already uses pieces of
> functionality from the old virtual media, why wouldn't this just go in
> https://github.com/openbmc/jsnbd
> 
> Ideally we shouldn't need two different virtual media implementations,
> and my understanding is that the "new" one is a complete replacement
> for jsnbd, while still using the javascript portions of it;  Moving
> the implementation there will simplify when people look for virtual
> media, and will promote the reuse of code, so I think that's what we
> should do.

I think it depends. From my perspective, mixing javascript and service
code is not a good thing. They come from different origins and does
quite different job (even if they are connected together)

Moreover virtual media service if far more than the current
implementation. Old functionality is just at least 1/4 of the
possibilities of new service.

This is, of course, my personal insight. There are more competent people
with wider background in OpenBMC to make the final decision.

Przemek


> 
>>
>> The service itself is a reworked Virtual Media which early stage is available here: https://github.com/Intel-BMC/provingground.
>>
>> And additional question: is there anything to do in order to enable CI for this repo?
> 
> I believe CI is already enabled for jsnbd, so I think we're already
> good to go there.
> 
>>
>> --
>> Best regards,
>> Przemyslaw Czarnowski
>>


-- 
Best regards,
Przemyslaw Czarnowski