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.
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
