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

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

On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev
wrote:
> On 3/2/23 20:36, Richard W.M. Jones wrote:
> > 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:
The environment variable is a bit of a mistake too.  It'd be better to
specify that through the command line; and in fact that's what I went
with for the new virt-customize --inject-virtio-win option:

       --inject-virtio-win METHOD
           Inject virtio-win drivers into a Windows guest.  These drivers add
           virtio accelerated drivers suitable when running on top of a
           hypervisor that supports virtio (such as qemu/KVM).  The operation
           also adjusts the Windows Registry so that the drivers are installed
           when the guest boots.

           The parameter can be one of:

           ISO The path to the ISO image containing the virtio-win drivers
               (eg. /usr/share/virtio-win/virtio-win.iso).
   [etc]
> > 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?
I prefer the command line option even though it's a little bit more
work.  Offer above still stands if you can't work out the complexities
of OCaml command line parsing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org