Libguestfs - Mar 2023 - [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows

On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev
wrote:
> On 2/28/23 16:39, Richard W.M. Jones wrote:
> > On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:
> >> On 2/28/23 14:01, Richard W.M. Jones wrote:
> >>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev
wrote:
> >>>> Since commits b28cd1dc ("Remove requested_guestcaps /
rcaps"), f0afc439
> >>>> ("Remove guestcaps_block_type Virtio_SCSI")
support for installing
> >>>> virtio-scsi driver is missing in virt-v2v.  AFAIU plans
and demands for
> >>>> bringing this feature back have been out there for a
while.  E.g. I've
> >>>> found a corresponding issue which is still open [1].
> >>>>
> >>>> The code in b28cd1dc, f0afc439 was removed due to removing
the old in-place
> >>>> support.  However, having the new in-place support present
and bringing
> >>>> this same code (partially) back with several additions and
improvements,
> >>>> I'm able to successfully convert and boot a Win guest
with a virtio-scsi
> >>>> disk controller.  So please consider the following
implementation of
> >>>> this feature.
> >>>>
> >>>> [1] https://github.com/libguestfs/virt-v2v/issues/12
> >>>
> >>> So you're right that we ended up removing virtio-scsi
support because
> >>> in-place conversion was temporarily removed, and it wasn't
restored
> >>> when the new virt-v2v-in-place tool was added.
> >>>
> >>> I looked at the patches, and I don't have any objection to
1-4.
> >>>
> >>> Patch 5 is the tricky one because it changes the default, but
we
> >>> (Red Hat) are likely to stick with virtio-blk for better or
worse.
> >>>
> >>> Could we make this configurable?  We've traditionally
shied away from
> >>> adding virt-v2v command line options which are purely for
fine-tuning
> >>> conversions.  The reason for this is that when you're
doing bulk
> >>> conversions of hundreds of VMs from VMware, you want virt-v2v
to do
> >>> "the best it can", and it's not really feasible
to hand configure
> >>> every conversion.  (There are some historical exceptions to
this, like
> >>> --root, but don't look at those!)  I know the bulk
conversion case
> >>> doesn't apply to Virtuozzo, but it does matter for us.
> >>>
> >>> It could be argued that this isn't the same, because if
we're bulk
> >>> converting, you're not fine-tuning virtio-scsi vs
virtio-blk per VM,
> >>> but per target hypervisor.  So a command line option would be
OK in
> >>> this case.
> >>>
> >>> BTW previously we "configured" this preference
through the input
> >>> libvirt XML.  That was a terrible idea, so I'd prefer to
avoid it this
> >>> time around.
> >>
> >> Ah, that's new information to me. I thought the
"rcaps" (requested caps,
> >> derived from the input XML) was exactly what was missing here. Not
> >> because of the particular reason(s) that may have motivated rcaps
in the
> >> past, but because in the use case at hand, the tweaking of the
domain
> >> XML precedes the conversion (more like, the injection of the
desired
> >> virtio drivers).
> >>
> >> Why was it a terrible idea?
> > 
> > It was a hack.  For copying (normal) conversions -i libvirtxml
> > describes the input metadata.  For in-place conversions
"input" is a
> > fuzzy concept, so it was overloaded to describe the conversion
> > preferences.  We can do better with a command line parameter.
> 
> 
> Could you please elaborate on why is that a fuzzy concept?  I understand
> that bringing back the entire requested capabilities (rcaps) framework
> might be undesirable, but the idea of choosing the block driver based on
> the source libvirt XML doesn't seem very wrong to me.  Even if we
> perform some modifications on that source XML (which I'm not sure of)
we
> end up with a VM config having a particular set of devices, and by that
> point all fuzziness should be eliminated.  Am I missing something?
It's mixing up two concepts, the input metadata and the conversion
parameters.  Plus it's plain weird.  And hard for end users to do.
Command line options would be how you normally influence the behaviour
of a command line tool.
> > 
> >>> Anyway if you can think of a better version of patch 5 then I
think we
> >>> can accept this.
> >>
> >> ITYM "if you *cannot* think of a better version"...
> >>
> >> But anyway, if a command line option is acceptable to you, I'm
perfectly
> >> fine with it as well. I'd like to avoid changing the default.
I quite
> >> foresee tens of QE test runs breaking, resulting in as many bug
reports,
> >> and then us eventually reverting the change-of-the-default
downstream.
> >> I'd rather favor an experimental command line switch.
> > 
> > Yes I'm sure we don't want to change the default; or at least
we
> > (meaning Red Hat, downstream) want to choose whether and when to
> > change the default.
> 
> Speaking of the command line option: if we end up with such a solution,
> do you think it should be specific to and only applicable in the
> in-place mode?
I think it would apply to all of the virt-v2v* tools, that seems
sensible to me I think.  When doing a copying conversion you
might also want to prefer virtio-scsi.

I can probably do an outline of a patch if you need it, but
not today.

Rich.
> > 
> > BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora).
> > 
> > Thanks,
> > 
> > Rich.
> > 
> 
> P.S. Resent, the original mail ended up off the mailing list.
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 3/2/23 20:36, Richard W.M. Jones wrote:
> On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev wrote:
>> On 2/28/23 16:39, Richard W.M. Jones wrote:
>>> On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:
>>>> On 2/28/23 14:01, Richard W.M. Jones wrote:
>>>>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev
wrote:
>>>>>> Since commits b28cd1dc ("Remove
requested_guestcaps / rcaps"), f0afc439
>>>>>> ("Remove guestcaps_block_type Virtio_SCSI")
support for installing
>>>>>> virtio-scsi driver is missing in virt-v2v.  AFAIU plans
and demands for
>>>>>> bringing this feature back have been out there for a
while.  E.g. I've
>>>>>> found a corresponding issue which is still open [1].
>>>>>>
>>>>>> The code in b28cd1dc, f0afc439 was removed due to
removing the old in-place
>>>>>> support.  However, having the new in-place support
present and bringing
>>>>>> this same code (partially) back with several additions
and improvements,
>>>>>> I'm able to successfully convert and boot a Win
guest with a virtio-scsi
>>>>>> disk controller.  So please consider the following
implementation of
>>>>>> this feature.
>>>>>>
>>>>>> [1] https://github.com/libguestfs/virt-v2v/issues/12
>>>>>
>>>>> So you're right that we ended up removing virtio-scsi
support because
>>>>> in-place conversion was temporarily removed, and it
wasn't restored
>>>>> when the new virt-v2v-in-place tool was added.
>>>>>
>>>>> I looked at the patches, and I don't have any objection
to 1-4.
>>>>>
>>>>> Patch 5 is the tricky one because it changes the default,
but we
>>>>> (Red Hat) are likely to stick with virtio-blk for better or
worse.
>>>>>
>>>>> Could we make this configurable?  We've traditionally
shied away from
>>>>> adding virt-v2v command line options which are purely for
fine-tuning
>>>>> conversions.  The reason for this is that when you're
doing bulk
>>>>> conversions of hundreds of VMs from VMware, you want
virt-v2v to do
>>>>> "the best it can", and it's not really
feasible to hand configure
>>>>> every conversion.  (There are some historical exceptions to
this, like
>>>>> --root, but don't look at those!)  I know the bulk
conversion case
>>>>> doesn't apply to Virtuozzo, but it does matter for us.
>>>>>
>>>>> It could be argued that this isn't the same, because if
we're bulk
>>>>> converting, you're not fine-tuning virtio-scsi vs
virtio-blk per VM,
>>>>> but per target hypervisor.  So a command line option would
be OK in
>>>>> this case.
>>>>>
>>>>> BTW previously we "configured" this preference
through the input
>>>>> libvirt XML.  That was a terrible idea, so I'd prefer
to avoid it this
>>>>> time around.
>>>>
>>>> Ah, that's new information to me. I thought the
"rcaps" (requested caps,
>>>> derived from the input XML) was exactly what was missing here.
Not
>>>> because of the particular reason(s) that may have motivated
rcaps in the
>>>> past, but because in the use case at hand, the tweaking of the
domain
>>>> XML precedes the conversion (more like, the injection of the
desired
>>>> virtio drivers).
>>>>
>>>> Why was it a terrible idea?
>>>
>>> It was a hack.  For copying (normal) conversions -i libvirtxml
>>> describes the input metadata.  For in-place conversions
"input" is a
>>> fuzzy concept, so it was overloaded to describe the conversion
>>> preferences.  We can do better with a command line parameter.
>>
>>
>> Could you please elaborate on why is that a fuzzy concept?  I
understand
>> that bringing back the entire requested capabilities (rcaps) framework
>> might be undesirable, but the idea of choosing the block driver based
on
>> the source libvirt XML doesn't seem very wrong to me.  Even if we
>> perform some modifications on that source XML (which I'm not sure
of) we
>> end up with a VM config having a particular set of devices, and by that
>> point all fuzziness should be eliminated.  Am I missing something?
> 
> It's mixing up two concepts, the input metadata and the conversion
> parameters.  Plus it's plain weird.  And hard for end users to do.
> Command line options would be how you normally influence the behaviour
> of a command line tool.
> 
>>>
>>>>> Anyway if you can think of a better version of patch 5 then
I think we
>>>>> can accept this.
>>>>
>>>> ITYM "if you *cannot* think of a better version"...
>>>>
>>>> But anyway, if a command line option is acceptable to you,
I'm perfectly
>>>> fine with it as well. I'd like to avoid changing the
default. I quite
>>>> foresee tens of QE test runs breaking, resulting in as many bug
reports,
>>>> and then us eventually reverting the change-of-the-default
downstream.
>>>> I'd rather favor an experimental command line switch.
>>>
>>> Yes I'm sure we don't want to change the default; or at
least we
>>> (meaning Red Hat, downstream) want to choose whether and when to
>>> change the default.
>>
>> Speaking of the command line option: if we end up with such a solution,
>> do you think it should be specific to and only applicable in the
>> in-place mode?
> 
> I think it would apply to all of the virt-v2v* tools, that seems
> sensible to me I think.  When doing a copying conversion you
> might also want to prefer virtio-scsi.
> 
> I can probably do an outline of a patch if you need it, but
> not today.
Yes, sure.  But speaking of the ways to influence the behaviour of a
command line tool: there're also env variables, which are being used by
v2v as well (for instance, for setting the tools ISO location).  So what
if we introduce an env variable here instead?  I presume it would be
much easier to implement.  Smth like:
> diff --git a/mlcustomize/inject_virtio_win.ml
b/mlcustomize/inject_virtio_win.ml
> index 62f7710..562f055 100644
> --- a/mlcustomize/inject_virtio_win.ml
> +++ b/mlcustomize/inject_virtio_win.ml
> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg
>      (* Can we install the block driver? *)
>      let block : block_type >        let filenames =
["virtio_blk"; "vrtioblk"; "viostor"] in
> +      let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in
> +      let filenames > +        match viostor_drv_env with
> +        | None -> filenames
> +        | Some str -> str :: filenames in
>        let viostor_driver = try (
>          Some (
>            List.find (
As you can see here the default doesn't change if the variable isn't
provided at all (or it is, but is set to a non-existing driver).
What do you think of such a solution?
> 
> Rich.
> 
>>>
>>> BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in
Fedora).
>>>
>>> Thanks,
>>>
>>> Rich.
>>>
>>
>> P.S. Resent, the original mail ended up off the mailing list.
>