[pve-devel] [PATCH storage/qemu-server/manager v4] implement ova/ovf import for file based storages
Dominik Csapak
d.csapak at proxmox.com
Thu Jun 13 12:52:25 CEST 2024
On 6/12/24 17:56, Max Carrara wrote:
> 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?
by default the images are extracted onto the import storage, so it
needs the disk images too in case they're left over, so users
have an easy way to clean them up.
you can give an explicit extraction storage though (that too
needs 'images' as content type)
>
> * 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.
sure, that should be possible to filter out somehow
>
> 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.
>
because they have 'images' enabled, as written above i can see that
i filter those out better
> * 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
because the extracting storage supported 'images'
>
> * 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.
as said above it's done so in case an image is left over, the user has an 'out' and
is able to see the images in the ui
we could allow it simply for any file based storages, but without the
'images' content type the user wouldn't see left over images and not notice
the used space (i can practically see the forum threads with:
'why is my storage full?')
(it shouldn't happen, but better safe than sorry)
>
> 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.
i don't think we'll import anything other than guests on PVE soon,
but i have no hard feelings about the name (i think it was a suggestion
from thomas previously)
>
> 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
sure that should be possible
> * 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
not quite sure about that (see my notes above)
> * Maybe use `PVE::Import::Guest` instead of `PVE::GuestImport`
idc really, maybe someone else wants to chime in here?
>
>>
>> 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(-)
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list