Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
On 2024-11-18 21:22, Thomas Lamprecht wrote:
I now looked into your diff.
Am 18.11.24 um 18:24 schrieb Aaron Lauterer:
On 2024-11-18 16:29, Dominik Csapak wrote:
+ if (!$extraction_scfg->{content}->{images} ||
!$extraction_scfg->{path}) {
I think the if condition here is grouped wrong.
As it is, once if one of the items (content->images or path) is falsy,
we trigger it, but it should not be the case, if the other is actually true.
how so? It must be both, file based and allow the import content type?
Consider the following diff, basically grouping the content->images ||
path condition, and only if the result is falsy, we want to raise the error.
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8db443d..545fe31 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -179,7 +179,7 @@ my $check_storage_access = sub {
$scfg;
my $extraction_param = defined($extraction_storage) ?
'import-working-storage' : $ds;
- if (!$extraction_scfg->{content}->{images} ||
!$extraction_scfg->{path}) {
+ if (!($extraction_scfg->{content}->{images} ||
$extraction_scfg->{path})) {
Besides above, the error would now be wrong too, it states:
"storage selected for extraction does not support 'images' content type or is not
file based"
(not (support 'images' content type)) or is (not (file based))
Exactly what Dominik used.
For completeness’ sake: I talked in person with Dominik and yes, my
understanding was wrong.
AFAIU Dominik will send a few follow-up patches, one of them will change
the error message to make it clearer that the issue is with the working
or OVA source storage. As that would probably have helped me yesterday
to not come to the wrong conclusion.
The fix here would add the 'images' content-type as supported one for
CephFS.
This is something I would like to avoid unless we have a way to prevent
using these images in guest configs. Starting a VM or CT with the disk
image on a Ceph FS will lead to all kinds of issues. RBD is what should
be used to store guest disk images in Ceph.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
I now looked into your diff.
Am 18.11.24 um 18:24 schrieb Aaron Lauterer:
> On 2024-11-18 16:29, Dominik Csapak wrote:
>> +if (!$extraction_scfg->{content}->{images} ||
>> !$extraction_scfg->{path}) {
>
> I think the if condition here is grouped wrong.
>
> As it is, once if one of the items (content->images or path) is falsy,
> we trigger it, but it should not be the case, if the other is actually true.
how so? It must be both, file based and allow the import content type?
> Consider the following diff, basically grouping the content->images ||
> path condition, and only if the result is falsy, we want to raise the error.
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8db443d..545fe31 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -179,7 +179,7 @@ my $check_storage_access = sub {
> $scfg;
> my $extraction_param = defined($extraction_storage) ?
> 'import-working-storage' : $ds;
>
> - if (!$extraction_scfg->{content}->{images} ||
> !$extraction_scfg->{path}) {
> + if (!($extraction_scfg->{content}->{images} ||
> $extraction_scfg->{path})) {
Besides above, the error would now be wrong too, it states:
"storage selected for extraction does not support 'images' content type or is
not file based"
(not (support 'images' content type)) or is (not (file based))
Exactly what Dominik used.
The fix here would add the 'images' content-type as supported one for
CephFS.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
On 2024-11-18 16:29, Dominik Csapak wrote:
this is to override the target extraction storage for the option disk
extraction for 'import-from'. This way if the storage does not
supports the content type 'images', one can give an alternative one.
Signed-off-by: Dominik Csapak
---
changes from v6:
* rename 'import-extraction-storage' to 'import-working-storage'
* rework permission checks (single branch)
i opted to not make the target the first default, since i did not want
to introduce such a change this late in the patch/review cycle and
AFAICS quite a few code changes would have been necessary for that
(we can still change that later too)
PVE/API2/Qemu.pm | 48 +++-
1 file changed, 39 insertions(+), 9 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c947f09c..701558a7 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -132,7 +132,7 @@ my $check_drive_param = sub {
};
my $check_storage_access = sub {
- my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
+ my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage,
$extraction_storage) = @_;
$foreach_volume_with_alloc->($settings, sub {
my ($ds, $drive) = @_;
@@ -174,9 +174,18 @@ my $check_storage_access = sub {
if $vtype ne 'images' && $vtype ne 'import';
if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) {
- raise_param_exc({ $ds => "$src_image is not on an storage with
'images' content type."})
- if !$scfg->{content}->{images};
- $rpcenv->check($authuser, "/storage/$storeid",
['Datastore.AllocateSpace']);
+ my $extraction_scfg = defined($extraction_storage) ?
+ PVE::Storage::storage_config($storecfg,
$extraction_storage) :
+ $scfg;
+ my $extraction_param = defined($extraction_storage) ?
'import-working-storage' : $ds;
+
+ if (!$extraction_scfg->{content}->{images} ||
!$extraction_scfg->{path}) {
I think the if condition here is grouped wrong.
As it is, once if one of the items (content->images or path) is falsy,
we trigger it, but it should not be the case, if the other is actually true.
Consider the following diff, basically grouping the content->images ||
path condition, and only if the result is falsy, we want to raise the error.
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8db443d..545fe31 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -179,7 +179,7 @@ my $check_storage_access = sub {
$scfg;
my $extraction_param = defined($extraction_storage)
? 'import-working-storage' : $ds;
- if (!$extraction_scfg->{content}->{images} ||
!$extraction_scfg->{path}) {
+ if (!($extraction_scfg->{content}->{images} ||
$extraction_scfg->{path})) {
raise_param_exc({
$extraction_param => "storage selected for
extraction does not support"
." 'images' content type or is not file
based.",
+ raise_param_exc({
+ $extraction_param => "storage selected for extraction
does not support"
+ ." 'images' content type or is not file based.",
+ });
+ }
+ $rpcenv->check($authuser, "/storage/" .
($extraction_storage // $storeid), ['Datastore.AllocateSpace']);
}
}
@@ -349,7 +358,7 @@ my sub prohibit_tpm_version_change {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
Am 18.11.24 um 18:39 schrieb Aaron Lauterer: > lore.proxmox.com seems to show the diff wrong... > I see them "wrong" too in my inbox, seems your mail user agent messes with them? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
lore.proxmox.com seems to show the diff wrong...
On 2024-11-18 18:24, Aaron Lauterer wrote:
On 2024-11-18 16:29, Dominik Csapak wrote:
this is to override the target extraction storage for the option disk
extraction for 'import-from'. This way if the storage does not
supports the content type 'images', one can give an alternative one.
Signed-off-by: Dominik Csapak
---
changes from v6:
* rename 'import-extraction-storage' to 'import-working-storage'
* rework permission checks (single branch)
i opted to not make the target the first default, since i did not want
to introduce such a change this late in the patch/review cycle and
AFAICS quite a few code changes would have been necessary for that
(we can still change that later too)
PVE/API2/Qemu.pm | 48 +++-
1 file changed, 39 insertions(+), 9 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c947f09c..701558a7 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -132,7 +132,7 @@ my $check_drive_param = sub {
};
my $check_storage_access = sub {
- my ($rpcenv, $authuser, $storecfg, $vmid, $settings,
$default_storage) = @_;
+ my ($rpcenv, $authuser, $storecfg, $vmid, $settings,
$default_storage, $extraction_storage) = @_;
$foreach_volume_with_alloc->($settings, sub {
my ($ds, $drive) = @_;
@@ -174,9 +174,18 @@ my $check_storage_access = sub {
if $vtype ne 'images' && $vtype ne 'import';
if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) {
- raise_param_exc({ $ds => "$src_image is not on an storage
with 'images' content type."})
- if !$scfg->{content}->{images};
- $rpcenv->check($authuser, "/storage/$storeid",
['Datastore.AllocateSpace']);
+ my $extraction_scfg = defined($extraction_storage) ?
+ PVE::Storage::storage_config($storecfg,
$extraction_storage) :
+ $scfg;
+ my $extraction_param = defined($extraction_storage) ?
'import-working-storage' : $ds;
+
+ if (!$extraction_scfg->{content}->{images} || !
$extraction_scfg->{path}) {
I think the if condition here is grouped wrong.
As it is, once if one of the items (content->images or path) is falsy,
we trigger it, but it should not be the case, if the other is actually
true.
Consider the following diff, basically grouping the content->images ||
path condition, and only if the result is falsy, we want to raise the
error.
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8db443d..545fe31 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -179,7 +179,7 @@ my $check_storage_access = sub {
$scfg;
my $extraction_param =
defined($extraction_storage) ? 'import-working-storage' : $ds;
- if (!$extraction_scfg->{content}->{images} || !
$extraction_scfg->{path}) {
+ if (!($extraction_scfg->{content}->{images} ||
$extraction_scfg->{path})) {
raise_param_exc({
$extraction_param => "storage selected for
extraction does not support"
." 'images' content type or is not file
based.",
Lets try it once more, hopefully the line endings will be as I want
them, even on lore.proxmox.com
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8db443d..545fe31 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -179,7 +179,7 @@ my $check_storage_access = sub {
$scfg;
my $extraction_param = defined($extraction_storage)
? 'import-working-storage' : $ds;
- if (!$extraction_scfg->{content}->{images} ||
!$extraction_scfg->{path}) {
+ if (!($extraction_scfg->{content}->{images} ||
$extraction_scfg->{path})) {
raise_param_exc({
$extraction_param => "storage selected for
extraction does not support"
." 'images' content type or is not file
based.",
Also, I was able to run into this by uploading the OVA to a CephFS.
+ raise_param_exc({
+ $extraction_param => "storage selected for extraction
does not support"
+ ." 'images' content type or is not file based.",
+ });
+ }
+ $rpcenv->check($authuser, "/storage/" .
($extraction_storage // $storeid), ['Datastore.AllocateSpace']);
}
}
@@ -349,7 +358,7 @@ my sub prohibit_tpm_version_change {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
