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
