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 346AA1FF348 for ; Wed, 17 Apr 2024 15:13:43 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8798475F8; Wed, 17 Apr 2024 15:13:43 +0200 (CEST) Message-ID: Date: Wed, 17 Apr 2024 15:13:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Fiona Ebner , Proxmox VE development discussion References: <20240416131909.2867605-1-d.csapak@proxmox.com> <20240416131909.2867605-3-d.csapak@proxmox.com> <0991e935-e29f-4459-9872-5da17cb7e929@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <0991e935-e29f-4459-9872-5da17cb7e929@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.014 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [dirplugin.pm, storage.pm, plugin.pm, ovf.pm] Subject: Re: [pve-devel] [PATCH storage 2/9] plugin: dir: implement import content type 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 4/17/24 12:07, Fiona Ebner wrote: > Am 16.04.24 um 15:18 schrieb Dominik Csapak: >> in DirPlugin and not Plugin (because of cyclic dependency of >> Plugin -> OVF -> Storage -> Plugin otherwise) >> >> only ovf is currently supported (though ova will be shown in import >> listing), expects the files to not be in a subdir, and adjacent to the >> ovf file. >> >> Signed-off-by: Dominik Csapak >> --- >> src/PVE/Storage.pm | 8 ++++++- >> src/PVE/Storage/DirPlugin.pm | 37 +++++++++++++++++++++++++++++- >> src/PVE/Storage/OVF.pm | 2 ++ >> src/PVE/Storage/Plugin.pm | 18 ++++++++++++++- >> src/test/parse_volname_test.pm | 13 +++++++++++ >> src/test/path_to_volume_id_test.pm | 16 +++++++++++++ >> 6 files changed, 91 insertions(+), 3 deletions(-) >> >> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm >> index 40314a8..f8ea93d 100755 >> --- a/src/PVE/Storage.pm >> +++ b/src/PVE/Storage.pm >> @@ -114,6 +114,8 @@ 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/\.(ov[af])/; >> + >> # FIXME remove with PVE 8.0, add versioned breaks for pve-manager >> our $vztmpl_extension_re = $VZTMPL_EXT_RE_1; >> >> @@ -612,6 +614,7 @@ sub path_to_volume_id { >> my $backupdir = $plugin->get_subdir($scfg, 'backup'); >> my $privatedir = $plugin->get_subdir($scfg, 'rootdir'); >> my $snippetsdir = $plugin->get_subdir($scfg, 'snippets'); >> + my $importdir = $plugin->get_subdir($scfg, 'import'); >> >> if ($path =~ m!^$imagedir/(\d+)/([^/\s]+)$!) { >> my $vmid = $1; >> @@ -640,6 +643,9 @@ sub path_to_volume_id { >> } elsif ($path =~ m!^$snippetsdir/([^/]+)$!) { >> my $name = $1; >> return ('snippets', "$sid:snippets/$name"); >> + } elsif ($path =~ m!^$importdir/([^/]+${IMPORT_EXT_RE_1})$!) { >> + my $name = $1; >> + return ('import', "$sid:import/$name"); >> } >> } >> >> @@ -2170,7 +2176,7 @@ sub normalize_content_filename { >> # If a storage provides an 'import' content type, it should be able to provide >> # an object implementing the import information interface. >> sub get_import_metadata { >> - my ($cfg, $volid) = @_; >> + my ($cfg, $volid, $target) = @_; >> > > $target is added here but not passed along when calling the plugin's > function leftover from a previous iteration of the patches > >> my ($storeid, $volname) = parse_volume_id($volid); >> > > Pre-existing and not directly related, but in the ESXi plugin the > prototype seems wrong too: > > sub get_import_metadata : prototype($$$$$) { > my ($class, $scfg, $volname, $storeid) = @_; same here > > >> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm >> index 2efa8d5..4dc7708 100644 >> --- a/src/PVE/Storage/DirPlugin.pm >> +++ b/src/PVE/Storage/DirPlugin.pm >> @@ -10,6 +10,7 @@ use IO::File; >> use POSIX; >> >> use PVE::Storage::Plugin; >> +use PVE::Storage::OVF; >> use PVE::JSONSchema qw(get_standard_option); >> >> use base qw(PVE::Storage::Plugin); >> @@ -22,7 +23,7 @@ sub type { >> >> sub plugindata { >> return { >> - content => [ { images => 1, rootdir => 1, vztmpl => 1, iso => 1, backup => 1, snippets => 1, none => 1 }, >> + content => [ { images => 1, rootdir => 1, vztmpl => 1, iso => 1, backup => 1, snippets => 1, none => 1, import => 1 }, >> { images => 1, rootdir => 1 }], >> format => [ { raw => 1, qcow2 => 1, vmdk => 1, subvol => 1 } , 'raw' ], >> }; >> @@ -247,4 +248,38 @@ sub check_config { >> return $opts; >> } >> >> +sub get_import_metadata { >> + my ($class, $scfg, $volname, $storeid, $target) = @_; >> + >> + if ($volname !~ m!^([^/]+)/.*${PVE::Storage::IMPORT_EXT_RE_1}$!) { >> + die "volume '$volname' does not look like an importable vm config\n"; >> + } >> + >> + my $path = $class->path($scfg, $volname, $storeid, undef); >> + >> + # NOTE: all types must be added to the return schema of the import-metadata API endpoint > > To be extra clear (was confused for a moment): "all types of warnings" > >> + my $warnings = []; >> + >> + my $res = PVE::Storage::OVF::parse_ovf($path, $isOva); > > $isOva does not exist yet (only added by a later patch). > >> + my $disks = {}; >> + for my $disk ($res->{disks}->@*) { >> + my $id = $disk->{disk_address}; >> + my $size = $disk->{virtual_size}; >> + my $path = $disk->{relative_path}; >> + $disks->{$id} = { >> + volid => "$storeid:import/$path", >> + defined($size) ? (size => $size) : (), >> + }; >> + } >> + >> + return { >> + type => 'vm', >> + source => $volname, >> + 'create-args' => $res->{qm}, >> + 'disks' => $disks, >> + warnings => $warnings, >> + net => [], >> + }; >> +} >> + >> 1; >> diff --git a/src/PVE/Storage/OVF.pm b/src/PVE/Storage/OVF.pm >> index 90ca453..4a322b9 100644 >> --- a/src/PVE/Storage/OVF.pm >> +++ b/src/PVE/Storage/OVF.pm >> @@ -222,6 +222,7 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id); >> } >> >> ($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint >> + ($filepath) = $filepath =~ m|^(.*)|; # untaint > > > Hmm, $backing_file_path is the result after going through realpath(), > $filepath is from before. We do check it's not a symlink, so I might be > a bit paranoid, but still, rather than doing a blanket untaint, you > could just use basename() (either here or not return anything new and do > it at the use-site). > >> >> my $virtual_size = PVE::Storage::file_size_info($backing_file_path); >> die "error parsing $backing_file_path, cannot determine file size\n" >> @@ -231,6 +232,7 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id); >> disk_address => $pve_disk_address, >> backing_file => $backing_file_path, >> virtual_size => $virtual_size >> + relative_path => $filepath, >> }; >> push @disks, $pve_disk; >> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm >> index 22a9729..deaf8b2 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::IMPORT_EXT_RE_1)$!) { >> + return ('import', $1); >> + } elsif ($volname =~ m!^import/([^/]+\.(raw|vmdk|qcow2))$!) { >> + return ('images', $1, 0, undef, undef, undef, $2); > > Hmm, $vmid=0, because we have currently have assumptions that each > volume has an associated guest ID? At least might be worth a comment > (also in API description if those volumes can somehow reach there). i mimicked the way ESXIPlugin handles that, there we also do the same with vmdks vs vmx files i.e. vmdks are returned as 'images' with a format and vmx files are just returned plain without a file format i can change it for the dir plugin, since i have to handle the ova contained images differently anyway and that requires adaption in qemu-server then we can simply do that check there differently (vtype import + fileformat vmdk/qcow2/raw for example) > >> } >> >> die "unable to parse directory volume name '$volname'\n"; >> @@ -666,6 +670,7 @@ my $vtype_subdirs = { >> vztmpl => 'template/cache', >> backup => 'dump', >> snippets => 'snippets', >> + import => 'import', >> }; >> >> sub get_vtype_subdirs { >> @@ -691,6 +696,11 @@ sub filesystem_path { >> my ($vtype, $name, $vmid, undef, undef, $isBase, $format) = >> $class->parse_volname($volname); >> >> + if (defined($vmid) && $vmid == 0) { >> + # import volumes? >> + $vtype = 'import'; >> + } > > It is rather hacky :/ At least we could check whether it's an volname > with "import/" instead of relying on $vmid==0 to set the $vtype. > > But why return type 'images' in parse_volname() if you override it here > if it's an import image? There should be some comments with the > rationale why it's done like this. > not needed when i return the images as 'import' type with a file format >> + >> # Note: qcow2/qed has internal snapshot, so path is always >> # the same (with or without snapshot => same file). >> die "can't snapshot this image format\n" >> @@ -1227,7 +1237,7 @@ sub list_images { >> return $res; >> } >> >> -# list templates ($tt = ) >> +# list templates ($tt = ) >> my $get_subdir_files = sub { >> my ($sid, $path, $tt, $vmid) = @_; >> >> @@ -1283,6 +1293,10 @@ my $get_subdir_files = sub { >> volid => "$sid:snippets/". basename($fn), >> format => 'snippet', >> }; >> + } elsif ($tt eq 'import') { >> + next if $fn !~ m!/([^/]+$PVE::Storage::IMPORT_EXT_RE_1)$!i; >> + >> + $info = { volid => "$sid:import/$1", format => "$2" }; >> } >> >> $info->{size} = $st->size; >> @@ -1317,6 +1331,8 @@ sub list_volumes { >> $data = $get_subdir_files->($storeid, $path, 'backup', $vmid); >> } elsif ($type eq 'snippets') { >> $data = $get_subdir_files->($storeid, $path, 'snippets'); >> + } elsif ($type eq 'import') { >> + $data = $get_subdir_files->($storeid, $path, 'import'); >> } >> } >> >> diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm >> index d6ac885..59819f0 100644 >> --- a/src/test/parse_volname_test.pm >> +++ b/src/test/parse_volname_test.pm >> @@ -81,6 +81,19 @@ my $tests = [ >> expected => ['snippets', 'hookscript.pl'], >> }, >> # >> + # >> + # >> + { >> + description => "Import, ova", >> + volname => 'import/import.ova', >> + expected => ['import', 'import.ova'], >> + }, >> + { >> + description => "Import, ovf", >> + volname => 'import/import.ovf', >> + expected => ['import', 'import.ovf'], >> + }, >> + # >> # failed matches >> # > > Would be nice to also test for failure (with a wrong extension). > >> { >> diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm >> index 8149c88..8bc1bf8 100644 >> --- a/src/test/path_to_volume_id_test.pm >> +++ b/src/test/path_to_volume_id_test.pm >> @@ -174,6 +174,22 @@ 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", >> + expected => [ >> + 'import', >> + 'local:import/import.ovf', >> + ], >> + }, >> >> # no matches, path or files with failures >> { > > > Would be nice to also test for failure (with a wrong extension). sure. will do in a v2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel