Re: [pve-devel] [PATCH storage/qemu-server/manager v4] implement ova/ovf import for file based storages

["Max Carrara" <m.carrara@proxmox.com>]

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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel