[pve-devel] [PATCH storage/qemu-server/manager v4] implement ova/ovf import for file based storages
Max Carrara
m.carrara at proxmox.com
Wed Jun 12 17:56:17 CEST 2024
On Fri May 24, 2024 at 3:21 PM CEST, Dominik Csapak wrote:
> This series enables importing ova/ovf from directory based storages,
> inclusive upload/download via the webui (ova only).
>
> It also improves the ovf importer by parsing the ostype, nics, bootorder
> (and firmware from vmware exported files).
Really great work! Built the packages and ran some tests. There are also
a couple comments, so see below.
Building
--------
* Patches applied cleanly
* Had to build the `qemu-server` package on a development VM and build
and install `pve-storage` with the patches installed there first,
because the new `PVE::GuestImport` module couldn't be found
(obviously). Worked fine though.
Testing
-------
* Installed all packages on a development VM, encountered no issues
* Checked if new `import` content type shows up only for directory-esque
storage types
--> sure does
* Added `import` as content type to my "local" storage
* Checked if "local" storage has new "import" tab
--> it does
* Exported a Debian VM from ESXi and tarballed the .ovf, .mf and .vmdk
to an .ova file
* Uploaded the file to the storage
--> succeeded
* Tried to import the VM onto my local lvm thin-pool storage
--> fails, because my "local" storage doesn't have the "Disk Image"
content type
I hadn't expected this to be honest, because I'm not trying to import it
to "local" here?
* Created a new storage with type "directory" and set its content types
to "import" and "disk image"
* Tried to import the VM again, but this time selected my LVM thin pool
as "Extraction Storage" in the dialogue window
--> also fails as expected, because an LVM thin pool is not file-based
A note on the above test: I'd prefer if the error message here was more
fine grained - the popup said:
> import-extraction-storage: local-lvm does not support 'images' content
> type or is not file based.
IMO it would probably be best to not display these storages at all if
they're not supported as an extraction target, but I'm not sure if that
can actually be done (conveniently) atm. Not a blocker or anything
though.
FYI, even though my development has multiple storage types, only the
following are shown in that drop down list:
- zfs
- lvm-thin
- directory (the new one, but not "local")
The only storages with the new "import" content type are the local one
and the new one I made for testing above, so I'm not sure why the zfs
pool and lvm thin pool are showing up there.
* Attempted another import, this time just keeping the "Extraction
Storage" as is
--> Worked quite fast, the VM was online really quick because I had
live import on and worked without any issues
* Attempted another import, but this time from the "local" storage again
as I did earlier, but I set the "Extraction Storage" to the new
directory storage
--> This time importing from "local" worked, somewhat surprisingly
* On the new directory storage, set the content type to "import" only
--> The icon changed to the little cloud with the arrow, cute
* Attempted yet another import, this time from "local" again and setting
the new directory storage as "Extraction Storage"
--> fails unexpectedly with:
> scsi0: test-dir-storage:import/debian-esxi.ova/debian-esxi-0.vmdk is
> not on an storage with 'images' content type and no
> 'import-extraction-storage' was given."
So, it's now pretty clear to me that the `disks` content type plays a
role in terms of whether something can actually be extracted or not
here. Is there perhaps any way the extraction can be "decoupled" from
the content type or be done in a different manner? If this is currently
a limitation (e.g. because of the storage API) then I'd say it's an
*okay* restriction to have at the moment, but it still feels a little
unintuitive from a user's perspective.
Code Review
-----------
The patches are rather easy to follow and logically structured, so
that's very nice. There are more comments inline.
The only other thing I'm not sure about is whether we'd actually want
the `PVE::GuestImport` module (or namespace) -- is there anything we'd
like to import in the future? Maybe something like `PVE::Import::Guest`
would be better, even if there's nothing in `PVE::Import` for the time
being. Just so that we don't have to touch the module structure again
soon.
Conclusion
----------
Very solid work, this is pretty great! This will be a lovely feature for
our users. Apart from the couple little things I encountered there's
nothing to complain about.
To sum all of the above up, the only suggestions I have are as follows:
* I think the error handling regarding the "Extraction Storage" should
be more fine-grained
* The `disks` content type shouldn't determine whether an .ova file is
able to be extracted or not, I think there should be some other
mechanism for that
* Maybe use `PVE::Import::Guest` instead of `PVE::GuestImport`
>
> I opted to move the OVF.pm to pve-storage, since there is no
> real other place where we could put it. I put it in a new module
> 'GuestImport'
>
> We now extract the images into either a given target storage or in the
> import storage in the 'images' dir so accidentally left over images
> are discoverable by the ui/cli.
>
> changes from v3:
> * fixed dependencies in control file
> * removed unnecessary use statements
> * removed unnecessary remove helper
> * moved 'needs_extract' helper to qemu-server
> * removed import storage param from PUT call
> * check down/uploaded ova filename more strictly (same as listing)
> * improved filepath checking in ovf
> * forbid importing when extracted image references a base/backing file
> * instead of trying to manually create a proper filename, use 'alloc' to
> create a small (1M) file with the same format and overwrite it with
> renaming. this also solves the cluster locking issue
> * prefer using PVE::Storage functions instead of plugin methods in
> ova extraction code
> * use $vollist for cleaning up extracted images in qemu-server and
> add manual cleanup for the success case
>
> changes from v2:
> * use better 'format' values for embedded images (e.g. ova+vmdk)
> * use this format to decide if images should be extracted
> * consistent use of the 'safe character' classes when listing
> and parsing
> * also list vmdk/qcow2/raw images in content listing
> (this will be useful when we have a gui for the 'import-from'
> in the wizard/disk edit for vms)
> * a few gui adaptions
>
>
> changes from v1:
> * move ovf code to GuestImport
> * move extract/checking code to GuestImport
> * don't return 'image' types from import volumes
> * use allow 'safe' characters for filenames of ova/ovfs and inside
> * check for non-regular files (e.g. symlinks) after extraction
> * add new 'import-extraction-storage' for import
> * rename panel in gui for directory storages
> * typo fixes
> * and probably more, see the individual patches for details
>
> pve-storage:
>
> Dominik Csapak (12):
> copy OVF.pm from qemu-server
> plugin: dir: implement import content type
> plugin: dir: handle ova files for import
> ovf: improve and simplify path checking code
> ovf: implement parsing the ostype
> ovf: implement parsing out firmware type
> ovf: implement rudimentary boot order
> ovf: implement parsing nics
> api: allow ova upload/download
> plugin: enable import for nfs/btrfs/cifs/cephfs/glusterfs
> add 'import' content type to 'check_volume_access'
> plugin: file_size_info: don't ignore base path with whitespace
>
> debian/control | 2 +
> src/PVE/API2/Storage/Status.pm | 19 +-
> src/PVE/GuestImport.pm | 77 ++++
> src/PVE/GuestImport/Makefile | 3 +
> src/PVE/GuestImport/OVF.pm | 383 ++++++++++++++++++
> src/PVE/Makefile | 2 +
> src/PVE/Storage.pm | 23 +-
> src/PVE/Storage/BTRFSPlugin.pm | 5 +
> src/PVE/Storage/CIFSPlugin.pm | 6 +-
> src/PVE/Storage/CephFSPlugin.pm | 6 +-
> src/PVE/Storage/DirPlugin.pm | 52 ++-
> src/PVE/Storage/GlusterfsPlugin.pm | 6 +-
> src/PVE/Storage/Makefile | 1 +
> src/PVE/Storage/NFSPlugin.pm | 6 +-
> src/PVE/Storage/Plugin.pm | 17 +-
> src/test/Makefile | 5 +-
> src/test/ovf_manifests/Win10-Liz-disk1.vmdk | Bin 0 -> 65536 bytes
> src/test/ovf_manifests/Win10-Liz.ovf | 142 +++++++
> .../ovf_manifests/Win10-Liz_no_default_ns.ovf | 143 +++++++
> .../ovf_manifests/Win_2008_R2_two-disks.ovf | 145 +++++++
> src/test/ovf_manifests/disk1.vmdk | Bin 0 -> 65536 bytes
> src/test/ovf_manifests/disk2.vmdk | Bin 0 -> 65536 bytes
> src/test/parse_volname_test.pm | 33 ++
> src/test/path_to_volume_id_test.pm | 21 +
> src/test/run_ovf_tests.pl | 85 ++++
> 25 files changed, 1169 insertions(+), 13 deletions(-)
> create mode 100644 src/PVE/GuestImport.pm
> create mode 100644 src/PVE/GuestImport/Makefile
> create mode 100644 src/PVE/GuestImport/OVF.pm
> create mode 100644 src/test/ovf_manifests/Win10-Liz-disk1.vmdk
> create mode 100755 src/test/ovf_manifests/Win10-Liz.ovf
> create mode 100755 src/test/ovf_manifests/Win10-Liz_no_default_ns.ovf
> create mode 100755 src/test/ovf_manifests/Win_2008_R2_two-disks.ovf
> create mode 100644 src/test/ovf_manifests/disk1.vmdk
> create mode 100644 src/test/ovf_manifests/disk2.vmdk
> create mode 100755 src/test/run_ovf_tests.pl
>
> qemu-server:
>
> Dominik Csapak (4):
> api: delete unused OVF.pm
> use OVF from Storage
> api: create: implement extracting disks when needed for import-from
> api: create: add 'import-extraction-storage' parameter
>
> PVE/API2/Qemu.pm | 91 +++++--
> PVE/API2/Qemu/Makefile | 2 +-
> PVE/API2/Qemu/OVF.pm | 53 ----
> PVE/CLI/qm.pm | 4 +-
> PVE/QemuServer.pm | 12 +
> PVE/QemuServer/Helpers.pm | 5 +
> PVE/QemuServer/Makefile | 1 -
> PVE/QemuServer/OVF.pm | 242 ------------------
> debian/control | 2 -
> test/Makefile | 5 +-
> test/ovf_manifests/Win10-Liz-disk1.vmdk | Bin 65536 -> 0 bytes
> test/ovf_manifests/Win10-Liz.ovf | 142 ----------
> .../ovf_manifests/Win10-Liz_no_default_ns.ovf | 142 ----------
> test/ovf_manifests/Win_2008_R2_two-disks.ovf | 145 -----------
> test/ovf_manifests/disk1.vmdk | Bin 65536 -> 0 bytes
> test/ovf_manifests/disk2.vmdk | Bin 65536 -> 0 bytes
> test/run_ovf_tests.pl | 71 -----
> 17 files changed, 97 insertions(+), 820 deletions(-)
> delete mode 100644 PVE/API2/Qemu/OVF.pm
> delete mode 100644 PVE/QemuServer/OVF.pm
> delete mode 100644 test/ovf_manifests/Win10-Liz-disk1.vmdk
> delete mode 100755 test/ovf_manifests/Win10-Liz.ovf
> delete mode 100755 test/ovf_manifests/Win10-Liz_no_default_ns.ovf
> delete mode 100755 test/ovf_manifests/Win_2008_R2_two-disks.ovf
> delete mode 100644 test/ovf_manifests/disk1.vmdk
> delete mode 100644 test/ovf_manifests/disk2.vmdk
> delete mode 100755 test/run_ovf_tests.pl
>
> pve-manager:
>
> Dominik Csapak (9):
> ui: fix special 'import' icon for non-esxi storages
> ui: guest import: add ova-needs-extracting warning text
> ui: enable import content type for relevant storages
> ui: enable upload/download/remove buttons for 'import' type storages
> ui: disable 'import' button for non importable formats
> ui: import: improve rendering of volume names
> ui: guest import: add storage selector for ova extraction storage
> ui: guest import: change icon/text for non-esxi import storage
> ui: import: show size for dir-based storages
>
> www/manager6/Utils.js | 11 +++++++++--
> www/manager6/form/ContentTypeSelector.js | 2 +-
> www/manager6/storage/Browser.js | 25 ++++++++++++++++++------
> www/manager6/storage/CephFSEdit.js | 2 +-
> www/manager6/storage/GlusterFsEdit.js | 2 +-
> www/manager6/window/GuestImport.js | 24 +++++++++++++++++++++++
> www/manager6/window/UploadToStorage.js | 1 +
> 7 files changed, 56 insertions(+), 11 deletions(-)
More information about the pve-devel
mailing list