From: Dominik Csapak <d.csapak@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH storage 2/9] plugin: dir: implement import content type
Date: Wed, 17 Apr 2024 15:13:38 +0200 [thread overview]
Message-ID: <b358df63-f550-4a4f-9deb-2b0cbbf075ba@proxmox.com> (raw)
In-Reply-To: <0991e935-e29f-4459-9872-5da17cb7e929@proxmox.com>
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 <d.csapak@proxmox.com>
>> ---
>> 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 = <iso|vztmpl|backup|snippets>)
>> +# list templates ($tt = <iso|vztmpl|backup|snippets|import>)
>> 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
next prev parent reply other threads:[~2024-04-17 13:13 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 13:18 [pve-devel] [PATCH storage/qemu-server/pve-manager] implement ova/ovf import for directory type storages Dominik Csapak
2024-04-16 13:18 ` [pve-devel] [PATCH storage 1/9] copy OVF.pm from qemu-server Dominik Csapak
2024-04-16 15:02 ` Thomas Lamprecht
2024-04-17 9:19 ` Fiona Ebner
2024-04-17 9:26 ` Thomas Lamprecht
2024-04-16 13:18 ` [pve-devel] [PATCH storage 2/9] plugin: dir: implement import content type Dominik Csapak
2024-04-17 10:07 ` Fiona Ebner
2024-04-17 10:07 ` Fiona Ebner
2024-04-17 13:13 ` Dominik Csapak [this message]
2024-04-17 12:46 ` Fabian Grünbichler
2024-04-16 13:18 ` [pve-devel] [PATCH storage 3/9] plugin: dir: handle ova files for import Dominik Csapak
2024-04-17 10:52 ` Fiona Ebner
2024-04-17 13:07 ` Dominik Csapak
2024-04-17 13:39 ` Fabian Grünbichler
2024-04-18 7:22 ` Fiona Ebner
2024-04-18 7:25 ` Fiona Ebner
2024-04-18 8:55 ` Fabian Grünbichler
2024-04-17 12:45 ` Fabian Grünbichler
2024-04-17 13:10 ` Dominik Csapak
2024-04-17 13:52 ` Fabian Grünbichler
2024-04-17 14:07 ` Dominik Csapak
2024-04-18 6:46 ` Fabian Grünbichler
2024-04-16 13:18 ` [pve-devel] [PATCH storage 4/9] ovf: implement parsing the ostype Dominik Csapak
2024-04-17 11:32 ` Fiona Ebner
2024-04-17 13:14 ` Dominik Csapak
2024-04-18 7:31 ` Fiona Ebner
2024-04-16 13:18 ` [pve-devel] [PATCH storage 5/9] ovf: implement parsing out firmware type Dominik Csapak
2024-04-17 11:43 ` Fiona Ebner
2024-04-16 13:18 ` [pve-devel] [PATCH storage 6/9] ovf: implement rudimentary boot order Dominik Csapak
2024-04-17 11:54 ` Fiona Ebner
2024-04-17 13:15 ` Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH storage 7/9] ovf: implement parsing nics Dominik Csapak
2024-04-17 12:09 ` Fiona Ebner
2024-04-17 13:16 ` Dominik Csapak
2024-04-18 8:22 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH storage 8/9] api: allow ova upload/download Dominik Csapak
2024-04-18 8:05 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH storage 9/9] plugin: enable import for nfs/btfs/cifs/cephfs Dominik Csapak
2024-04-18 8:43 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 1/3] api: delete unused OVF.pm Dominik Csapak
2024-04-18 8:52 ` Fiona Ebner
2024-04-18 8:57 ` Dominik Csapak
2024-04-18 9:03 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 2/3] use OVF from Storage Dominik Csapak
2024-04-18 9:07 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 3/3] api: create: implement extracting disks when needed for import-from Dominik Csapak
2024-04-18 9:41 ` Fiona Ebner
2024-04-18 9:48 ` Dominik Csapak
2024-04-18 9:55 ` Fiona Ebner
2024-04-18 9:58 ` Dominik Csapak
2024-04-18 10:01 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH manager 1/4] ui: fix special 'import' icon for non-esxi storages Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 2/4] ui: guest import: add ova-needs-extracting warning text Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 3/4] ui: enable import content type for relevant storages Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 4/4] ui: enable upload/download buttons for 'import' type storages Dominik Csapak
2024-04-17 12:37 ` Fabian Grünbichler
2024-04-18 11:20 ` Fiona Ebner
2024-04-18 11:23 ` Dominik Csapak
2024-04-18 11:26 ` Fiona Ebner
2024-04-17 13:11 ` [pve-devel] [PATCH storage/qemu-server/pve-manager] implement ova/ovf import for directory " Fabian Grünbichler
2024-04-17 13:19 ` Dominik Csapak
2024-04-18 6:40 ` Fabian Grünbichler
2024-04-18 9:27 ` Dominik Csapak
2024-04-18 10:35 ` Fiona Ebner
2024-04-18 11:10 ` Dominik Csapak
2024-04-18 11:13 ` Fiona Ebner
2024-04-18 11:17 ` Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b358df63-f550-4a4f-9deb-2b0cbbf075ba@proxmox.com \
--to=d.csapak@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox