Connecting to an NBD server temporarily, for a "one-shot" operation, is quite similar to "Std_utils.with_open_in" and "Std_utils.with_open_out", as there are cleanup operations regardless of whether the "one-shot" operation completes successfully or throws an exception. Introduce the "Nbdkit.with_connect_unix" function, which takes a Unix domain socket pathname, a list of metadata contexts to request from the NBD server, and calls a function with the live NBD server connection. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/Makefile.am | 2 +- lib/nbdkit.mli | 10 ++++++++++ lib/nbdkit.ml | 12 ++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/Makefile.am b/lib/Makefile.am index c274b9ecf6c7..1fab25b326f5 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -80,7 +80,7 @@ BOBJECTS = config.cmo $(SOURCES_ML:.ml=.cmo) XOBJECTS = $(BOBJECTS:.cmo=.cmx) OCAMLPACKAGES = \ - -package str,unix \ + -package str,unix,nbd \ -I $(builddir) \ -I $(top_builddir)/common/mlgettext \ -I $(top_builddir)/common/mlpcre \ diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli index ae19295186ed..825755e61dbe 100644 --- a/lib/nbdkit.mli +++ b/lib/nbdkit.mli @@ -107,3 +107,13 @@ val run_unix : ?socket:string -> cmd -> string * int The --exit-with-parent, --foreground, --pidfile, --newstyle and --unix flags are added automatically. Other flags are set as in the {!cmd} struct. *) + +val with_connect_unix : socket:string -> + meta_contexts:string list -> + f:(NBD.t -> 'a) -> + 'a +(** [with_connect_unix socket meta_contexts f] calls function [f] with the NBD + server at Unix domain socket [socket] connected, and the metadata contexts + in [meta_contexts] requested (each of which is not necessarily supported by + the server though). The connection is torn down either on normal return or + if the function [f] throws an exception. *)

+ ~f:(fun () ->

+ NBD.connect_unix nbd socket; + protect + ~f:(fun () -> f nbd) + ~finally:(fun () -> NBD.shutdown nbd) + ) + ~finally:(fun () -> NBD.close nbd)

On 12/08/21 16:18, Richard W.M. Jones wrote: > On Wed, Dec 08, 2021 at 01:20:48PM +0100, Laszlo Ersek wrote: >> Connecting to an NBD server temporarily, for a "one-shot" operation, is >> quite similar to "Std_utils.with_open_in" and "Std_utils.with_open_out", >> as there are cleanup operations regardless of whether the "one-shot" >> operation completes successfully or throws an exception. >> >> Introduce the "Nbdkit.with_connect_unix" function, which takes a Unix >> domain socket pathname, a list of metadata contexts to request from the >> NBD server, and calls a function with the live NBD server connection. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> lib/Makefile.am | 2 +- >> lib/nbdkit.mli | 10 ++++++++++ >> lib/nbdkit.ml | 12 ++++++++++++ >> 3 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/lib/Makefile.am b/lib/Makefile.am >> index c274b9ecf6c7..1fab25b326f5 100644 >> --- a/lib/Makefile.am >> +++ b/lib/Makefile.am >> @@ -80,7 +80,7 @@ BOBJECTS = config.cmo $(SOURCES_ML:.ml=.cmo) >> XOBJECTS = $(BOBJECTS:.cmo=.cmx) >> >> OCAMLPACKAGES = \ >> - -package str,unix \ >> + -package str,unix,nbd \ >> -I $(builddir) \ >> -I $(top_builddir)/common/mlgettext \ >> -I $(top_builddir)/common/mlpcre \ >> diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli >> index ae19295186ed..825755e61dbe 100644 >> --- a/lib/nbdkit.mli >> +++ b/lib/nbdkit.mli >> @@ -107,3 +107,13 @@ val run_unix : ?socket:string -> cmd -> string * int >> The --exit-with-parent, --foreground, --pidfile, --newstyle and >> --unix flags are added automatically. Other flags are set as >> in the {!cmd} struct. *) >> + >> +val with_connect_unix : socket:string -> >> + meta_contexts:string list -> >> + f:(NBD.t -> 'a) -> >> + 'a >> +(** [with_connect_unix socket meta_contexts f] calls function [f] with the NBD >> + server at Unix domain socket [socket] connected, and the metadata contexts >> + in [meta_contexts] requested (each of which is not necessarily supported by >> + the server though). The connection is torn down either on normal return or >> + if the function [f] throws an exception. *) > > Interesting choice of module for this. nbdkit is a server. I would > have put it into lib/utils.mli or created a new module. Yes, I was certainly lost as to what module to pick :) I found "run_unix" in this one, and thought that a wrapper for "connect_unix" (from the OCaml binding of libnbd) belonged here. I can add it to "utils" if you think that's a good fit. (I feel that creating a new module just for this is overkill, but I could be wrong.)

> The function itself looks fine although personally I might have > abstracted the meta_context function to be a "do anything between > creation and connection" (let's call that "preparation"). What do you > think about: > > val with_connect_unix : socket:string -> > ?prepare:(NBD.t -> unit) -> > f:(NBD.t -> 'a) -> > 'a > > I think you would have to replace the List.iter line: > > ... >> + ~f:(fun () -> > > (match prepare with Some pf -> pf nbd | None -> ()); > >> + NBD.connect_unix nbd socket; >> + protect >> + ~f:(fun () -> f nbd) >> + ~finally:(fun () -> NBD.shutdown nbd) >> + ) >> + ~finally:(fun () -> NBD.close nbd) > > It would be called like this: > > let prepare nbd = NBD.add_meta_context nbd "base:allocation" in > with_connect_unix socket ~prepare ( > fun () -> > (* the code *) > ) > > Hmm, maybe this is getting more complicated :-( Well my concern is the following; let me paste the full function again: let with_connect_unix ~socket ~meta_contexts ~f = let nbd = NBD.create () in protect ~f:(fun () -> List.iter (NBD.add_meta_context nbd) meta_contexts; NBD.connect_unix nbd socket; protect ~f:(fun () -> f nbd) ~finally:(fun () -> NBD.shutdown nbd) ) ~finally:(fun () -> NBD.close nbd) "NBD.close", which mirrors "NBD.create", suffices for undoing "NBD.add_meta_context" as well. But, if we turn "NBD.add_meta_context" into a generic "do anything between NBD.create and NBD.connect_unix", I don't know if "NBD.close" can still satisfy the teardown role. In that case, we might have to accept another "rollback" function, and *that* I do find too complicated. FWIW, the current code does make a similar assumption about "f", and "NBD.shutdown". However, I thought that that was OK (and didn't need spelling out): namely, once "NBD.connect_unix" completes, any valid operation on the connection is considered... well, valid (or "expected"), and "NBD.shutdown" is considered appropriate to finish *any* such sequence of operations. I don't have the same confidence in a generic "prepare" being undone by just NBD.close.

Add the "Nbdkit.get_disk_allocated" helper function, which calculates the number of allocated bytes in an output disk image, through a corresponding NBD server connection. Original "NBD.block_status" summation example code by Rich Jones (thanks!), chunking, metadata context simplification, and virt-v2v integration by myself. Chunking added because "NBD.block_status" is not 64-bit clean just yet (Eric Blake is working on it). Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/nbdkit.mli | 8 ++++++ lib/nbdkit.ml | 30 ++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli index 825755e61dbe..03107dab109d 100644 --- a/lib/nbdkit.mli +++ b/lib/nbdkit.mli @@ -117,3 +117,11 @@ val with_connect_unix : socket:string -> in [meta_contexts] requested (each of which is not necessarily supported by the server though). The connection is torn down either on normal return or if the function [f] throws an exception. *) + +val get_disk_allocated : dir:string -> disknr:int -> int64 option +(** Callable only in the finalization step. [get_disk_allocated dir disknr] + examines output disk [disknr] through the corresponding NBD server socket + that resides in [dir]. Returns the number of bytes allocated in the disk + image, according to the "base:allocation" metadata context. If the context + is not supported by the NBD server behind the socket, the function returns + None. *)

diff --git a/lib/nbdkit.ml b/lib/nbdkit.ml index 43e0fc5c728a..e142c210c66f 100644 --- a/lib/nbdkit.ml +++ b/lib/nbdkit.ml @@ -229,3 +229,33 @@ let with_connect_unix ~socket ~meta_contexts ~f = ~finally:(fun () -> NBD.shutdown nbd) ) ~finally:(fun () -> NBD.close nbd) + +let get_disk_allocated ~dir ~disknr = + let socket = sprintf "%s/out%d" dir disknr + and alloc_ctx = "base:allocation" in + with_connect_unix ~socket ~meta_contexts:[alloc_ctx] + ~f:(fun nbd -> + if NBD.can_meta_context nbd alloc_ctx then ( + (* Get the list of extents, using a 2GiB chunk size as hint. *) + let size = NBD.get_size nbd + and allocated = ref 0_L + and fetch_offset = ref 0_L in + while !fetch_offset < size do + let remaining = size -^ !fetch_offset in + let fetch_size = min 0x8000_0000_L remaining in + NBD.block_status nbd fetch_size !fetch_offset + (fun ctx offset entries err -> + assert (ctx = alloc_ctx); + for i = 0 to Array.length entries / 2 - 1 do + let len = Int64.of_int32 entries.(i * 2) + and typ = entries.(i * 2 + 1) in + if Int32.logand typ 1_l = 0_l then + allocated := !allocated +^ len; + fetch_offset := !fetch_offset +^ len + done; + 0 + ) + done; + Some !allocated + ) else None + )

On 12/08/21 16:21, Richard W.M. Jones wrote: > On Wed, Dec 08, 2021 at 01:20:49PM +0100, Laszlo Ersek wrote: >> Add the "Nbdkit.get_disk_allocated" helper function, which calculates the >> number of allocated bytes in an output disk image, through a corresponding >> NBD server connection. >> >> Original "NBD.block_status" summation example code by Rich Jones >> (thanks!), chunking, metadata context simplification, and virt-v2v >> integration by myself. Chunking added because "NBD.block_status" is not >> 64-bit clean just yet (Eric Blake is working on it). >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> lib/nbdkit.mli | 8 ++++++ >> lib/nbdkit.ml | 30 ++++++++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli >> index 825755e61dbe..03107dab109d 100644 >> --- a/lib/nbdkit.mli >> +++ b/lib/nbdkit.mli >> @@ -117,3 +117,11 @@ val with_connect_unix : socket:string -> >> in [meta_contexts] requested (each of which is not necessarily supported by >> the server though). The connection is torn down either on normal return or >> if the function [f] throws an exception. *) >> + >> +val get_disk_allocated : dir:string -> disknr:int -> int64 option >> +(** Callable only in the finalization step. [get_disk_allocated dir disknr] >> + examines output disk [disknr] through the corresponding NBD server socket >> + that resides in [dir]. Returns the number of bytes allocated in the disk >> + image, according to the "base:allocation" metadata context. If the context >> + is not supported by the NBD server behind the socket, the function returns >> + None. *) > > This really is the right function, wrong module. > > Since it only applies to output modules, the logical place is in > output/output.ml, where there are already a bunch of random helper > functions. That's precisely where I put it first (given that we have "get_disks" there too!). However, I proved simply unable to *call* the function, from "lib/create_ovf.ml", in the next patch!

I tried "Output.get_disk_allocated", and I got a weird compilation failure, even though the function was properly declared in "output.mli".

At that point, I couldn't decide if I was missing something about the tricky "first class modules" thing, or if it was really a layering violation to call an Output function from a *lower level* utility function (such as "create_ovf" in "lib/create_ovf.ml").

