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(a)redhat.com&gt; > --- > > 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.

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(a)redhat.com&gt; >>> --- >>> >>> 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(a)redhat.com&gt;