Re: [pve-devel] [PATCH storage v4 03/12] plugin: dir: handle ova files for import

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

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 <d.csapak@proxmox.com>
> ---
>  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