Re: [pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
Am 22.04.24 um 13:56 schrieb Dominik Csapak: > i have to apologize, it seems my previous message was wrong. > after re-checking and re-testing a variant i had before changing it > to not returning any format for ova/ovfs it seems to work fine. > > not sure where the error i got came from (maybe i added some extra check > and > forgot about/missed it?) > > anyway, yes, i can send a v3 where i return ova/ovf as format for those > files, and even use 'ova+vmdk' etc. for the files contained in ovas > which makes checking for extraction a bit better > > does sound ok for you? > Sure, sounds good :) ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
On 4/22/24 13:34, Fiona Ebner wrote:
Am 22.04.24 um 13:09 schrieb Dominik Csapak:
On 4/22/24 13:00, Fiona Ebner wrote:
Am 19.04.24 um 11:45 schrieb Dominik Csapak:
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 22a9729..39a8354 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}+$PVE::Storage::IMPORT_EXT_RE_1)$!)
{
+ return ('import', $1);
Wouldn't it be nicer to return 'ovf' and 'ova' as the $file_format here
and check for that at the call sites? Currently you rely on the presence
or absence of $file_format in copy_needs_extraction() and
get_import_metadata() and then re-match on the ova extension. Having the
format right away would be a bit cleaner and more future-proof or is
there a specific reason against doing it?
i explained it in either the cover letter or in some commit message,
probably would have fit
in here too:
we currently only support raw/vmdk/qcow2/subvol here and it is intended
only for image formats
IIUC.
Hmm, maybe we can lift that limitation? Needs a bit of consideration of
course, but I don't see why the format returned by parse_volname()
should be limited to images, since there already is a vtype to
distinguish. And your series already uses the format for 'import' vtype
too ;)
Also we would have to adapt the `verify_format` for that, since it
will be
tested by that at least once. (not sure where exactly though, found out
by testing)
and that would mean we could set it as 'default' format on the storage,
which is not what we want...
We shouldn't allow that of course, but I don't see verify_format()
called other than for the "pve-storage-format" schema format, so maybe
some other check that fails? Or some use site of "pve-storage-format"
that should first check that it's an image? And I guess we should rename
it to "pve-storage-image-format", because it's used as that to avoid
future confusion.
i have to apologize, it seems my previous message was wrong.
after re-checking and re-testing a variant i had before changing it
to not returning any format for ova/ovfs it seems to work fine.
not sure where the error i got came from (maybe i added some extra check and
forgot about/missed it?)
anyway, yes, i can send a v3 where i return ova/ovf as format for those
files, and even use 'ova+vmdk' etc. for the files contained in ovas
which makes checking for extraction a bit better
does sound ok for you?
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
Am 22.04.24 um 13:09 schrieb Dominik Csapak:
> On 4/22/24 13:00, Fiona Ebner wrote:
>> Am 19.04.24 um 11:45 schrieb Dominik Csapak:
>>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>>> index 22a9729..39a8354 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}+$PVE::Storage::IMPORT_EXT_RE_1)$!)
>>> {
>>> + return ('import', $1);
>>
>> Wouldn't it be nicer to return 'ovf' and 'ova' as the $file_format here
>> and check for that at the call sites? Currently you rely on the presence
>> or absence of $file_format in copy_needs_extraction() and
>> get_import_metadata() and then re-match on the ova extension. Having the
>> format right away would be a bit cleaner and more future-proof or is
>> there a specific reason against doing it?
>
> i explained it in either the cover letter or in some commit message,
> probably would have fit
> in here too:
>
> we currently only support raw/vmdk/qcow2/subvol here and it is intended
> only for image formats
> IIUC.
Hmm, maybe we can lift that limitation? Needs a bit of consideration of
course, but I don't see why the format returned by parse_volname()
should be limited to images, since there already is a vtype to
distinguish. And your series already uses the format for 'import' vtype
too ;)
> Also we would have to adapt the `verify_format` for that, since it
> will be
> tested by that at least once. (not sure where exactly though, found out
> by testing)
> and that would mean we could set it as 'default' format on the storage,
> which is not what we want...
>
We shouldn't allow that of course, but I don't see verify_format()
called other than for the "pve-storage-format" schema format, so maybe
some other check that fails? Or some use site of "pve-storage-format"
that should first check that it's an image? And I guess we should rename
it to "pve-storage-image-format", because it's used as that to avoid
future confusion.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
On 4/22/24 13:00, Fiona Ebner wrote:
Am 19.04.24 um 11:45 schrieb Dominik Csapak:
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 22a9729..39a8354 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}+$PVE::Storage::IMPORT_EXT_RE_1)$!)
{
+ return ('import', $1);
Wouldn't it be nicer to return 'ovf' and 'ova' as the $file_format here
and check for that at the call sites? Currently you rely on the presence
or absence of $file_format in copy_needs_extraction() and
get_import_metadata() and then re-match on the ova extension. Having the
format right away would be a bit cleaner and more future-proof or is
there a specific reason against doing it?
i explained it in either the cover letter or in some commit message, probably
would have fit
in here too:
we currently only support raw/vmdk/qcow2/subvol here and it is intended only
for image formats
IIUC. Also we would have to adapt the `verify_format` for that, since it will be
tested by that at least once. (not sure where exactly though, found out by
testing)
and that would mean we could set it as 'default' format on the storage, which
is not what we want...
+} elsif ($volname =~
m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+\.(raw|vmdk|qcow2))$!) {
+ return ('import', $1, undef, undef, undef, undef, $2);
}
die "unable to parse directory volume name '$volname'\n";
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
Am 19.04.24 um 11:45 schrieb Dominik Csapak:
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 22a9729..39a8354 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}+$PVE::Storage::IMPORT_EXT_RE_1)$!)
> {
> + return ('import', $1);
Wouldn't it be nicer to return 'ovf' and 'ova' as the $file_format here
and check for that at the call sites? Currently you rely on the presence
or absence of $file_format in copy_needs_extraction() and
get_import_metadata() and then re-match on the ova extension. Having the
format right away would be a bit cleaner and more future-proof or is
there a specific reason against doing it?
> +} elsif ($volname =~
> m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+\.(raw|vmdk|qcow2))$!) {
> + return ('import', $1, undef, undef, undef, undef, $2);
> }
>
> die "unable to parse directory volume name '$volname'\n";
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type
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
---
changes from v1:
* only allow 'safe characters' in the relative filepath
* don't return 'image' type for vmdk/qcow2/raw files, instead use
'import' with a file_format, makes much of fionas review not necessary
* incorporated fionas suggestions
* added failure test
src/PVE/GuestImport/OVF.pm | 3 +++
src/PVE/Storage.pm | 8 +++
src/PVE/Storage/DirPlugin.pm | 36 +-
src/PVE/Storage/Plugin.pm | 13 ++-
src/test/parse_volname_test.pm | 18 +++
src/test/path_to_volume_id_test.pm | 21 +
6 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/src/PVE/GuestImport/OVF.pm b/src/PVE/GuestImport/OVF.pm
index 055ebf5..0eb5e9c 100644
--- a/src/PVE/GuestImport/OVF.pm
+++ b/src/PVE/GuestImport/OVF.pm
@@ -222,6 +222,8 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType",
$controller_id);
}
($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
+ ($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"
@@ -231,6 +233,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.pm b/src/PVE/Storage.pm
index f19a115..863dbdb 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -114,6 +114,10 @@ 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])/;
+
+our $SAFE_CHAR_CLASS_RE = qr/[a-zA-Z0-9\-\.\+\=\_]/;
+
# FIXME remove with PVE 8.0, add versioned breaks for pve-manager
our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
@@ -612,6 +616,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 +645,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");
}
}
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index 2efa8d5..6f9c2b9 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::GuestImport::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,37 @@ sub check_config {
return $opts;
}
+sub get_import_metadata {
+my ($class, $scfg, $volname, $storeid) = @_;
+
+my ($vtype, $name, undef, undef, undef, undef, $fmt) =
$class->parse_volname($volname);
+die "invalid content type '$vtype'\n" if $vtype ne 'import';
+die "invalid format\n" if defined($fmt);
+
+# NOTE: all types of warnings must be added to the return schema of the
import-metadata API endpoint
+my $warnings = [];
+
+my $path = $class->path($scfg, $volname, $storeid, undef);
+my $res = PVE::GuestImport::OVF::parse_ovf($path);
+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