Commit 255722cbf39a ("v2v: Modular virt-v2v", 2021-09-07) removed the following attributes from the OVF output: - ovf:Envelope/References/File/@ovf:size - ovf:Envelope/Section[@xsi:type='ovf:DiskSection_Type']/Disk/@ovf:actual_size Unfortunately, ovirt-engine considers the second one mandatory; without it, ovirt-engine refuses to import the OVF. Restore both attributes, using the utility functions added to the Nbdkit module previously. (If we do not have the information necessary to fill in @ovf:actual_size, we still have to generate the attribute, only with empty contents. Ovirt-engine does cope with that.) Fixes: 255722cbf39afc0b012e2ac00d16fa6ba2f8c21f Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/create_ovf.mli | 2 +- lib/create_ovf.ml | 33 +++++++++----------- output/output_rhv.ml | 2 +- output/output_rhv_upload.ml | 2 +- output/output_vdsm.ml | 1 + tests/test-v2v-o-rhv.ovf.expected | 4 +-- tests/test-v2v-o-vdsm-options.ovf.expected | 4 +-- 7 files changed, 23 insertions(+), 25 deletions(-) diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli index 701152d93dd2..0d1cc5a9311a 100644 --- a/lib/create_ovf.mli +++ b/lib/create_ovf.mli @@ -46,7 +46,7 @@ val ovf_flavour_to_string : ovf_flavour -> string val create_ovf : Types.source -> Types.inspect -> Types.target_meta -> int64 list -> Types.output_allocation -> string -> string -> string list -> - string list -> string -> ovf_flavour -> DOM.doc + string list -> string -> string -> ovf_flavour -> DOM.doc (** Create the OVF file. Actually a {!DOM} document is created, not a file. It can be written diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index c9f14635363e..7bc15b74662a 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -531,7 +531,7 @@ let rec create_ovf source inspect { output_name; guestcaps; target_firmware; target_nics } sizes output_alloc output_format - sd_uuid image_uuids vol_uuids vm_uuid ovf_flavour = + sd_uuid image_uuids vol_uuids dir vm_uuid ovf_flavour = assert (List.length sizes = List.length vol_uuids); let memsize_mb = source.s_memory /^ 1024L /^ 1024L in @@ -745,7 +745,7 @@ let rec create_ovf source inspect (* Add disks to the OVF XML. *) add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids ovf_flavour ovf; + sd_uuid image_uuids vol_uuids dir ovf_flavour ovf; (* Old virt-v2v ignored removable media. XXX *) @@ -791,7 +791,7 @@ and get_flavoured_section ovf ovirt_path rhv_path rhv_path_attr = function (* This modifies the OVF DOM, adding a section for each disk. *) and add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids ovf_flavour ovf = + sd_uuid image_uuids vol_uuids dir ovf_flavour ovf = let references = let nodes = path_to_nodes ovf ["ovf:Envelope"; "References"] in match nodes with @@ -839,13 +839,7 @@ and add_disks sizes guestcaps output_alloc output_format b /^ 1073741824L in let size_gb = bytes_to_gb size in - (* virt-v2v 1.4x used to try to collect the actual size of the - * sparse disk. It would be possible to get this information - * accurately now by reading the extent map of the output disk - * (this function is called during finalization), but we don't - * yet do that. XXX - *) - let actual_size_gb = None in + let actual_size = Nbdkit.get_disk_allocated ~dir ~disknr:i in let format_for_rhv = match output_format with @@ -867,13 +861,11 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:id", vol_uuid; "ovf:description", generated_by; ] in - (* See note above about actual_size_gb - (match t.target_overlay.ov_stats.target_actual_size with + (match actual_size with | None -> () | Some actual_size -> List.push_back attrs ("ovf:size", Int64.to_string actual_size) ); - *) e "File" !attrs [] in append_child disk references; @@ -899,11 +891,16 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; ] in - (match actual_size_gb with - | None -> () - | Some actual_size_gb -> - List.push_back attrs ("ovf:actual_size", Int64.to_string actual_size_gb) - ); + (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. If + * we don't know the actual size, we must create the attribute with + * empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) + ); e "Disk" !attrs [] in append_child disk disk_section; diff --git a/output/output_rhv.ml b/output/output_rhv.ml index 4550dc4f306f..b902a7ee4619 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -183,7 +183,7 @@ and rhv_finalize dir source inspect target_meta (* Create the metadata. *) let ovf = Create_ovf.create_ovf source inspect target_meta sizes - output_alloc output_format esd_uuid image_uuids vol_uuids vm_uuid + output_alloc output_format esd_uuid image_uuids vol_uuids dir vm_uuid Create_ovf.RHVExportStorageDomain in (* Write it to the metadata file. *) diff --git a/output/output_rhv_upload.ml b/output/output_rhv_upload.ml index 64b88b67793c..89ff350be567 100644 --- a/output/output_rhv_upload.ml +++ b/output/output_rhv_upload.ml @@ -420,7 +420,7 @@ and rhv_upload_finalize dir source inspect target_meta (* Create the metadata. *) let ovf = Create_ovf.create_ovf source inspect target_meta disk_sizes - Sparse output_format sd_uuid disk_uuids vol_uuids vm_uuid + Sparse output_format sd_uuid disk_uuids vol_uuids dir vm_uuid OVirt in let ovf = DOM.doc_to_string ovf in diff --git a/output/output_vdsm.ml b/output/output_vdsm.ml index c9b552805ccd..ce0d5b5e4b2a 100644 --- a/output/output_vdsm.ml +++ b/output/output_vdsm.ml @@ -201,6 +201,7 @@ and vdsm_finalize dir source inspect target_meta output_alloc output_format dd_uuid image_uuids vol_uuids + dir vm_uuid ovf_flavour in diff --git a/tests/test-v2v-o-rhv.ovf.expected b/tests/test-v2v-o-rhv.ovf.expected index 5fda41c9488e..25e492fdbabc 100644 --- a/tests/test-v2v-o-rhv.ovf.expected +++ b/tests/test-v2v-o-rhv.ovf.expected @@ -2,7 +2,7 @@ <ovf:Envelope xmlns:rasd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Res... xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Vir... xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:ovf='http://schemas.dmtf.org/ovf/envelope/1/' xmlns:ovirt='http://www.ovirt.org/ovf' ovf:version='0.9'> <!-- generated by virt-v2v --> <References> - <File ovf:href='#DISK_ID#/#VOL_ID#' ovf:id='#VOL_ID#' ovf:description='generated by virt-v2v'/> + <File ovf:href='#DISK_ID#/#VOL_ID#' ovf:id='#VOL_ID#' ovf:description='generated by virt-v2v' ovf:size='#SIZE#'/> </References> <Section xsi:type='ovf:NetworkSection_Type'> <Info>List of networks</Info> @@ -10,7 +10,7 @@ </Section> <Section xsi:type='ovf:DiskSection_Type'> <Info>List of Virtual Disks</Info> - <Disk ovf:diskId='#VOL_ID#' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='#DISK_ID#/#VOL_ID#' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='RAW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True'/> + <Disk ovf:diskId='#VOL_ID#' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='#DISK_ID#/#VOL_ID#' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='RAW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True' ovf:actual_size='1'/> </Section> <Content ovf:id='out' xsi:type='ovf:VirtualSystem_Type'> <Name>windows</Name> diff --git a/tests/test-v2v-o-vdsm-options.ovf.expected b/tests/test-v2v-o-vdsm-options.ovf.expected index 23ca180f4c2f..bd5b5e7d38ec 100644 --- a/tests/test-v2v-o-vdsm-options.ovf.expected +++ b/tests/test-v2v-o-vdsm-options.ovf.expected @@ -2,7 +2,7 @@ <ovf:Envelope xmlns:rasd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Res... xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Vir... xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:ovf='http://schemas.dmtf.org/ovf/envelope/1/' xmlns:ovirt='http://www.ovirt.org/ovf' ovf:version='0.9'> <!-- generated by virt-v2v --> <References> - <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v'/> + <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v' ovf:size='#SIZE#'/> </References> <NetworkSection> <Info>List of networks</Info> @@ -10,7 +10,7 @@ </NetworkSection> <DiskSection> <Info>List of Virtual Disks</Info> - <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True'/> + <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True' ovf:actual_size='1'/> </DiskSection> <VirtualSystem ovf:id='VM'> <Name>windows</Name> -- 2.19.1.3.g30247aa5d201