From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id CED6F1FF391 for ; Wed, 12 Jun 2024 17:56:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EBCF0147C0; Wed, 12 Jun 2024 17:57:30 +0200 (CEST) Date: Wed, 12 Jun 2024 17:56:55 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240524132209.703402-1-d.csapak@proxmox.com> <20240524132209.703402-4-d.csapak@proxmox.com> In-Reply-To: <20240524132209.703402-4-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.034 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH storage v4 03/12] plugin: dir: handle ova files for import X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Fri May 24, 2024 at 3:21 PM CEST, Dominik Csapak wrote: > since we want to handle ova files (which are only ovf+images bundled in > a tar file) for import, add code that handles that. > > we introduce a valid volname for files contained in ovas like this: > > storage:import/archive.ova/disk-1.vmdk > > by basically treating the last part of the path as the name for the > contained disk we want. > > in that case we return 'import' as type with 'vmdk/qcow2/raw' as format > (we cannot use something like 'ova+vmdk' without extending the 'format' > parsing to that for all storages/formats. This is because it runs > though a verify format check at least once) > > we then provide 3 functions to use for that: > > * copy_needs_extraction: determines from the given volid (like above) if > that needs extraction to copy it, currently only 'import' vtype + a > volid with the above format returns true > > * extract_disk_from_import_file: this actually extracts the file from > the archive. Currently only ova is supported, so the extraction with > 'tar' is hardcoded, but again we can easily extend/modify that should > we need to. > > we currently extract into the either the import storage or a given > target storage in the images directory so if the cleanup does not > happen, the user can still see and interact with the image via > api/cli/gui > > * cleanup_extracted_image: intended to cleanup the extracted images from > above > > we have to modify the `parse_ovf` a bit to handle the missing disk > images, and we parse the size out of the ovf part (since this is > informal only, it should be no problem if we cannot parse it sometimes) > > Signed-off-by: Dominik Csapak > --- > src/PVE/API2/Storage/Status.pm | 1 + > src/PVE/GuestImport.pm | 77 ++++++++++++++++++++++++++++++ > src/PVE/GuestImport/OVF.pm | 52 +++++++++++++++++--- > src/PVE/Makefile | 1 + > src/PVE/Storage.pm | 4 +- > src/PVE/Storage/DirPlugin.pm | 15 +++++- > src/PVE/Storage/Plugin.pm | 4 ++ > src/test/parse_volname_test.pm | 20 ++++++++ > src/test/path_to_volume_id_test.pm | 8 ++++ > 9 files changed, 173 insertions(+), 9 deletions(-) > create mode 100644 src/PVE/GuestImport.pm > > diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm > index dc6cc69..acde730 100644 > --- a/src/PVE/API2/Storage/Status.pm > +++ b/src/PVE/API2/Storage/Status.pm > @@ -749,6 +749,7 @@ __PACKAGE__->register_method({ > 'efi-state-lost', > 'guest-is-running', > 'nvme-unsupported', > + 'ova-needs-extracting', > 'ovmf-with-lsi-unsupported', > 'serial-port-socket-only', > ], > diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm > new file mode 100644 > index 0000000..988d1f6 > --- /dev/null > +++ b/src/PVE/GuestImport.pm > @@ -0,0 +1,77 @@ > +package PVE::GuestImport; > + > +use strict; > +use warnings; > + > +use File::Path; > + > +use PVE::Storage; > +use PVE::Tools qw(run_command); > + > +sub extract_disk_from_import_file { > + my ($volid, $vmid, $target_storeid) = @_; > + > + my ($source_storeid, $volname) = PVE::Storage::parse_volume_id($volid); > + $target_storeid //= $source_storeid; > + my $cfg = PVE::Storage::config(); > + > + my ($vtype, $name, undef, undef, undef, undef, $fmt) = > + PVE::Storage::parse_volname($cfg, $volid); > + > + die "only files with content type 'import' can be extracted\n" > + if $vtype ne 'import' || $fmt !~ m/^ova\+/; > + > + # extract the inner file from the name > + my $archive_volid; > + my $inner_file; > + my $inner_fmt; > + if ($name =~ m!^(.*\.ova)/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$!) { > + $archive_volid = "$source_storeid:import/$1"; > + $inner_file = $2; > + ($inner_fmt) = $fmt =~ /^ova\+(.*)$/; > + } else { > + die "cannot extract $volid - invalid volname $volname\n"; > + } > + > + my $ova_path = PVE::Storage::path($cfg, $archive_volid); > + > + my $tmpdir = PVE::Storage::get_image_dir($cfg, $target_storeid, $vmid); > + my $pid = $$; > + $tmpdir .= "/tmp_${pid}_${vmid}"; > + mkpath $tmpdir; > + > + ($ova_path) = $ova_path =~ m|^(.*)$|; # untaint > + > + my $source_path = "$tmpdir/$inner_file"; > + my $target_path; > + my $target_volid; > + eval { > + run_command(['tar', '-x', '--force-local', '-C', $tmpdir, '-f', $ova_path, $inner_file]); > + > + # check for symlinks and other non regular files > + if (-l $source_path || ! -f $source_path) { > + die "only regular files are allowed\n"; > + } > + > + # TODO check for base images in file > + > + # create temporary 1M image that will get overwritten by the rename > + # to reserve the filename and take care of locking > + $target_volid = PVE::Storage::vdisk_alloc($cfg, $target_storeid, $vmid, $inner_fmt, undef, 1024); > + $target_path = PVE::Storage::path($cfg, $target_volid); > + > + print "renaming $source_path to $target_path\n"; > + > + rename($source_path, $target_path) or die "unable to move - $!\n"; > + }; > + if (my $err = $@) { > + File::Path::remove_tree($tmpdir); > + die "error during extraction: $err\n"; > + } > + > + File::Path::remove_tree($tmpdir); > + > + return $target_volid; > +} > + > +1; > diff --git a/src/PVE/GuestImport/OVF.pm b/src/PVE/GuestImport/OVF.pm > index 899cfdc..0728684 100644 > --- a/src/PVE/GuestImport/OVF.pm > +++ b/src/PVE/GuestImport/OVF.pm > @@ -84,11 +84,37 @@ sub id_to_pve { > } > } > > +# technically defined in DSP0004 (https://www.dmtf.org/dsp/DSP0004) as an ABNF > +# but realistically this always takes the form of 'byte * base^exponent' > +sub try_parse_capacity_unit { > + my ($unit_text) = @_; > + > + if ($unit_text =~ m/^\s*byte\s*\*\s*([0-9]+)\s*\^\s*([0-9]+)\s*$/) { > + my $base = $1; > + my $exp = $2; > + return $base ** $exp; > + } > + > + return undef; > +} > + > # returns two references, $qm which holds qm.conf style key/values, and \@disks > sub parse_ovf { > - my ($ovf, $debug) = @_; > + my ($ovf, $isOva, $debug) = @_; > + > + # we have to ignore missing disk images for ova > + my $dom; > + if ($isOva) { > + my $raw = ""; > + PVE::Tools::run_command(['tar', '-xO', '--wildcards', '--occurrence=1', '-f', $ovf, '*.ovf'], outfunc => sub { > + my $line = shift; > + $raw .= $line; > + }) Style: Perhaps this could be something like this: PVE::Tools::run_command( ['tar', '-xO', '--wildcards', '--occurrence=1', '-f', $ovf, '*.ovf'], outfunc => sub { my $line = shift; $raw .= $line; }, ) > + $dom = XML::LibXML->load_xml(string => $raw, no_blanks => 1); > + } else { > + $dom = XML::LibXML->load_xml(location => $ovf, no_blanks => 1); > + } > > - my $dom = XML::LibXML->load_xml(location => $ovf, no_blanks => 1); > > # register the xml namespaces in a xpath context object > # 'ovf' is the default namespace so it will prepended to each xml element > @@ -176,7 +202,17 @@ sub parse_ovf { > # @ needs to be escaped to prevent Perl double quote interpolation > my $xpath_find_fileref = sprintf("/ovf:Envelope/ovf:DiskSection/\ > ovf:Disk[\@ovf:diskId='%s']/\@ovf:fileRef", $disk_id); > + my $xpath_find_capacity = sprintf("/ovf:Envelope/ovf:DiskSection/\ > +ovf:Disk[\@ovf:diskId='%s']/\@ovf:capacity", $disk_id); > + my $xpath_find_capacity_unit = sprintf("/ovf:Envelope/ovf:DiskSection/\ > +ovf:Disk[\@ovf:diskId='%s']/\@ovf:capacityAllocationUnits", $disk_id); > my $fileref = $xpc->findvalue($xpath_find_fileref); > + my $capacity = $xpc->findvalue($xpath_find_capacity); > + my $capacity_unit = $xpc->findvalue($xpath_find_capacity_unit); > + my $virtual_size; > + if (my $factor = try_parse_capacity_unit($capacity_unit)) { > + $virtual_size = $capacity * $factor; > + } > > my $valid_url_chars = qr@${valid_uripath_chars}|/@; > if (!$fileref || $fileref !~ m/^${valid_url_chars}+$/) { > @@ -216,7 +252,7 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id); > die "error parsing $filepath, are you using a symlink ?\n"; > } > > - if (!-e $backing_file_path) { > + if (!-e $backing_file_path && !$isOva) { > die "error parsing $filepath, file seems not to exist at $backing_file_path\n"; > } > > @@ -224,16 +260,20 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id); > ($filepath) = $filepath =~ m|^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/parent dirs > die "invalid path\n" if !$filepath; > > - my $virtual_size = PVE::Storage::file_size_info($backing_file_path); > - die "error parsing $backing_file_path, cannot determine file size\n" > - if !$virtual_size; > + if (!$isOva) { > + my $size = PVE::Storage::file_size_info($backing_file_path); > + die "error parsing $backing_file_path, cannot determine file size\n" > + if !$size; > > + $virtual_size = $size; > + } > $pve_disk = { > disk_address => $pve_disk_address, > backing_file => $backing_file_path, > virtual_size => $virtual_size, > relative_path => $filepath, > }; > + $pve_disk->{virtual_size} = $virtual_size if defined($virtual_size); > push @disks, $pve_disk; > > } > diff --git a/src/PVE/Makefile b/src/PVE/Makefile > index e15a275..0af3081 100644 > --- a/src/PVE/Makefile > +++ b/src/PVE/Makefile > @@ -5,6 +5,7 @@ install: > install -D -m 0644 Storage.pm ${DESTDIR}${PERLDIR}/PVE/Storage.pm > install -D -m 0644 Diskmanage.pm ${DESTDIR}${PERLDIR}/PVE/Diskmanage.pm > install -D -m 0644 CephConfig.pm ${DESTDIR}${PERLDIR}/PVE/CephConfig.pm > + install -D -m 0644 GuestImport.pm ${DESTDIR}${PERLDIR}/PVE/GuestImport.pm > make -C Storage install > make -C GuestImport install > make -C API2 install > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 1ed91c2..bd6c4df 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -114,10 +114,12 @@ our $VZTMPL_EXT_RE_1 = qr/\.tar\.(gz|xz|zst)/i; > > our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/; > > -our $IMPORT_EXT_RE_1 = qr/\.(ovf|qcow2|raw|vmdk)/; > +our $IMPORT_EXT_RE_1 = qr/\.(ova|ovf|qcow2|raw|vmdk)/; > > our $SAFE_CHAR_CLASS_RE = qr/[a-zA-Z0-9\-\.\+\=\_]/; > > +our $OVA_CONTENT_RE_1 = qr/${SAFE_CHAR_CLASS_RE}+\.(qcow2|raw|vmdk)/; > + > # FIXME remove with PVE 8.0, add versioned breaks for pve-manager > our $vztmpl_extension_re = $VZTMPL_EXT_RE_1; > > diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm > index 3e3b1e7..ea89464 100644 > --- a/src/PVE/Storage/DirPlugin.pm > +++ b/src/PVE/Storage/DirPlugin.pm > @@ -258,15 +258,26 @@ sub get_import_metadata { > # NOTE: all types of warnings must be added to the return schema of the import-metadata API endpoint > my $warnings = []; > > + my $isOva = 0; > + if ($name =~ m/\.ova$/) { > + $isOva = 1; > + push @$warnings, { type => 'ova-needs-extracting' }; > + } > my $path = $class->path($scfg, $volname, $storeid, undef); > - my $res = PVE::GuestImport::OVF::parse_ovf($path); > + my $res = PVE::GuestImport::OVF::parse_ovf($path, $isOva); > my $disks = {}; > for my $disk ($res->{disks}->@*) { > my $id = $disk->{disk_address}; > my $size = $disk->{virtual_size}; > my $path = $disk->{relative_path}; > + my $volid; > + if ($isOva) { > + $volid = "$storeid:$volname/$path"; > + } else { > + $volid = "$storeid:import/$path", > + } > $disks->{$id} = { > - volid => "$storeid:import/$path", > + volid => $volid, > defined($size) ? (size => $size) : (), > }; > } > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index fc30894..848e053 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -654,6 +654,10 @@ sub parse_volname { > return ('backup', $fn); > } elsif ($volname =~ m!^snippets/([^/]+)$!) { > return ('snippets', $1); > + } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+\.ova\/${PVE::Storage::OVA_CONTENT_RE_1})$!) { > + my $archive = $1; > + my $format = $2; > + return ('import', $archive, undef, undef, undef, undef, "ova+$format"); > } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!) { > return ('import', $1, undef, undef, undef, undef, $2); > } > diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm > index 0367b83..bc7b4e8 100644 > --- a/src/test/parse_volname_test.pm > +++ b/src/test/parse_volname_test.pm > @@ -83,11 +83,31 @@ my $tests = [ > # > # Import > # > + { > + description => "Import, ova", > + volname => 'import/import.ova', > + expected => ['import', 'import.ova', undef, undef, undef ,undef, 'ova'], > + }, > { > description => "Import, ovf", > volname => 'import/import.ovf', > expected => ['import', 'import.ovf', undef, undef, undef ,undef, 'ovf'], > }, > + { > + description => "Import, innner file of ova", > + volname => 'import/import.ova/disk.qcow2', > + expected => ['import', 'import.ova/disk.qcow2', undef, undef, undef, undef, 'ova+qcow2'], > + }, > + { > + description => "Import, innner file of ova", > + volname => 'import/import.ova/disk.vmdk', > + expected => ['import', 'import.ova/disk.vmdk', undef, undef, undef, undef, 'ova+vmdk'], > + }, > + { > + description => "Import, innner file of ova", > + volname => 'import/import.ova/disk.raw', > + expected => ['import', 'import.ova/disk.raw', undef, undef, undef, undef, 'ova+raw'], > + }, > # > # failed matches > # > diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm > index 0e0c15f..0d238f9 100644 > --- a/src/test/path_to_volume_id_test.pm > +++ b/src/test/path_to_volume_id_test.pm > @@ -174,6 +174,14 @@ my @tests = ( > 'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz', > ], > }, > + { > + description => 'Import, ova', > + volname => "$storage_dir/import/import.ova", > + expected => [ > + 'import', > + 'local:import/import.ova', > + ], > + }, > { > description => 'Import, ovf', > volname => "$storage_dir/import/import.ovf", _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel