[Libguestfs] [v2v PATCH] RHV outputs: limit copied disk count to 23

Fri Jun 17 09:56:46 UTC 2022

On 06/17/22 11:33, Richard W.M. Jones wrote:
> On Fri, Jun 17, 2022 at 11:32:36AM +0200, Laszlo Ersek wrote:
>> On 06/17/22 11:17, Richard W.M. Jones wrote:
>>> On Fri, Jun 17, 2022 at 11:08:52AM +0200, Laszlo Ersek wrote:
>>>> We currently support virtio-blk (commonly) or IDE (unusually) for exposing
>>>> disks to the converted guest; refer to "guestcaps.gcaps_block_bus" in
>>>> "lib/create_ovf.ml". When using virtio-blk (i.e., in the common case), RHV
>>>> can deal with at most 23 disks, as it plugs each virtio-blk device in a
>>>> separate slot on the PCI(e) root bus; and the other slots are reserved for
>>>> various purposes. When a domain has too many disks, the problem only
>>>> becomes apparent once the copying finishes and an import is attempted.
>>>> Modify the RHV outputs to fail relatively early when a domain has more
>>>> than 23 disks that need to be copied.
>>>>
>>>> Notes:
>>>>
>>>> - With IDE, the theoretical limit may even be as low as 4. However, in the
>>>>   "Output_module.setup" function, we don't have access to
>>>>   "guestcaps.gcaps_block_bus", and in practice the IDE limitation has not
>>>>   caused surprises. So for now stick with 23, assuming virtio-blk.
>>>>   Modifying the "Output_module.setup" parameter list just for this seems
>>>>   overkill.
>>>>
>>>> - We could move the new check to an even earlier step, namely
>>>>   "Output_module.parse_options", due to the v2v directory deliberately
>>>>   existing (and having been populated with input sockets) at that time.
>>>>   However, even discounting the fact that "parse_options" is not a good
>>>>   name for including this kind of step, "parse_options" does not have
>>>>   access to the v2v directory name, and modifying the signature just for
>>>>   this is (again) overkill.
>>>>
>>>> - By adding the check to "Output_module.setup", we waste *some* effort
>>>>   (namely, the conversion occurs between "parse_options" and "setup"),
>>>>   but: (a) the "rhv-disk-uuid" count check (against the disk count) is
>>>>   already being done in the rhv-upload module's "setup" function, (b) in
>>>>   practice the slowest step ought to be the copying, and placing the new
>>>>   check in "setup" is early enough to prevent that.
>>>>
>>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2051564
>>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     This patch can be tested easily by replacing 23 with 0 in all three
>>>>     affected output modules -- then the following test cases all fail in
>>>>     "make check":
>>>>     
>>>>     FAIL: test-v2v-o-rhv.sh
>>>>     FAIL: test-v2v-o-vdsm-options.sh
>>>>     FAIL: test-v2v-o-rhv-upload.sh
>>>>     
>>>>     > [  11.3] Setting up the destination: -o rhv
>>>>     > virt-v2v: error: this output module doesn't support copying more than 0 disks
>>>>     
>>>>     > [   9.0] Setting up the destination: -o vdsm
>>>>     > virt-v2v: error: this output module doesn't support copying more than 0 disks
>>>>     
>>>>     > [  11.2] Setting up the destination: -o rhv-upload -oc https://example.com/ovirt-engine/api -os Storage
>>>>     > virt-v2v: error: this output module doesn't support copying more than 0 disks
>>>>
>>>>  output/output.mli           | 7 +++++++
>>>>  output/output.ml            | 5 +++++
>>>>  output/output_rhv.ml        | 1 +
>>>>  output/output_rhv_upload.ml | 1 +
>>>>  output/output_vdsm.ml       | 1 +
>>>>  5 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/output/output.mli b/output/output.mli
>>>> index 533a0c51d31c..2dec8ccdc690 100644
>>>> --- a/output/output.mli
>>>> +++ b/output/output.mli
>>>> @@ -76,6 +76,13 @@ val get_disks : string -> (int * int64) list
>>>>  (** Examines the v2v directory and opens each input socket (in0 etc),
>>>>      returning a list of input disk index and size. *)
>>>>  
>>>> +val bail_if_disk_count_gt : string -> int -> unit
>>>> +(** This function lets an output module enforce a maximum disk count.
>>>> +    [bail_if_disk_count_gt dir n] checks whether the domain has more than [n]
>>>> +    disks that need to be copied, by examining the existence of input NBD socket
>>>> +    "in[n]" in the v2v directory [dir].  If the socket exists, [error] is
>>>> +    called. *)
>>>
>>> We've commonly called these types of functions "error_if_..." or
>>> "error_unless_..." (you'll find many examples in the existing code).
>>> Could be better to rename it that way for consistency.
>>>
>>>>  val output_to_local_file : ?changeuid:((unit -> unit) -> unit) ->
>>>>                             Types.output_allocation ->
>>>>                             string -> string -> int64 -> string ->
>>>> diff --git a/output/output.ml b/output/output.ml
>>>> index 10e685c46926..0c4b437997d4 100644
>>>> --- a/output/output.ml
>>>> +++ b/output/output.ml
>>>> @@ -64,6 +64,11 @@ let get_disks dir =
>>>>    in
>>>>    loop [] 0
>>>>  
>>>> +let bail_if_disk_count_gt dir n =
>>>> +  let socket = sprintf "%s/in%d" dir n in
>>>> +  if Sys.file_exists socket then
>>>> +    error (f_"this output module doesn't support copying more than %d disks") n
>>>> +
>>>>  let output_to_local_file ?(changeuid = fun f -> f ())
>>>>        output_alloc output_format filename size socket =
>>>>    (* Check nbdkit is installed and has the required plugin. *)
>>>> diff --git a/output/output_rhv.ml b/output/output_rhv.ml
>>>> index 119207fdc065..a0c0be270755 100644
>>>> --- a/output/output_rhv.ml
>>>> +++ b/output/output_rhv.ml
>>>> @@ -56,6 +56,7 @@ module RHV = struct
>>>>      (options.output_alloc, options.output_format, output_name, output_storage)
>>>>  
>>>>    let rec setup dir options source =
>>>> +    bail_if_disk_count_gt dir 23;
>>>>      let disks = get_disks dir in
>>>>      let output_alloc, output_format, output_name, output_storage = options in
>>>>  
>>>> diff --git a/output/output_rhv_upload.ml b/output/output_rhv_upload.ml
>>>> index 828996b36261..6a9abf4eecdf 100644
>>>> --- a/output/output_rhv_upload.ml
>>>> +++ b/output/output_rhv_upload.ml
>>>> @@ -133,6 +133,7 @@ after their uploads (if you do, you must supply one for each disk):
>>>>      else PCRE.matches (Lazy.force rex_uuid) uuid
>>>>  
>>>>    let rec setup dir options source =
>>>> +    bail_if_disk_count_gt dir 23;
>>>>      let disks = get_disks dir in
>>>>      let output_conn, output_format,
>>>>          output_password, output_name, output_storage,
>>>> diff --git a/output/output_vdsm.ml b/output/output_vdsm.ml
>>>> index a1e8c2465810..02a2b5817fbc 100644
>>>> --- a/output/output_vdsm.ml
>>>> +++ b/output/output_vdsm.ml
>>>> @@ -119,6 +119,7 @@ For each disk you must supply one of each of these options:
>>>>       compat, ovf_flavour)
>>>>  
>>>>    let setup dir options source =
>>>> +    bail_if_disk_count_gt dir 23;
>>>>      let disks = get_disks dir in
>>>>      let output_alloc, output_format,
>>>>          output_name, output_storage,
>>>> -- 
>>>> 2.19.1.3.g30247aa5d201
>>>
>>> Looks fine, although I'd prefer the function to be renamed.
>>
>> Yes I will, consistency is important!
> 
> With the function renamed for consistency:
> 
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
> 

Thanks -- and I'm sorry, I ended up posting v2 before seeing this
response of yours -- I started make "check-slow" on v2, got distracted
with other BZs, and then posted v2 before fetching email.

Laszlo