[pve-devel] [PATCH storage/qemu-server/manager v4] implement ova/ovf import for file based storages
Max Carrara
m.carrara at proxmox.com
Fri Jun 14 10:57:12 CEST 2024
On Thu Jun 13, 2024 at 12:52 PM CEST, Dominik Csapak wrote:
> 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.
Ah, I hadn't considered that. That makes more sense, then. IIRC in my
tests there were no disk images left behind, so that's why I was
wondering why the extra content type was necessary.
>
> 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
Sweet! Think that might avoid some noise in the forum as well. ;)
>
> >
> > 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)
Yeah, I completely get that rationale. In that case I'm fine with
keeping it that way, but I think we should document that at some point,
so users aren't confused about the import / disks content type
"interplay", if that makes sense.
Would it also make sense to extract the disks to /tmp? Since that's
periodically cleaned up by default. Though, that could clog up the root
FS nevertheless ...
>
> >
> > 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