* [pve-devel] [PATCH pve-storage v2 1/5] api: {upload, download}_url: factor out common parameter hash accesses
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
@ 2025-02-11 16:07 ` Daniel Kral
2025-02-19 15:39 ` [pve-devel] applied: " Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes Daniel Kral
` (31 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:07 UTC (permalink / raw)
To: pve-devel
Minor cleanup to reduce the amount of `$param->{...}` to variables in
the upload and download url API handler.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/API2/Storage/Status.pm | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index e9a451c..c854b53 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -438,8 +438,8 @@ __PACKAGE__->register_method ({
my $cfg = PVE::Storage::config();
- my $node = $param->{node};
- my $scfg = PVE::Storage::storage_check_enabled($cfg, $param->{storage}, $node);
+ my ($node, $storage) = $param->@{qw(node storage)};
+ my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
die "can't upload to storage type '$scfg->{type}'\n"
if !defined($scfg->{path});
@@ -461,24 +461,24 @@ __PACKAGE__->register_method ({
if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
raise_param_exc({ filename => "wrong file extension" });
}
- $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
+ $path = PVE::Storage::get_iso_dir($cfg, $storage);
} elsif ($content eq 'vztmpl') {
if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
raise_param_exc({ filename => "wrong file extension" });
}
- $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
+ $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
} elsif ($content eq 'import') {
if ($filename !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$!) {
raise_param_exc({ filename => "invalid filename or wrong extension" });
}
$isOva = 1;
- $path = PVE::Storage::get_import_dir($cfg, $param->{storage});
+ $path = PVE::Storage::get_import_dir($cfg, $storage);
} else {
raise_param_exc({ content => "upload content type '$content' not allowed" });
}
- die "storage '$param->{storage}' does not support '$content' content\n"
+ die "storage '$storage' does not support '$content' content\n"
if !$scfg->{content}->{$content};
my $dest = "$path/$filename";
@@ -498,9 +498,9 @@ __PACKAGE__->register_method ({
my @remcmd = ('/usr/bin/ssh', $ssh_options->@*, $remip, '--');
eval { # activate remote storage
- run_command([@remcmd, '/usr/sbin/pvesm', 'status', '--storage', $param->{storage}]);
+ run_command([@remcmd, '/usr/sbin/pvesm', 'status', '--storage', $storage]);
};
- die "can't activate storage '$param->{storage}' on node '$node': $@\n" if $@;
+ die "can't activate storage '$storage' on node '$node': $@\n" if $@;
run_command(
[@remcmd, '/bin/mkdir', '-p', '--', PVE::Tools::shell_quote($dirname)],
@@ -511,7 +511,7 @@ __PACKAGE__->register_method ({
$err_cleanup = sub { run_command([@remcmd, 'rm', '-f', '--', $dest]) };
} else {
- PVE::Storage::activate_storage($cfg, $param->{storage});
+ PVE::Storage::activate_storage($cfg, $storage);
File::Path::make_path($dirname);
$cmd = ['cp', '--', $tmpfilename, $dest];
}
@@ -687,7 +687,7 @@ __PACKAGE__->register_method({
$isOva = 1;
}
- $path = PVE::Storage::get_import_dir($cfg, $param->{storage});
+ $path = PVE::Storage::get_import_dir($cfg, $storage);
} else {
raise_param_exc({ content => "upload content-type '$content' is not allowed" });
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 1/5] api: {upload, download}_url: factor out common parameter hash accesses Daniel Kral
@ 2025-02-11 16:07 ` Daniel Kral
2025-02-19 14:54 ` Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper Daniel Kral
` (30 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:07 UTC (permalink / raw)
To: pve-devel
Add subroutines for asserting the content types of storages and volumes
to reduce code duplication, e.g. when implementing preconditions in an
API handler before calling vdisk_alloc.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- moved from qemu-server to pve-storage
- add missing $node parameter to helpers
- adapt and fix wrong docs (copy paste error)
- remove `alloc_volume_disk` and `check_{volume,storage}_alloc`
src/PVE/Storage.pm | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 3b4f041..ca69cd6 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -529,6 +529,46 @@ sub parse_volume_id {
return PVE::Storage::Plugin::parse_volume_id($volid, $noerr);
}
+=head3 assert_content_type_supported($cfg, $storeid, $content_type [, $node])
+
+Asserts whether the storage with the identifier C<$storeid>, which is defined in C<$cfg>, supports
+the content type C<$content_type>.
+
+If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
+
+If the check fails, the subroutine will C<die> with an error message for either the storage being
+unavailable or the storage not supporting the specified content type.
+
+=head3 assert_volume_type_supported($cfg, $volid [, $node])
+
+Asserts whether the volume with the identifier C<$volid>, which is on a storage defined in C<$cfg>,
+supports the volume's content type determined by L<parse_volname>.
+
+If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
+
+If the check fails, the subroutine will C<die> with an error message for either the storage being
+unavailable or the storage not supporting the volume's content type.
+
+=cut
+
+sub assert_content_type_supported : prototype($$$;$) {
+ my ($cfg, $storeid, $content_type, $node) = @_;
+
+ my $scfg = storage_config($cfg, $storeid, $node);
+
+ die "storage '$storeid' does not support content type '$content_type'\n"
+ if !$scfg->{content}->{$content_type};
+}
+
+sub assert_volume_type_supported : prototype($$;$) {
+ my ($cfg, $volid, $node) = @_;
+
+ my ($storeid) = parse_volume_id($volid);
+ my ($vtype) = parse_volname($cfg, $volid);
+
+ assert_content_type_supported($cfg, $storeid, $vtype, $node);
+}
+
# test if we have read access to volid
sub check_volume_access {
my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes Daniel Kral
@ 2025-02-19 14:54 ` Fiona Ebner
2025-02-20 9:14 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-19 14:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> Add subroutines for asserting the content types of storages and volumes
> to reduce code duplication, e.g. when implementing preconditions in an
> API handler before calling vdisk_alloc.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - moved from qemu-server to pve-storage
> - add missing $node parameter to helpers
> - adapt and fix wrong docs (copy paste error)
> - remove `alloc_volume_disk` and `check_{volume,storage}_alloc`
>
> src/PVE/Storage.pm | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 3b4f041..ca69cd6 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -529,6 +529,46 @@ sub parse_volume_id {
> return PVE::Storage::Plugin::parse_volume_id($volid, $noerr);
> }
>
> +=head3 assert_content_type_supported($cfg, $storeid, $content_type [, $node])
> +
> +Asserts whether the storage with the identifier C<$storeid>, which is defined in C<$cfg>, supports
> +the content type C<$content_type>.
> +
> +If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
> +
> +If the check fails, the subroutine will C<die> with an error message for either the storage being
> +unavailable or the storage not supporting the specified content type.
> +
I'd rather group the functions with their respective doc. I.e.
doc+function,doc+function instead of doc+doc,function+function.
> +=head3 assert_volume_type_supported($cfg, $volid [, $node])
> +
> +Asserts whether the volume with the identifier C<$volid>, which is on a storage defined in C<$cfg>,
> +supports the volume's content type determined by L<parse_volname>.
> +
> +If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
> +
> +If the check fails, the subroutine will C<die> with an error message for either the storage being
> +unavailable or the storage not supporting the volume's content type.
> +
> +=cut
> +
> +sub assert_content_type_supported : prototype($$$;$) {
> + my ($cfg, $storeid, $content_type, $node) = @_;
> +
> + my $scfg = storage_config($cfg, $storeid, $node);
The storage_config() function does not have a $node parameter, but a
$noerr parameter. I guess you want to use storage_check_enabled() since
the documentation talks about "storage being unavailable"?
> +
> + die "storage '$storeid' does not support content type '$content_type'\n"
> + if !$scfg->{content}->{$content_type};
> +}
> +
> +sub assert_volume_type_supported : prototype($$;$) {
> + my ($cfg, $volid, $node) = @_;
> +
> + my ($storeid) = parse_volume_id($volid);
> + my ($vtype) = parse_volname($cfg, $volid);
> +
> + assert_content_type_supported($cfg, $storeid, $vtype, $node);
> +}
> +
> # test if we have read access to volid
> sub check_volume_access {
> my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-19 14:54 ` Fiona Ebner
@ 2025-02-20 9:14 ` Daniel Kral
2025-02-20 9:36 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 9:14 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/19/25 15:54, Fiona Ebner wrote:
> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>> Add subroutines for asserting the content types of storages and volumes
>> to reduce code duplication, e.g. when implementing preconditions in an
>> API handler before calling vdisk_alloc.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes since v1:
>> - moved from qemu-server to pve-storage
>> - add missing $node parameter to helpers
>> - adapt and fix wrong docs (copy paste error)
>> - remove `alloc_volume_disk` and `check_{volume,storage}_alloc`
>>
>> src/PVE/Storage.pm | 40 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 40 insertions(+)
>>
>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>> index 3b4f041..ca69cd6 100755
>> --- a/src/PVE/Storage.pm
>> +++ b/src/PVE/Storage.pm
>> @@ -529,6 +529,46 @@ sub parse_volume_id {
>> return PVE::Storage::Plugin::parse_volume_id($volid, $noerr);
>> }
>>
>> +=head3 assert_content_type_supported($cfg, $storeid, $content_type [, $node])
>> +
>> +Asserts whether the storage with the identifier C<$storeid>, which is defined in C<$cfg>, supports
>> +the content type C<$content_type>.
>> +
>> +If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
>> +
>> +If the check fails, the subroutine will C<die> with an error message for either the storage being
>> +unavailable or the storage not supporting the specified content type.
>> +
>
> I'd rather group the functions with their respective doc. I.e.
> doc+function,doc+function instead of doc+doc,function+function.
Will do so in a v3!
>
>> +=head3 assert_volume_type_supported($cfg, $volid [, $node])
>> +
>> +Asserts whether the volume with the identifier C<$volid>, which is on a storage defined in C<$cfg>,
>> +supports the volume's content type determined by L<parse_volname>.
>> +
>> +If C<$node> is set, the assertion is made for the specified C<$node>, else for the current node.
>> +
>> +If the check fails, the subroutine will C<die> with an error message for either the storage being
>> +unavailable or the storage not supporting the volume's content type.
>> +
>> +=cut
>> +
>> +sub assert_content_type_supported : prototype($$$;$) {
>> + my ($cfg, $storeid, $content_type, $node) = @_;
>> +
>> + my $scfg = storage_config($cfg, $storeid, $node);
>
> The storage_config() function does not have a $node parameter, but a
> $noerr parameter. I guess you want to use storage_check_enabled() since
> the documentation talks about "storage being unavailable"?
Uff sorry, that's right that was an oversight and doesn't make sense...
Yes you're correct, I had the `storage_check_enabled` here before, but
replaced it in the end but forgot to remove the parameter.
There are AFAIK 5 instances where I used this helper where there wasn't
a `storage_check_enabled` before and I was worried about breaking any
existing checks at these locations and another helper (with just an
added `storage_check_enabled`) felt like bloat.
I checked the locations again where there wasn't a
`storage_check_enabled` before:
- qemu-server patch #9 (in `config_to_command`),
- qemu-server patch #11 (in `check_storage_access`),
- qemu-server patch #13 (in `parse_backup_hints`),
- container patch #10 (in `update_pct_config`), and
- container patch #11 (in `__mountpoint_mount`).
If we could use the `storage_check_enabled` in all of those, I'd move
the `storage_check_enabled` in the helper method and add to each patch
message where there was a `storage_check_enabled` before with "No
functional changes intended" and those where it wasn't with a reason why
it makes sense to add one now. If I didn't miss anything:
- qemu-server #9 - `config_to_command` obviously fails at another
location if the storage is not currently enabled anyway
- qemu-server #11 - `check_storage_access` is called in the create_vm
and update_vm API handler... where it checks whether any new disk can be
put on the storage (also fails when the storage is not enabled)
- qemu-server #13 - `parse_backup_hints` is used in
`restore_vma_archive` and `restore_proxmox_backup_archive` to parse and
check whether the devices can be allocated (with
`$restore_allocate_devices` afterwards)
- container #10 - `update_pct_config` asserts whether new or changed
mountpoints can be allocated... and fails if the storage is not enabled
- container #11 - `__mountpoint_mount` is used in more places, but all
look to need the volume on the storage immediately afterwards anyway
(`snapshot`, `archive`, resize_vm API handler, `copy_volume`)... and I
guess it would fail in any of those with
`PVE::Storage::activate_volumes` calling `activate_storage` before the
helper anyway
So if I didn't miss anything, I'd do this in a v3.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-20 9:14 ` Daniel Kral
@ 2025-02-20 9:36 ` Fiona Ebner
2025-02-20 12:53 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 9:36 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 20.02.25 um 10:14 schrieb Daniel Kral:
> On 2/19/25 15:54, Fiona Ebner wrote:
>> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>>> +sub assert_content_type_supported : prototype($$$;$) {
>>> + my ($cfg, $storeid, $content_type, $node) = @_;
>>> +
>>> + my $scfg = storage_config($cfg, $storeid, $node);
>>
>> The storage_config() function does not have a $node parameter, but a
>> $noerr parameter. I guess you want to use storage_check_enabled() since
>> the documentation talks about "storage being unavailable"?
>
> Uff sorry, that's right that was an oversight and doesn't make sense...
>
> Yes you're correct, I had the `storage_check_enabled` here before, but
> replaced it in the end but forgot to remove the parameter.
>
> There are AFAIK 5 instances where I used this helper where there wasn't
> a `storage_check_enabled` before and I was worried about breaking any
> existing checks at these locations and another helper (with just an
> added `storage_check_enabled`) felt like bloat.
>
> I checked the locations again where there wasn't a
> `storage_check_enabled` before:
> - qemu-server patch #9 (in `config_to_command`),
> - qemu-server patch #11 (in `check_storage_access`),
> - qemu-server patch #13 (in `parse_backup_hints`),
> - container patch #10 (in `update_pct_config`), and
> - container patch #11 (in `__mountpoint_mount`).
>
> If we could use the `storage_check_enabled` in all of those, I'd move
> the `storage_check_enabled` in the helper method and add to each patch
> message where there was a `storage_check_enabled` before with "No
> functional changes intended" and those where it wasn't with a reason why
> it makes sense to add one now. If I didn't miss anything:
>
> - qemu-server #9 - `config_to_command` obviously fails at another
> location if the storage is not currently enabled anyway
Not necessarily? What about qm showcmd <ID>? Question is, do we want to
do storage-related checks there or not? If we already check for the
content type, I don't see much reason not to check for storage being
enabled either.
It could be seen as surprising, because it's just building the command,
why would it do storage checks? But if we follow that rationale, it
shouldn't do either of the checks.
I'm fine with keeping/adding the checks, because we already do that for
other things while building the command too. Otherwise, we'd need some
nocheck flag or similar. I have no strong opinion against that either.
Just nobody complained yet about this I guess and there's nothing really
wrong with having the semantics be getting a "checked" command.
> - qemu-server #11 - `check_storage_access` is called in the create_vm
> and update_vm API handler... where it checks whether any new disk can be
> put on the storage (also fails when the storage is not enabled)
> - qemu-server #13 - `parse_backup_hints` is used in
> `restore_vma_archive` and `restore_proxmox_backup_archive` to parse and
> check whether the devices can be allocated (with
> `$restore_allocate_devices` afterwards)
> - container #10 - `update_pct_config` asserts whether new or changed
> mountpoints can be allocated... and fails if the storage is not enabled
> - container #11 - `__mountpoint_mount` is used in more places, but all
> look to need the volume on the storage immediately afterwards anyway
> (`snapshot`, `archive`, resize_vm API handler, `copy_volume`)... and I
> guess it would fail in any of those with
> `PVE::Storage::activate_volumes` calling `activate_storage` before the
> helper anyway
>
> So if I didn't miss anything, I'd do this in a v3.
It does seem natural that callers that check for a content type being
supported actually want to do something with that content type too and
actually care about the content type being "ready"/"available". The only
cases I would imagine is checks about the storage config whether it
would be supported in principle, but if we ever have one of those, we
just don't need to use the helper.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-20 9:36 ` Fiona Ebner
@ 2025-02-20 12:53 ` Daniel Kral
2025-02-20 12:58 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 12:53 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 10:36, Fiona Ebner wrote:
> Not necessarily? What about qm showcmd <ID>? Question is, do we want to
> do storage-related checks there or not? If we already check for the
> content type, I don't see much reason not to check for storage being
> enabled either.
>
> It could be seen as surprising, because it's just building the command,
> why would it do storage checks? But if we follow that rationale, it
> shouldn't do either of the checks.
>
> I'm fine with keeping/adding the checks, because we already do that for
> other things while building the command too. Otherwise, we'd need some
> nocheck flag or similar. I have no strong opinion against that either.
> Just nobody complained yet about this I guess and there's nothing really
> wrong with having the semantics be getting a "checked" command.
Right, there's also `qm showcmd`, I only thought about the
vm_start_nolock case here! I guess the best thing would be to move it
there, or would an extra iteration through the disks like in cfg2cmd be
too expensive there? Otherwise I'll let it be in cfg2cmd.
On 2/20/25 10:36, Fiona Ebner wrote:
> It does seem natural that callers that check for a content type being
> supported actually want to do something with that content type too and
> actually care about the content type being "ready"/"available". The only
> cases I would imagine is checks about the storage config whether it
> would be supported in principle, but if we ever have one of those, we
> just don't need to use the helper.
Yes, I'd double check that when working on the v3, but as far as I've
seen how they current have been used and going through the cases above
it'd sound like they were used like this before anyway.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes
2025-02-20 12:53 ` Daniel Kral
@ 2025-02-20 12:58 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 12:58 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 20.02.25 um 13:53 schrieb Daniel Kral:
> On 2/20/25 10:36, Fiona Ebner wrote:
>> Not necessarily? What about qm showcmd <ID>? Question is, do we want to
>> do storage-related checks there or not? If we already check for the
>> content type, I don't see much reason not to check for storage being
>> enabled either.
>>
>> It could be seen as surprising, because it's just building the command,
>> why would it do storage checks? But if we follow that rationale, it
>> shouldn't do either of the checks.
>>
>> I'm fine with keeping/adding the checks, because we already do that for
>> other things while building the command too. Otherwise, we'd need some
>> nocheck flag or similar. I have no strong opinion against that either.
>> Just nobody complained yet about this I guess and there's nothing really
>> wrong with having the semantics be getting a "checked" command.
>
> Right, there's also `qm showcmd`, I only thought about the
> vm_start_nolock case here! I guess the best thing would be to move it
> there, or would an extra iteration through the disks like in cfg2cmd be
> too expensive there? Otherwise I'll let it be in cfg2cmd.
From my side, it's fine to do the checks even in the "qm showcmd" case.
We also do other kinds of checks in config_to_command() already and I
don't think we ever got user complaints about that. It's good to know
even when querying the command when something is misconfigured IMHO.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 1/5] api: {upload, download}_url: factor out common parameter hash accesses Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes Daniel Kral
@ 2025-02-11 16:07 ` Daniel Kral
2025-02-19 15:16 ` Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument Daniel Kral
` (29 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:07 UTC (permalink / raw)
To: pve-devel
Make any code path with an existent content type assertion use the newly
introduced content type assertion helper.
As those code paths must perform actions on the storage anyway, the
`storage_check_enabled` in the helper subroutine adds an additional
precondition check without breaking the existing APIs with a new error.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/API2/Storage/Status.pm | 6 ++----
src/PVE/Storage.pm | 3 ++-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index c854b53..e5652f4 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -478,8 +478,7 @@ __PACKAGE__->register_method ({
raise_param_exc({ content => "upload content type '$content' not allowed" });
}
- die "storage '$storage' does not support '$content' content\n"
- if !$scfg->{content}->{$content};
+ PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);
my $dest = "$path/$filename";
my $dirname = dirname($dest);
@@ -660,8 +659,7 @@ __PACKAGE__->register_method({
my ($content, $url) = $param->@{'content', 'url'};
- die "storage '$storage' is not configured for content-type '$content'\n"
- if !$scfg->{content}->{$content};
+ PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);
my $filename = PVE::Storage::normalize_content_filename($param->{filename});
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index ca69cd6..0134893 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1816,7 +1816,8 @@ sub prune_backups {
my ($cfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
my $scfg = storage_config($cfg, $storeid);
- die "storage '$storeid' does not support backups\n" if !$scfg->{content}->{backup};
+
+ PVE::Storage::assert_content_type_supported($cfg, $storeid, "backup");
if (!defined($keep)) {
die "no prune-backups options configured for storage '$storeid'\n"
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper Daniel Kral
@ 2025-02-19 15:16 ` Fiona Ebner
2025-02-20 9:31 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-19 15:16 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> Make any code path with an existent content type assertion use the newly
> introduced content type assertion helper.
>
> As those code paths must perform actions on the storage anyway, the
> `storage_check_enabled` in the helper subroutine adds an additional
> precondition check without breaking the existing APIs with a new error.
>
So here you do talk about storage_check_enabled(). Did you maybe send an
incorrect version of the previous patch ;)?
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
With the previous patch fixed:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
However, see below:
> ---
> changes since v1:
> - new!
>
> src/PVE/API2/Storage/Status.pm | 6 ++----
> src/PVE/Storage.pm | 3 ++-
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
> index c854b53..e5652f4 100644
> --- a/src/PVE/API2/Storage/Status.pm
> +++ b/src/PVE/API2/Storage/Status.pm
> @@ -478,8 +478,7 @@ __PACKAGE__->register_method ({
> raise_param_exc({ content => "upload content type '$content' not allowed" });
> }
>
> - die "storage '$storage' does not support '$content' content\n"
> - if !$scfg->{content}->{$content};
> + PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);
Above here is already a storage_check_enabled() check that would become
superfluous and could be removed. While it doesn't hurt to keep, I'm
wondering if we can better encode the semantics for the new helper in
its name and get rid of the duplicate check after all. Also to make it
easier for future usages to remember that the enabled check is already
done too. Maybe calling the helper assert_content_type_available() or to
be rather explicit assert_storage_ready_for_content_type() would make it
clear that it means that both, the storage is enabled on the node and
the content type is configured for the storage? Other suggestions are
welcome!
>
> my $dest = "$path/$filename";
> my $dirname = dirname($dest);
> @@ -660,8 +659,7 @@ __PACKAGE__->register_method({
>
> my ($content, $url) = $param->@{'content', 'url'};
>
> - die "storage '$storage' is not configured for content-type '$content'\n"
> - if !$scfg->{content}->{$content};
> + PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);
Similar here.
>
> my $filename = PVE::Storage::normalize_content_filename($param->{filename});
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index ca69cd6..0134893 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1816,7 +1816,8 @@ sub prune_backups {
> my ($cfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
>
> my $scfg = storage_config($cfg, $storeid);
> - die "storage '$storeid' does not support backups\n" if !$scfg->{content}->{backup};
> +
> + PVE::Storage::assert_content_type_supported($cfg, $storeid, "backup");
>
> if (!defined($keep)) {
> die "no prune-backups options configured for storage '$storeid'\n"
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper
2025-02-19 15:16 ` Fiona Ebner
@ 2025-02-20 9:31 ` Daniel Kral
0 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 9:31 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/19/25 16:16, Fiona Ebner wrote:
> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>> Make any code path with an existent content type assertion use the newly
>> introduced content type assertion helper.
>>
>> As those code paths must perform actions on the storage anyway, the
>> `storage_check_enabled` in the helper subroutine adds an additional
>> precondition check without breaking the existing APIs with a new error.
>>
>
> So here you do talk about storage_check_enabled(). Did you maybe send an
> incorrect version of the previous patch ;)?
That was an oversight, but as mentioned in the previous response, I'd
hope to be able to make the `storage_config` to a
`storage_check_enabled` in a v3 if there's nothing breaking about this
when replacing the existing checks :).
>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>
> With the previous patch fixed:
>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
>
> However, see below:
>
>> ---
>> changes since v1:
>> - new!
>>
>> src/PVE/API2/Storage/Status.pm | 6 ++----
>> src/PVE/Storage.pm | 3 ++-
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
>> index c854b53..e5652f4 100644
>> --- a/src/PVE/API2/Storage/Status.pm
>> +++ b/src/PVE/API2/Storage/Status.pm
>> @@ -478,8 +478,7 @@ __PACKAGE__->register_method ({
>> raise_param_exc({ content => "upload content type '$content' not allowed" });
>> }
>>
>> - die "storage '$storage' does not support '$content' content\n"
>> - if !$scfg->{content}->{$content};
>> + PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);
>
> Above here is already a storage_check_enabled() check that would become
> superfluous and could be removed. While it doesn't hurt to keep, I'm
> wondering if we can better encode the semantics for the new helper in
> its name and get rid of the duplicate check after all. Also to make it
> easier for future usages to remember that the enabled check is already
> done too. Maybe calling the helper assert_content_type_available() or to
> be rather explicit assert_storage_ready_for_content_type() would make it
> clear that it means that both, the storage is enabled on the node and
> the content type is configured for the storage? Other suggestions are
> welcome!
Agreed, a better name would be good here, so it doesn't add confusion! I
think I'd go for the first suggestion in a v3, but I'll think about
it... The second suggestion is great, but I'd like to keep most helpers
in one line if possible - but this shouldn't be more important than
clarity of course.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (2 preceding siblings ...)
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper Daniel Kral
@ 2025-02-11 16:07 ` Daniel Kral
2025-02-20 8:49 ` Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type Daniel Kral
` (28 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:07 UTC (permalink / raw)
To: pve-devel
Moves the optional parameter `name` into an optional hash argument at
the end of the function.
This allows to add more optional arguments in the future and makes the
function call easier to follow when a name is not required.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/API2/Storage/Content.pm | 6 +++---
src/PVE/GuestImport.pm | 2 +-
src/PVE/Storage.pm | 30 ++++++++++++++++++++++++++++--
src/test/run_test_zfspoolplugin.pl | 8 ++++----
4 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index fe0ad4a..77924a6 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -220,9 +220,9 @@ __PACKAGE__->register_method ({
my $cfg = PVE::Storage::config();
- my $volid = PVE::Storage::vdisk_alloc ($cfg, $storeid, $param->{vmid},
- $param->{format},
- $name, $size);
+ my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $param->{vmid}, $param->{format}, $size, {
+ name => $name,
+ });
return $volid;
}});
diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm
index 16099ca..76ebc9d 100644
--- a/src/PVE/GuestImport.pm
+++ b/src/PVE/GuestImport.pm
@@ -69,7 +69,7 @@ sub extract_disk_from_import_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_volid = PVE::Storage::vdisk_alloc($cfg, $target_storeid, $vmid, $inner_fmt, 1024);
$target_path = PVE::Storage::path($cfg, $target_volid);
print "renaming $source_path to $target_path\n";
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 0134893..3776565 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1041,8 +1041,34 @@ sub unmap_volume {
return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
}
-sub vdisk_alloc {
- my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
+=head3 vdisk_alloc($cfg, $storeid, $vmid, $size, %opts)
+
+Allocates a volume on the storage with the identifier C<$storeid> (defined in the storage config
+C<$cfg>) for the VM with the id C<$vmid> with format C<$fmt> and a size of C<$size> kilobytes.
+
+The format C<$fmt> can be C<'raw'>, C<'qcow2'>, C<'subvol'> or C<'vmdk'>. If C<$fmt> is left
+undefined, it will fallback on the default format of the storage type of C<$storeid>.
+
+Optionally, the following key-value pairs are available for C<%opts>:
+
+=over
+
+=item * C<< $name => $string >>
+
+Specifies the name of the new volume.
+
+If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
+
+=back
+
+Returns the identifier for the new volume in the format C<"$storeid:$volname">.
+
+=cut
+
+sub vdisk_alloc : prototype($$$$$;%) {
+ my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
+
+ my $name = $opts->{name};
die "no storage ID specified\n" if !$storeid;
diff --git a/src/test/run_test_zfspoolplugin.pl b/src/test/run_test_zfspoolplugin.pl
index 095ccb3..7dd0a2b 100755
--- a/src/test/run_test_zfspoolplugin.pl
+++ b/src/test/run_test_zfspoolplugin.pl
@@ -565,7 +565,7 @@ my $test13 = sub {
print "\nrun test13 \"vdisk_alloc\"\n";
eval {
- my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", undef ,1024 * 1024);
+ my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 1024 * 1024);
if ($tmp_volid ne "$storagename:vm-112-disk-0") {
die "volname:$tmp_volid don't match\n";
@@ -589,7 +589,7 @@ my $test13 = sub {
}
eval {
- my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", undef ,2048 * 1024);
+ my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 2048 * 1024);
if ($tmp_volid ne "$storagename:vm-112-disk-1") {
die "volname:$tmp_volid don't match\n";
@@ -613,7 +613,7 @@ my $test13 = sub {
}
eval {
- my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", undef ,1024 * 1024);
+ my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 1024 * 1024);
if ($tmp_volid ne "$storagename:subvol-113-disk-0") {
die "volname:$tmp_volid don't match\n";
@@ -637,7 +637,7 @@ my $test13 = sub {
}
eval {
- my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", undef ,2048 * 1024);
+ my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 2048 * 1024);
if ($tmp_volid ne "$storagename:subvol-113-disk-1") {
die "volname:$tmp_volid don't match\n";
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument Daniel Kral
@ 2025-02-20 8:49 ` Fiona Ebner
2025-02-20 9:34 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 8:49 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> Moves the optional parameter `name` into an optional hash argument at
> the end of the function.
>
> This allows to add more optional arguments in the future and makes the
> function call easier to follow when a name is not required.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
While the cover letter already talks about it, it never hurts to
explicitly mention that this requires a versioned breaks for qemu-server
and pve-container again in the patch itself.
> changes since v1:
> - new!
>
> src/PVE/API2/Storage/Content.pm | 6 +++---
> src/PVE/GuestImport.pm | 2 +-
> src/PVE/Storage.pm | 30 ++++++++++++++++++++++++++++--
> src/test/run_test_zfspoolplugin.pl | 8 ++++----
> 4 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index fe0ad4a..77924a6 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -220,9 +220,9 @@ __PACKAGE__->register_method ({
>
> my $cfg = PVE::Storage::config();
>
> - my $volid = PVE::Storage::vdisk_alloc ($cfg, $storeid, $param->{vmid},
> - $param->{format},
> - $name, $size);
> + my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $param->{vmid}, $param->{format}, $size, {
> + name => $name,
> + });
Style nit: the first line is still too long and just having the last
param split out like this looks kinda wonky here. I'd go for this instead:
> my $volid = PVE::Storage::vdisk_alloc(
> $cfg, $storeid, $param->{vmid}, $param->{format}, $size, { name => $name });
>
> return $volid;
> }});
---snip 8<---
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 0134893..3776565 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1041,8 +1041,34 @@ sub unmap_volume {
> return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
> }
>
> -sub vdisk_alloc {
> - my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
> +=head3 vdisk_alloc($cfg, $storeid, $vmid, $size, %opts)
It's missing $fmt here.
> +
> +Allocates a volume on the storage with the identifier C<$storeid> (defined in the storage config
> +C<$cfg>) for the VM with the id C<$vmid> with format C<$fmt> and a size of C<$size> kilobytes.
> +
> +The format C<$fmt> can be C<'raw'>, C<'qcow2'>, C<'subvol'> or C<'vmdk'>. If C<$fmt> is left
> +undefined, it will fallback on the default format of the storage type of C<$storeid>.
> +
> +Optionally, the following key-value pairs are available for C<%opts>:
> +
> +=over
> +
> +=item * C<< $name => $string >>
> +
> +Specifies the name of the new volume.
> +
> +If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
This might be true for our plugins, but other plugins might not use this
method. I'd just mention that it will be generated, but not how.
> +
> +=back
> +
> +Returns the identifier for the new volume in the format C<"$storeid:$volname">.
> +
> +=cut
> +
> +sub vdisk_alloc : prototype($$$$$;%) {
> + my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
> +
> + my $name = $opts->{name};
>
> die "no storage ID specified\n" if !$storeid;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument
2025-02-20 8:49 ` Fiona Ebner
@ 2025-02-20 9:34 ` Daniel Kral
0 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 9:34 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 09:49, Fiona Ebner wrote:
> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>> Moves the optional parameter `name` into an optional hash argument at
>> the end of the function.
>>
>> This allows to add more optional arguments in the future and makes the
>> function call easier to follow when a name is not required.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>
> While the cover letter already talks about it, it never hurts to
> explicitly mention that this requires a versioned breaks for qemu-server
> and pve-container again in the patch itself.
>
>> changes since v1:
>> - new!
>>
>> src/PVE/API2/Storage/Content.pm | 6 +++---
>> src/PVE/GuestImport.pm | 2 +-
>> src/PVE/Storage.pm | 30 ++++++++++++++++++++++++++++--
>> src/test/run_test_zfspoolplugin.pl | 8 ++++----
>> 4 files changed, 36 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
>> index fe0ad4a..77924a6 100644
>> --- a/src/PVE/API2/Storage/Content.pm
>> +++ b/src/PVE/API2/Storage/Content.pm
>> @@ -220,9 +220,9 @@ __PACKAGE__->register_method ({
>>
>> my $cfg = PVE::Storage::config();
>>
>> - my $volid = PVE::Storage::vdisk_alloc ($cfg, $storeid, $param->{vmid},
>> - $param->{format},
>> - $name, $size);
>> + my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $param->{vmid}, $param->{format}, $size, {
>> + name => $name,
>> + });
>
> Style nit: the first line is still too long and just having the last
> param split out like this looks kinda wonky here. I'd go for this instead:
Will do so (also at other locations where this happened)!
>
>> my $volid = PVE::Storage::vdisk_alloc(
>> $cfg, $storeid, $param->{vmid}, $param->{format}, $size, { name => $name });
>
>
>>
>> return $volid;
>> }});
>
> ---snip 8<---
>
>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>> index 0134893..3776565 100755
>> --- a/src/PVE/Storage.pm
>> +++ b/src/PVE/Storage.pm
>> @@ -1041,8 +1041,34 @@ sub unmap_volume {
>> return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
>> }
>>
>> -sub vdisk_alloc {
>> - my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
>> +=head3 vdisk_alloc($cfg, $storeid, $vmid, $size, %opts)
>
> It's missing $fmt here.
Sorry, yes it does! Thanks for spotting this and will take more care
with the documentation when preparing the v3.
>
>> +
>> +Allocates a volume on the storage with the identifier C<$storeid> (defined in the storage config
>> +C<$cfg>) for the VM with the id C<$vmid> with format C<$fmt> and a size of C<$size> kilobytes.
>> +
>> +The format C<$fmt> can be C<'raw'>, C<'qcow2'>, C<'subvol'> or C<'vmdk'>. If C<$fmt> is left
>> +undefined, it will fallback on the default format of the storage type of C<$storeid>.
>> +
>> +Optionally, the following key-value pairs are available for C<%opts>:
>> +
>> +=over
>> +
>> +=item * C<< $name => $string >>
>> +
>> +Specifies the name of the new volume.
>> +
>> +If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
>
> This might be true for our plugins, but other plugins might not use this
> method. I'd just mention that it will be generated, but not how.
Will do so!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (3 preceding siblings ...)
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument Daniel Kral
@ 2025-02-11 16:07 ` Daniel Kral
2025-02-20 9:03 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage " Daniel Kral
` (27 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:07 UTC (permalink / raw)
To: pve-devel
Allow a caller to specify the volume's intended content type and assert
whether the specified content type may be stored on the specified
storage before allocating any volume.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk`
src/PVE/Storage.pm | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 3776565..96d4e41 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1059,6 +1059,13 @@ Specifies the name of the new volume.
If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
+=item * C<< $vtype => $string >>
+
+Specifies the content type of the new volume, which can be one of C<'images'>, C<'rootdir'>,
+C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>.
+
+If set, this will assert whether the storage supports the specified content type.
+
=back
Returns the identifier for the new volume in the format C<"$storeid:$volname">.
@@ -1069,6 +1076,7 @@ sub vdisk_alloc : prototype($$$$$;%) {
my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
my $name = $opts->{name};
+ my $vtype = $opts->{vtype};
die "no storage ID specified\n" if !$storeid;
@@ -1086,6 +1094,8 @@ sub vdisk_alloc : prototype($$$$$;%) {
activate_storage($cfg, $storeid);
+ assert_content_type_supported($cfg, $storeid, $vtype) if $vtype;
+
my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
# lock shared storage
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type Daniel Kral
@ 2025-02-20 9:03 ` Fiona Ebner
2025-02-20 10:40 ` Fabian Grünbichler
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 9:03 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> Allow a caller to specify the volume's intended content type and assert
> whether the specified content type may be stored on the specified
> storage before allocating any volume.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk`
>
> src/PVE/Storage.pm | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 3776565..96d4e41 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1059,6 +1059,13 @@ Specifies the name of the new volume.
>
> If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
>
> +=item * C<< $vtype => $string >>
> +
> +Specifies the content type of the new volume, which can be one of C<'images'>, C<'rootdir'>,
> +C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>.
Hmm, vdisk_alloc() is only for allocating guest and import images. So I
think the other content types should be prohibited here (can still be
extended later if it ever comes up).
We plan to better distinguish between 'rootdir' and 'images' in the
future, so I think we should aim to make the $vtype parameter even
required here. Not quite possible yet, because that would require
breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
all other callers already use it.
So my question is if we should rather make it a required parameter
already rather than putting it into $opts? I mean while supporting
passing in undef too, until we are ready to require it for 'pvesm alloc'
too.
@Fabian sounds good to you too?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-20 9:03 ` Fiona Ebner
@ 2025-02-20 10:40 ` Fabian Grünbichler
2025-02-20 10:48 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Fabian Grünbichler @ 2025-02-20 10:40 UTC (permalink / raw)
To: Daniel Kral, Fiona Ebner, Proxmox VE development discussion
Quoting Fiona Ebner (2025-02-20 10:03:09)
> Am 11.02.25 um 17:07 schrieb Daniel Kral:
> > Allow a caller to specify the volume's intended content type and assert
> > whether the specified content type may be stored on the specified
> > storage before allocating any volume.
> >
> > Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> > ---
> > changes since v1:
> > - add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk`
> >
> > src/PVE/Storage.pm | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> > index 3776565..96d4e41 100755
> > --- a/src/PVE/Storage.pm
> > +++ b/src/PVE/Storage.pm
> > @@ -1059,6 +1059,13 @@ Specifies the name of the new volume.
> >
> > If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
> >
> > +=item * C<< $vtype => $string >>
> > +
> > +Specifies the content type of the new volume, which can be one of C<'images'>, C<'rootdir'>,
> > +C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>.
>
> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
> think the other content types should be prohibited here (can still be
> extended later if it ever comes up).
>
> We plan to better distinguish between 'rootdir' and 'images' in the
> future, so I think we should aim to make the $vtype parameter even
> required here. Not quite possible yet, because that would require
> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
> all other callers already use it.
>
> So my question is if we should rather make it a required parameter
> already rather than putting it into $opts? I mean while supporting
> passing in undef too, until we are ready to require it for 'pvesm alloc'
> too.
>
> @Fabian sounds good to you too?
we could also make it required for real in vdisk_alloc, and make `pvesm alloc`
auto-select one of the allowed ones based on the storage config and error out
if the storage doesn't support either rootdir or images? or use a different
"magic" placeholder value like "any" - using undef is a bit opaque and could
happen by accident, although it is not very likely for this hash ;) then when
we introduce properly scoped volume names, we can replace that fallback logic
in `pvesm alloc` with properly setting *either* rootdir or images, depending on
what gets allocated?
OTOH, vdisk_alloc doesn't have too call sites anyway, so doing $opts now, and
then updating those when it becomes a regular argument would also work fine I
think.. the only downside to that approach is that we might miss setting the
option for newly introduce callers in the meantime..
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-20 10:40 ` Fabian Grünbichler
@ 2025-02-20 10:48 ` Fiona Ebner
2025-02-20 12:33 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 10:48 UTC (permalink / raw)
To: Fabian Grünbichler, Daniel Kral, Proxmox VE development discussion
Am 20.02.25 um 11:40 schrieb Fabian Grünbichler:
> Quoting Fiona Ebner (2025-02-20 10:03:09)
>> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>>> Allow a caller to specify the volume's intended content type and assert
>>> whether the specified content type may be stored on the specified
>>> storage before allocating any volume.
>>>
>>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>>> ---
>>> changes since v1:
>>> - add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk`
>>>
>>> src/PVE/Storage.pm | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>>> index 3776565..96d4e41 100755
>>> --- a/src/PVE/Storage.pm
>>> +++ b/src/PVE/Storage.pm
>>> @@ -1059,6 +1059,13 @@ Specifies the name of the new volume.
>>>
>>> If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
>>>
>>> +=item * C<< $vtype => $string >>
>>> +
>>> +Specifies the content type of the new volume, which can be one of C<'images'>, C<'rootdir'>,
>>> +C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>.
>>
>> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
>> think the other content types should be prohibited here (can still be
>> extended later if it ever comes up).
>>
>> We plan to better distinguish between 'rootdir' and 'images' in the
>> future, so I think we should aim to make the $vtype parameter even
>> required here. Not quite possible yet, because that would require
>> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
>> all other callers already use it.
>>
>> So my question is if we should rather make it a required parameter
>> already rather than putting it into $opts? I mean while supporting
>> passing in undef too, until we are ready to require it for 'pvesm alloc'
>> too.
>>
>> @Fabian sounds good to you too?
>
> we could also make it required for real in vdisk_alloc, and make `pvesm alloc`
> auto-select one of the allowed ones based on the storage config and error out
> if the storage doesn't support either rootdir or images? or use a different
> "magic" placeholder value like "any" - using undef is a bit opaque and could
> happen by accident, although it is not very likely for this hash ;) then when
> we introduce properly scoped volume names, we can replace that fallback logic
> in `pvesm alloc` with properly setting *either* rootdir or images, depending on
> what gets allocated?
Sounds good to me. I'm in favor of making it required already, since we
already need versioned breaks for the series.
>
> OTOH, vdisk_alloc doesn't have too call sites anyway, so doing $opts now, and
> then updating those when it becomes a regular argument would also work fine I
> think.. the only downside to that approach is that we might miss setting the
> option for newly introduce callers in the meantime..
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-20 10:48 ` Fiona Ebner
@ 2025-02-20 12:33 ` Daniel Kral
2025-02-20 13:09 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 12:33 UTC (permalink / raw)
To: Fiona Ebner, Fabian Grünbichler, Proxmox VE development discussion
On 2/20/25 11:48, Fiona Ebner wrote:
> Am 20.02.25 um 11:40 schrieb Fabian Grünbichler:
>> Quoting Fiona Ebner (2025-02-20 10:03:09)
>>> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
>>> think the other content types should be prohibited here (can still be
>>> extended later if it ever comes up).
>>>
>>> We plan to better distinguish between 'rootdir' and 'images' in the
>>> future, so I think we should aim to make the $vtype parameter even
>>> required here. Not quite possible yet, because that would require
>>> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
>>> all other callers already use it.
>>>
>>> So my question is if we should rather make it a required parameter
>>> already rather than putting it into $opts? I mean while supporting
>>> passing in undef too, until we are ready to require it for 'pvesm alloc'
>>> too.
>>>
>>> @Fabian sounds good to you too?
>>
>> we could also make it required for real in vdisk_alloc, and make `pvesm alloc`
>> auto-select one of the allowed ones based on the storage config and error out
>> if the storage doesn't support either rootdir or images? or use a different
>> "magic" placeholder value like "any" - using undef is a bit opaque and could
>> happen by accident, although it is not very likely for this hash ;) then when
>> we introduce properly scoped volume names, we can replace that fallback logic
>> in `pvesm alloc` with properly setting *either* rootdir or images, depending on
>> what gets allocated?
>
> Sounds good to me. I'm in favor of making it required already, since we
> already need versioned breaks for the series.
Sounds like a good plan, then I'll move the $vtype parameter into the
signature itself in a v3 and implement the behavior suggested by
@Fabian, thanks!
Thinking about the changes for PVE 9, I'd best retrieve the type of the
resource behind $vmid with `PVE::Cluster::get_vmlist()` to select
between the two content types, right?
>
>>
>> OTOH, vdisk_alloc doesn't have too call sites anyway, so doing $opts now, and
>> then updating those when it becomes a regular argument would also work fine I
>> think.. the only downside to that approach is that we might miss setting the
>> option for newly introduce callers in the meantime..
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type
2025-02-20 12:33 ` Daniel Kral
@ 2025-02-20 13:09 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 13:09 UTC (permalink / raw)
To: Daniel Kral, Fabian Grünbichler, Proxmox VE development discussion
Am 20.02.25 um 13:33 schrieb Daniel Kral:
> On 2/20/25 11:48, Fiona Ebner wrote:
>> Am 20.02.25 um 11:40 schrieb Fabian Grünbichler:
>>> Quoting Fiona Ebner (2025-02-20 10:03:09)
>>>> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
>>>> think the other content types should be prohibited here (can still be
>>>> extended later if it ever comes up).
>>>>
>>>> We plan to better distinguish between 'rootdir' and 'images' in the
>>>> future, so I think we should aim to make the $vtype parameter even
>>>> required here. Not quite possible yet, because that would require
>>>> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and
>>>> have
>>>> all other callers already use it.
>>>>
>>>> So my question is if we should rather make it a required parameter
>>>> already rather than putting it into $opts? I mean while supporting
>>>> passing in undef too, until we are ready to require it for 'pvesm
>>>> alloc'
>>>> too.
>>>>
>>>> @Fabian sounds good to you too?
>>>
>>> we could also make it required for real in vdisk_alloc, and make
>>> `pvesm alloc`
>>> auto-select one of the allowed ones based on the storage config and
>>> error out
>>> if the storage doesn't support either rootdir or images? or use a
>>> different
>>> "magic" placeholder value like "any" - using undef is a bit opaque
>>> and could
>>> happen by accident, although it is not very likely for this hash ;)
>>> then when
>>> we introduce properly scoped volume names, we can replace that
>>> fallback logic
>>> in `pvesm alloc` with properly setting *either* rootdir or images,
>>> depending on
>>> what gets allocated?
>>
>> Sounds good to me. I'm in favor of making it required already, since we
>> already need versioned breaks for the series.
>
> Sounds like a good plan, then I'll move the $vtype parameter into the
> signature itself in a v3 and implement the behavior suggested by
> @Fabian, thanks!
>
> Thinking about the changes for PVE 9, I'd best retrieve the type of the
> resource behind $vmid with `PVE::Cluster::get_vmlist()` to select
> between the two content types, right?
For PVE 9, we might already have encoded the type in the name.
For now, you could do as Fabian suggested, i.e. either just look at the
storage config or at support a placeholder value. The storage layer
should not be concerned with guests. The single existing get_vmlist()
call in the storage modules is already frowned upon :P and the guest
might not exist yet.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage content type
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (4 preceding siblings ...)
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-19 15:45 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage Daniel Kral
` (26 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Tests whether when running `config_to_command` it will correctly fail
with an error message that a volume cannot be used if the underlying
storage does not support its content type.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- no changes except removing unrelated diff
test/cfg2cmd/unsupported-storage-content-type.conf | 3 +++
test/run_config2command_tests.pl | 4 ++++
2 files changed, 7 insertions(+)
create mode 100644 test/cfg2cmd/unsupported-storage-content-type.conf
diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
new file mode 100644
index 00000000..e33165a8
--- /dev/null
+++ b/test/cfg2cmd/unsupported-storage-content-type.conf
@@ -0,0 +1,3 @@
+# TEST: Unsupported storage content type in a volume disk
+# EXPECT_ERROR: storage 'noimages' does not support content-type 'images'
+scsi0: noimages:8006/vm-8006-disk-0.raw,iothread=1,size=32G
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 2feebd4a..440682d4 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -31,6 +31,10 @@ my $base_env = {
type => 'dir',
shared => 0,
},
+ noimages => {
+ path => '/var/lib/vz',
+ type => 'dir',
+ },
'btrfs-store' => {
content => {
images => 1,
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage content type
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage " Daniel Kral
@ 2025-02-19 15:45 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-19 15:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Tests whether when running `config_to_command` it will correctly fail
> with an error message that a volume cannot be used if the underlying
> storage does not support its content type.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - no changes except removing unrelated diff
>
> test/cfg2cmd/unsupported-storage-content-type.conf | 3 +++
> test/run_config2command_tests.pl | 4 ++++
> 2 files changed, 7 insertions(+)
> create mode 100644 test/cfg2cmd/unsupported-storage-content-type.conf
>
> diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
> new file mode 100644
> index 00000000..e33165a8
> --- /dev/null
> +++ b/test/cfg2cmd/unsupported-storage-content-type.conf
> @@ -0,0 +1,3 @@
> +# TEST: Unsupported storage content type in a volume disk
> +# EXPECT_ERROR: storage 'noimages' does not support content-type 'images'
> +scsi0: noimages:8006/vm-8006-disk-0.raw,iothread=1,size=32G
> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
> index 2feebd4a..440682d4 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -31,6 +31,10 @@ my $base_env = {
> type => 'dir',
> shared => 0,
> },
> + noimages => {
> + path => '/var/lib/vz',
> + type => 'dir',
> + },
I'd add some other content type in this hash. Because we have default
content types, this is not a hash that could be reached by parsing a
storage configuration. Just to avoid any potential (future) breakage
with not having a 'content' key at all.
> 'btrfs-store' => {
> content => {
> images => 1,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (5 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: " Daniel Kral
` (25 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Asserts whether the target storage supports storing VM images before
moving a VM volume to the target storage.
Without the check in place, a VM volume can be moved to a storage, which
does not support VM images, but won't be able to start since any
attached volume must be stored on a supported storage.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- patch without any helpers needed
PVE/API2/Qemu.pm | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 295260e7..52234afd 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4233,6 +4233,10 @@ __PACKAGE__->register_method({
die "you can't move to the same storage with same format\n"
if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
+ my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
+ raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
+ if !$scfg->{content}->{images};
+
# this only checks snapshots because $disk is passed!
my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
$storecfg,
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage Daniel Kral
@ 2025-02-20 14:04 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:04 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Asserts whether the target storage supports storing VM images before
> moving a VM volume to the target storage.
>
> Without the check in place, a VM volume can be moved to a storage, which
> does not support VM images, but won't be able to start since any
> attached volume must be stored on a supported storage.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - patch without any helpers needed
>
> PVE/API2/Qemu.pm | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 295260e7..52234afd 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4233,6 +4233,10 @@ __PACKAGE__->register_method({
> die "you can't move to the same storage with same format\n"
> if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
>
> + my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
> + raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
> + if !$scfg->{content}->{images};
> +
> # this only checks snapshots because $disk is passed!
> my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
> $storecfg,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: assert content type support for target storage
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (6 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access Daniel Kral
` (24 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Asserts whether the target storage supports storing VM images before
cloning a VM and its volumes to the target storage.
Without the check in place, a VMs volumes can be cloned to a storage,
which does not support VM images, but won't be able to start since any
attached volume must be stored on a supported storage.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new! (was fixed without special notice in rfc)
PVE/API2/Qemu.pm | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 52234afd..370036b8 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3851,13 +3851,15 @@ __PACKAGE__->register_method({
my $storecfg = PVE::Storage::config();
if ($storage) {
- # check if storage is enabled on local node
- PVE::Storage::storage_check_enabled($storecfg, $storage);
+ # check if storage is enabled on local node and supports vm images
+ my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
+ raise_param_exc({ storage => "storage '$storage' does not support vm images" })
+ if !$scfg->{content}->{images};
+
if ($target) {
# check if storage is available on target node
PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
# clone only works if target storage is shared
- my $scfg = PVE::Storage::storage_config($storecfg, $storage);
die "can't clone to non-shared storage '$storage'\n"
if !$scfg->{shared};
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: assert content type support for target storage
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: " Daniel Kral
@ 2025-02-20 14:04 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:04 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Asserts whether the target storage supports storing VM images before
> cloning a VM and its volumes to the target storage.
>
> Without the check in place, a VMs volumes can be cloned to a storage,
> which does not support VM images, but won't be able to start since any
> attached volume must be stored on a supported storage.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new! (was fixed without special notice in rfc)
>
> PVE/API2/Qemu.pm | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 52234afd..370036b8 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3851,13 +3851,15 @@ __PACKAGE__->register_method({
> my $storecfg = PVE::Storage::config();
>
> if ($storage) {
> - # check if storage is enabled on local node
> - PVE::Storage::storage_check_enabled($storecfg, $storage);
> + # check if storage is enabled on local node and supports vm images
> + my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
> + raise_param_exc({ storage => "storage '$storage' does not support vm images" })
> + if !$scfg->{content}->{images};
> +
> if ($target) {
> # check if storage is available on target node
> PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
> # clone only works if target storage is shared
> - my $scfg = PVE::Storage::storage_config($storecfg, $storage);
> die "can't clone to non-shared storage '$storage'\n"
> if !$scfg->{shared};
> }
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (7 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter " Daniel Kral
` (23 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Remove variable `$size`, which is not used here and likely a copy-paste
redundancy from the `create_disks` subroutine.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
PVE/API2/Qemu.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 370036b8..5ac61aa5 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -148,7 +148,7 @@ my $check_storage_access = sub {
} elsif ($isCDROM && ($volid eq 'cdrom')) {
$rpcenv->check($authuser, "/", ['Sys.Console']);
} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
- my ($storeid, $size) = ($2 || $default_storage, $3);
+ my $storeid = $2 || $default_storage;
die "no storage ID specified (and no default storage)\n" if !$storeid;
$rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access Daniel Kral
@ 2025-02-20 14:04 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:04 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Remove variable `$size`, which is not used here and likely a copy-paste
> redundancy from the `create_disks` subroutine.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new!
>
> PVE/API2/Qemu.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 370036b8..5ac61aa5 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -148,7 +148,7 @@ my $check_storage_access = sub {
> } elsif ($isCDROM && ($volid eq 'cdrom')) {
> $rpcenv->check($authuser, "/", ['Sys.Console']);
> } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
> - my ($storeid, $size) = ($2 || $default_storage, $3);
> + my $storeid = $2 || $default_storage;
> die "no storage ID specified (and no default storage)\n" if !$storeid;
> $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter in check_storage_access
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (8 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:09 ` Fiona Ebner
2025-02-20 14:15 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images Daniel Kral
` (22 subsequent siblings)
32 siblings, 2 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Since 0541eeb8 ("use property strings for drive options") the user input
of a volume with allocation support must be a pair of a PVE-managed
storage and an arbitrary string (i.e. the volume name or the size of a
new disk in GB) [0]. Therefore, the `$volid` must always be the string
"$storeid:$volname_or_size" for cloudinit images and new disks.
Therefore, the `$default_storage` parameter is redundant.
Remove it as it is rejected by `verify_volume_id_or_qm_path` for
allocatable disk drives before calling this subroutine anyway, which is
used by both API handlers, i.e. `create_vm` and `update_vm`, that call
the subroutine.
[0] except the special cases "none", "cdrom" and absolute paths, which
were introduced some time later with `pve-volume-id-or-absolute-path`
and `pve-volume-id-or-qm-path`.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
PVE/API2/Qemu.pm | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 5ac61aa5..2a2d971e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -133,7 +133,7 @@ my $check_drive_param = sub {
};
my $check_storage_access = sub {
- my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_;
+ my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $extraction_storage) = @_;
$foreach_volume_with_alloc->($settings, sub {
my ($ds, $drive) = @_;
@@ -143,13 +143,11 @@ my $check_storage_access = sub {
my $volid = $drive->{file};
my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
- if (!$volid || ($volid eq 'none' || $volid eq 'cloudinit' || (defined($volname) && $volname eq 'cloudinit'))) {
+ if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
# nothing to check
} elsif ($isCDROM && ($volid eq 'cdrom')) {
$rpcenv->check($authuser, "/", ['Sys.Console']);
} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
- my $storeid = $2 || $default_storage;
- die "no storage ID specified (and no default storage)\n" if !$storeid;
$rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
@@ -1106,8 +1104,7 @@ __PACKAGE__->register_method({
if (scalar(keys $param->%*) > 0) {
&$resolve_cdrom_alias($param);
- &$check_storage_access(
- $rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage);
+ &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
&$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
@@ -1874,7 +1871,7 @@ my $update_vm_api = sub {
&$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]);
- &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, undef, $extraction_storage);
+ &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter in check_storage_access
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter " Daniel Kral
@ 2025-02-20 14:09 ` Fiona Ebner
2025-02-21 8:27 ` Daniel Kral
2025-02-20 14:15 ` Fiona Ebner
1 sibling, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:09 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Since 0541eeb8 ("use property strings for drive options") the user input
> of a volume with allocation support must be a pair of a PVE-managed
> storage and an arbitrary string (i.e. the volume name or the size of a
> new disk in GB) [0]. Therefore, the `$volid` must always be the string
> "$storeid:$volname_or_size" for cloudinit images and new disks.
> Therefore, the `$default_storage` parameter is redundant.
>
> Remove it as it is rejected by `verify_volume_id_or_qm_path` for
> allocatable disk drives before calling this subroutine anyway, which is
> used by both API handlers, i.e. `create_vm` and `update_vm`, that call
> the subroutine.
>
> [0] except the special cases "none", "cdrom" and absolute paths, which
> were introduced some time later with `pve-volume-id-or-absolute-path`
> and `pve-volume-id-or-qm-path`.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - new!
>
> PVE/API2/Qemu.pm | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 5ac61aa5..2a2d971e 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -133,7 +133,7 @@ my $check_drive_param = sub {
> };
>
> my $check_storage_access = sub {
> - my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_;
> + my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $extraction_storage) = @_;
>
> $foreach_volume_with_alloc->($settings, sub {
> my ($ds, $drive) = @_;
> @@ -143,13 +143,11 @@ my $check_storage_access = sub {
> my $volid = $drive->{file};
> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>
> - if (!$volid || ($volid eq 'none' || $volid eq 'cloudinit' || (defined($volname) && $volname eq 'cloudinit'))) {
> + if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
> # nothing to check
> } elsif ($isCDROM && ($volid eq 'cdrom')) {
> $rpcenv->check($authuser, "/", ['Sys.Console']);
> } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
> - my $storeid = $2 || $default_storage;
The rest looks fine, but I'd rather keep the assignment with the result
from the regex match here. Because otherwise, the code would rely on
parse_volume_id() to work for everything matching the regex and that is
a pretty implicit assumption and might not stay true in the future.
> - die "no storage ID specified (and no default storage)\n" if !$storeid;
> $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
> @@ -1106,8 +1104,7 @@ __PACKAGE__->register_method({
> if (scalar(keys $param->%*) > 0) {
> &$resolve_cdrom_alias($param);
>
> - &$check_storage_access(
> - $rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage);
> + &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
>
> &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
>
> @@ -1874,7 +1871,7 @@ my $update_vm_api = sub {
>
> &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]);
>
> - &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, undef, $extraction_storage);
> + &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
>
> PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter in check_storage_access
2025-02-20 14:09 ` Fiona Ebner
@ 2025-02-21 8:27 ` Daniel Kral
2025-02-21 9:15 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-21 8:27 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 15:09, Fiona Ebner wrote:
> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>> Since 0541eeb8 ("use property strings for drive options") the user input
>> of a volume with allocation support must be a pair of a PVE-managed
>> storage and an arbitrary string (i.e. the volume name or the size of a
>> new disk in GB) [0]. Therefore, the `$volid` must always be the string
>> "$storeid:$volname_or_size" for cloudinit images and new disks.
>> Therefore, the `$default_storage` parameter is redundant.
>>
>> Remove it as it is rejected by `verify_volume_id_or_qm_path` for
>> allocatable disk drives before calling this subroutine anyway, which is
>> used by both API handlers, i.e. `create_vm` and `update_vm`, that call
>> the subroutine.
>>
>> [0] except the special cases "none", "cdrom" and absolute paths, which
>> were introduced some time later with `pve-volume-id-or-absolute-path`
>> and `pve-volume-id-or-qm-path`.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes since v1:
>> - new!
>>
>> PVE/API2/Qemu.pm | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 5ac61aa5..2a2d971e 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -133,7 +133,7 @@ my $check_drive_param = sub {
>> };
>>
>> my $check_storage_access = sub {
>> - my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_;
>> + my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $extraction_storage) = @_;
>>
>> $foreach_volume_with_alloc->($settings, sub {
>> my ($ds, $drive) = @_;
>> @@ -143,13 +143,11 @@ my $check_storage_access = sub {
>> my $volid = $drive->{file};
>> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>>
>> - if (!$volid || ($volid eq 'none' || $volid eq 'cloudinit' || (defined($volname) && $volname eq 'cloudinit'))) {
>> + if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
>> # nothing to check
>> } elsif ($isCDROM && ($volid eq 'cdrom')) {
>> $rpcenv->check($authuser, "/", ['Sys.Console']);
>> } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
>> - my $storeid = $2 || $default_storage;
>
> The rest looks fine, but I'd rather keep the assignment with the result
> from the regex match here. Because otherwise, the code would rely on
> parse_volume_id() to work for everything matching the regex and that is
> a pretty implicit assumption and might not stay true in the future.
Hm, the reason why I did it this way was so that the following fix for
cloudinit drives could be written a little bit cleaner as they both need
the same storage access checks, so I don't need to duplicate the same
core logic.
I guess I could leave, but I'd have to fallback the `$storeid` provided
by `parse_volume_id()` for the cloudinit case then, as $2 will not
contain anything since the NEW_DISK_RE regex was unsuccessful (captures
only if the $storeid follow a digit). Would that way work for you?
I guess a cleaner way to do this in the future is to make `NEW_DISK_RE`
depend on the regex of the "pve-volume-id" format as much as possible
(e.g. the now required $storeid prefix), but that'd be beyond this patch
series and one should take a closer look before doing this.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter in check_storage_access
2025-02-21 8:27 ` Daniel Kral
@ 2025-02-21 9:15 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-21 9:15 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 21.02.25 um 09:27 schrieb Daniel Kral:
> On 2/20/25 15:09, Fiona Ebner wrote:
>> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>>> Since 0541eeb8 ("use property strings for drive options") the user input
>>> of a volume with allocation support must be a pair of a PVE-managed
>>> storage and an arbitrary string (i.e. the volume name or the size of a
>>> new disk in GB) [0]. Therefore, the `$volid` must always be the string
>>> "$storeid:$volname_or_size" for cloudinit images and new disks.
>>> Therefore, the `$default_storage` parameter is redundant.
>>>
>>> Remove it as it is rejected by `verify_volume_id_or_qm_path` for
>>> allocatable disk drives before calling this subroutine anyway, which is
>>> used by both API handlers, i.e. `create_vm` and `update_vm`, that call
>>> the subroutine.
>>>
>>> [0] except the special cases "none", "cdrom" and absolute paths, which
>>> were introduced some time later with `pve-volume-id-or-absolute-
>>> path`
>>> and `pve-volume-id-or-qm-path`.
>>>
>>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>>> ---
>>> changes since v1:
>>> - new!
>>>
>>> PVE/API2/Qemu.pm | 11 ++++-------
>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>>> index 5ac61aa5..2a2d971e 100644
>>> --- a/PVE/API2/Qemu.pm
>>> +++ b/PVE/API2/Qemu.pm
>>> @@ -133,7 +133,7 @@ my $check_drive_param = sub {
>>> };
>>> my $check_storage_access = sub {
>>> - my ($rpcenv, $authuser, $storecfg, $vmid, $settings,
>>> $default_storage, $extraction_storage) = @_;
>>> + my ($rpcenv, $authuser, $storecfg, $vmid, $settings,
>>> $extraction_storage) = @_;
>>> $foreach_volume_with_alloc->($settings, sub {
>>> my ($ds, $drive) = @_;
>>> @@ -143,13 +143,11 @@ my $check_storage_access = sub {
>>> my $volid = $drive->{file};
>>> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid,
>>> 1);
>>> - if (!$volid || ($volid eq 'none' || $volid eq 'cloudinit' ||
>>> (defined($volname) && $volname eq 'cloudinit'))) {
>>> + if (!$volid || ($volid eq 'none' || (defined($volname) &&
>>> $volname eq 'cloudinit'))) {
>>> # nothing to check
>>> } elsif ($isCDROM && ($volid eq 'cdrom')) {
>>> $rpcenv->check($authuser, "/", ['Sys.Console']);
>>> } elsif (!$isCDROM && ($volid =~
>>> $PVE::QemuServer::Drive::NEW_DISK_RE)) {
>>> - my $storeid = $2 || $default_storage;
>>
>> The rest looks fine, but I'd rather keep the assignment with the result
>> from the regex match here. Because otherwise, the code would rely on
>> parse_volume_id() to work for everything matching the regex and that is
>> a pretty implicit assumption and might not stay true in the future.
>
> Hm, the reason why I did it this way was so that the following fix for
> cloudinit drives could be written a little bit cleaner as they both need
> the same storage access checks, so I don't need to duplicate the same
> core logic.
>
> I guess I could leave, but I'd have to fallback the `$storeid` provided
> by `parse_volume_id()` for the cloudinit case then, as $2 will not
> contain anything since the NEW_DISK_RE regex was unsuccessful (captures
> only if the $storeid follow a digit). Would that way work for you?
Yes. You could also add a $is_new_disk variable and do the check up
front to make the code cleaner.
>
> I guess a cleaner way to do this in the future is to make `NEW_DISK_RE`
> depend on the regex of the "pve-volume-id" format as much as possible
> (e.g. the now required $storeid prefix), but that'd be beyond this patch
> series and one should take a closer look before doing this.
No, I don't think those should be tightly coupled. For example,
parse_volume_id() might be restricted to not work for storeid:number at
some point.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter in check_storage_access
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter " Daniel Kral
2025-02-20 14:09 ` Fiona Ebner
@ 2025-02-20 14:15 ` Fiona Ebner
1 sibling, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:15 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> @@ -1106,8 +1104,7 @@ __PACKAGE__->register_method({
> if (scalar(keys $param->%*) > 0) {
> &$resolve_cdrom_alias($param);
>
> - &$check_storage_access(
> - $rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage);
> + &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
Also style nit: this is still (slightly) too long to fit on one line
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (9 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:23 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage Daniel Kral
` (21 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Asserts whether the target storage supports storing cloudinit images,
i.e. VM images, before creating a cloudinit image on the target storage.
Without the check in place, a cloudinit image can be created on the
storage, which does not support VM images, but won't be able to start
since any attached volume must be stored on a supported storage.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new bug fix! (was indirectly fixed in rfc at commit_cloudinit_image)
PVE/API2/Qemu.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 2a2d971e..9fadf3e5 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -142,12 +142,13 @@ my $check_storage_access = sub {
my $volid = $drive->{file};
my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
+ my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
- if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
+ if (!$volid || $volid eq 'none') {
# nothing to check
} elsif ($isCDROM && ($volid eq 'cdrom')) {
$rpcenv->check($authuser, "/", ['Sys.Console']);
- } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
+ } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
$rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images Daniel Kral
@ 2025-02-20 14:23 ` Fiona Ebner
2025-02-21 8:30 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Asserts whether the target storage supports storing cloudinit images,
> i.e. VM images, before creating a cloudinit image on the target storage.
>
> Without the check in place, a cloudinit image can be created on the
> storage, which does not support VM images, but won't be able to start
> since any attached volume must be stored on a supported storage.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - new bug fix! (was indirectly fixed in rfc at commit_cloudinit_image)
>
> PVE/API2/Qemu.pm | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2a2d971e..9fadf3e5 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -142,12 +142,13 @@ my $check_storage_access = sub {
>
> my $volid = $drive->{file};
> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
> + my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
>
> - if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
> + if (!$volid || $volid eq 'none') {
> # nothing to check
> } elsif ($isCDROM && ($volid eq 'cdrom')) {
> $rpcenv->check($authuser, "/", ['Sys.Console']);
> - } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
> + } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
A cloudinit drive should be a CD-ROM. Can we even reach here?
> $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images
2025-02-20 14:23 ` Fiona Ebner
@ 2025-02-21 8:30 ` Daniel Kral
2025-02-21 9:42 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-21 8:30 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 15:23, Fiona Ebner wrote:
> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>> Asserts whether the target storage supports storing cloudinit images,
>> i.e. VM images, before creating a cloudinit image on the target storage.
>>
>> Without the check in place, a cloudinit image can be created on the
>> storage, which does not support VM images, but won't be able to start
>> since any attached volume must be stored on a supported storage.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes since v1:
>> - new bug fix! (was indirectly fixed in rfc at commit_cloudinit_image)
>>
>> PVE/API2/Qemu.pm | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 2a2d971e..9fadf3e5 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -142,12 +142,13 @@ my $check_storage_access = sub {
>>
>> my $volid = $drive->{file};
>> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>> + my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
>>
>> - if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
>> + if (!$volid || $volid eq 'none') {
>> # nothing to check
>> } elsif ($isCDROM && ($volid eq 'cdrom')) {
>> $rpcenv->check($authuser, "/", ['Sys.Console']);
>> - } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
>> + } elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
>
> A cloudinit drive should be a CD-ROM. Can we even reach here?
Partly, but you're correct that was bad testing on my part, sorry.
As it turns out, it is reachable for some cloudinit images...But it's
not obvious why and I'll fix this and make the why clearer in the v3's
patch message:
`PVE::QemuServer::drive_is_cdrom` checks whether the given drive has the
property key-value pair "media=cdrom". But we neither add that to the
drive string in the pve-manager (which only sends, e.g.
"ide2": "local-lvm:cloudinit,format=qcow2"
for any format) and in the rarer case someone allocates a cloudinit
image via `qm set`, they are likely to not append "media=cdrom"
themselves like:
"qm set -ide2 local-lvm:cloudinit,media=cdrom"
Therefore, the check here doesn't detect a cloudinit image as a cdrom as
long as the "media=cdrom" is not set. But you're correct, with this
patch applied the check would just be skipped like before if someone
explicitly provides this setting.
If it doesn't add too much confusion here and we decide to merge the
cloudinit && new_disk branch here (see my reply for #5), I suggest to
make the `drive_is_cdrom` exclude cloudinit images here in v3 (so that
$isCDROM is only 1 when media=cdrom except for cloudinit drives).
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images
2025-02-21 8:30 ` Daniel Kral
@ 2025-02-21 9:42 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-21 9:42 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 21.02.25 um 09:30 schrieb Daniel Kral:
> On 2/20/25 15:23, Fiona Ebner wrote:
>> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>>> Asserts whether the target storage supports storing cloudinit images,
>>> i.e. VM images, before creating a cloudinit image on the target storage.
>>>
>>> Without the check in place, a cloudinit image can be created on the
>>> storage, which does not support VM images, but won't be able to start
>>> since any attached volume must be stored on a supported storage.
>>>
>>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>>> ---
>>> changes since v1:
>>> - new bug fix! (was indirectly fixed in rfc at commit_cloudinit_image)
>>>
>>> PVE/API2/Qemu.pm | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>>> index 2a2d971e..9fadf3e5 100644
>>> --- a/PVE/API2/Qemu.pm
>>> +++ b/PVE/API2/Qemu.pm
>>> @@ -142,12 +142,13 @@ my $check_storage_access = sub {
>>> my $volid = $drive->{file};
>>> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid,
>>> 1);
>>> + my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
>>> - if (!$volid || ($volid eq 'none' || (defined($volname) &&
>>> $volname eq 'cloudinit'))) {
>>> + if (!$volid || $volid eq 'none') {
>>> # nothing to check
>>> } elsif ($isCDROM && ($volid eq 'cdrom')) {
>>> $rpcenv->check($authuser, "/", ['Sys.Console']);
>>> - } elsif (!$isCDROM && ($volid =~
>>> $PVE::QemuServer::Drive::NEW_DISK_RE)) {
>>> + } elsif (!$isCDROM && ($volid =~
>>> $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
>>
>> A cloudinit drive should be a CD-ROM. Can we even reach here?
>
> Partly, but you're correct that was bad testing on my part, sorry.
>
> As it turns out, it is reachable for some cloudinit images...But it's
> not obvious why and I'll fix this and make the why clearer in the v3's
> patch message:
>
> `PVE::QemuServer::drive_is_cdrom` checks whether the given drive has the
> property key-value pair "media=cdrom". But we neither add that to the
> drive string in the pve-manager (which only sends, e.g.
> "ide2": "local-lvm:cloudinit,format=qcow2"
> for any format) and in the rarer case someone allocates a cloudinit
> image via `qm set`, they are likely to not append "media=cdrom"
> themselves like:
> "qm set -ide2 local-lvm:cloudinit,media=cdrom"
>
> Therefore, the check here doesn't detect a cloudinit image as a cdrom as
> long as the "media=cdrom" is not set. But you're correct, with this
> patch applied the check would just be skipped like before if someone
> explicitly provides this setting.
Right.
>
> If it doesn't add too much confusion here and we decide to merge the
> cloudinit && new_disk branch here (see my reply for #5), I suggest to
> make the `drive_is_cdrom` exclude cloudinit images here in v3 (so that
> $isCDROM is only 1 when media=cdrom except for cloudinit drives).
I don't see a reason to distinguish based on the media=cdrom flag being
set or not. There are two cases to consider:
1. checking access for an existing cloudinit image
This goes to the "else" branch and we should not change that.
2. checking access for allocating a new cloudinit image
The $check_storage_access helper currently returns early in this case. I
think it's fine to have this also take the new disk branch, because
that's what it is. But this will break allocating cloudinit images for
users without Datastore.AllocateSpace on the storage. In the past, we
had the cloudinit UI do a two step, remove volume, allocate new one, but
this was changed a while ago if you remember ;) There could be other API
users that rely on no such permission being required for allocating a
cloudinit drive. If we want to be really careful, we should wait until
PVE 9 with this change and note that the cloudinit_update endpoint
should be used by everybody. Or we could take the stance that no
Datastore.AllocateSpace permission means no Datastore.AllocateSpace
permission even in this edge case and not wait.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (10 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:29 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature Daniel Kral
` (20 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Asserts whether the target storage supports storing VM images before
importing a OVF manifest as a VM to the target storage.
Without the check in place, a VM volume can be imported to a storage,
which does not support VM images, but won't be able to start since any
attached volume must be stored on a supported storage.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new! (was fixed without special notice in rfc, now it's more obvious)
PVE/CLI/qm.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 4214a7ca..58167050 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -756,7 +756,9 @@ __PACKAGE__->register_method ({
die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
my $storecfg = PVE::Storage::config();
- PVE::Storage::storage_check_enabled($storecfg, $storeid);
+ my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
+ raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
+ if !$scfg->{content}->{images};
my $parsed = PVE::GuestImport::OVF::parse_ovf($ovf_file);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage Daniel Kral
@ 2025-02-20 14:29 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:29 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Asserts whether the target storage supports storing VM images before
> importing a OVF manifest as a VM to the target storage.
>
> Without the check in place, a VM volume can be imported to a storage,
> which does not support VM images, but won't be able to start since any
> attached volume must be stored on a supported storage.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new! (was fixed without special notice in rfc, now it's more obvious)
>
> PVE/CLI/qm.pm | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 4214a7ca..58167050 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -756,7 +756,9 @@ __PACKAGE__->register_method ({
>
> die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
> my $storecfg = PVE::Storage::config();
> - PVE::Storage::storage_check_enabled($storecfg, $storeid);
> + my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
> + raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
> + if !$scfg->{content}->{images};
>
> my $parsed = PVE::GuestImport::OVF::parse_ovf($ovf_file);
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (11 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:36 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 09/15] cfg2cmd: improve error message for invalid volume content type Daniel Kral
` (19 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Update any callers of `PVE::Storage::vdisk_alloc` to the updated
function signature, which moves the optional arguments `$fmt` and
`$name` (as these may be passed as undefined), to the optional hash
argument at the end of the argument list.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- update `vdisk_alloc` instead of creating `alloc_volume_disk`
PVE/API2/Qemu.pm | 8 +++++---
PVE/QemuConfig.pm | 4 +++-
PVE/QemuServer.pm | 15 ++++++++-------
PVE/QemuServer/Cloudinit.pm | 4 +++-
PVE/QemuServer/ImportDisk.pm | 3 ++-
PVE/VZDump/QemuServer.pm | 4 +++-
| 3 +--
test/MigrationTest/QmMock.pm | 4 ++--
8 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9fadf3e5..92d24443 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -414,7 +414,9 @@ my sub create_disks : prototype($$$$$$$$$$$) {
# Initial disk created with 4 MB and aligned to 4MB on regeneration
my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
- my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
+ my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $ci_size/1024, {
+ name => $name,
+ });
$disk->{file} = $volid;
$disk->{media} = 'cdrom';
push @$vollist, $volid;
@@ -552,9 +554,9 @@ my sub create_disks : prototype($$$$$$$$$$$) {
} elsif ($ds eq 'tpmstate0') {
# swtpm can only use raw volumes, and uses a fixed size
$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, 'raw', $size);
} else {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $size);
}
# change created disk to a base volume in case the VM is a template
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index ffdf9f03..45660abb 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -226,7 +226,9 @@ sub __snapshot_save_vmstate {
my $name = "vm-$vmid-state-$snapname";
$name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
- my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
+ my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $size*1024, {
+ name => $name,
+ });
my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
# get current QEMU -cpu argument to ensure consistency of custom CPU models
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..2f14bb52 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5471,7 +5471,7 @@ sub vm_migrate_alloc_nbd_disks {
$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
my $size = $drive->{size} / 1024;
- my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, undef, $size);
+ my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size);
my $newdrive = $drive;
$newdrive->{format} = $format;
$newdrive->{file} = $newvolid;
@@ -6684,8 +6684,9 @@ my $restore_allocate_devices = sub {
}
}
- my $volid = PVE::Storage::vdisk_alloc(
- $storecfg, $storeid, $vmid, $d->{format}, $name, $alloc_size);
+ my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $d->{format}, $alloc_size, {
+ name => $name,
+ });
print STDERR "new volume ID is '$volid'\n";
$d->{volid} = $volid;
@@ -8243,9 +8244,9 @@ sub clone_disk {
$size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
}
- $newvolid = PVE::Storage::vdisk_alloc(
- $storecfg, $storeid, $newvmid, $dst_format, $name, ($size/1024)
- );
+ $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $newvmid, $dst_format, ($size/1024), {
+ name => $name,
+ });
push @$newvollist, $newvolid;
PVE::Storage::activate_volumes($storecfg, [$newvolid]);
@@ -8368,7 +8369,7 @@ sub create_efidisk($$$$$$$) {
my $vars_size_b = -s $ovmf_vars;
my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
- my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
+ my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $vars_size);
PVE::Storage::activate_volumes($storecfg, [$volid]);
qemu_img_convert($ovmf_vars, $volid, $vars_size_b, undef, 0);
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index f1143aeb..0dc99e05 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -44,7 +44,9 @@ sub commit_cloudinit_disk {
$volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/;
my $name = $1;
$size = 4 * 1024;
- PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, $size);
+ PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size, {
+ name => $name,
+ });
$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
}
my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 30d56bae..22dbd4fc 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -28,7 +28,8 @@ sub do_import {
warn "format '$format' is not supported by the target storage - using '$dst_format' instead\n"
if $format && $format ne $dst_format;
- my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid, $dst_format, undef, $src_size / 1024);
+ my $dst_volid = PVE::Storage::vdisk_alloc(
+ $storecfg, $storage_id, $vmid, $dst_format, $src_size / 1024);
my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 44b68ae9..c5c94a91 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -561,7 +561,9 @@ my sub allocate_fleecing_images {
my $size = PVE::Tools::convert_size($di->{size}, 'b' => 'kb');
$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
- $self->{storecfg}, $fleecing_storeid, $vmid, $format, $name, $size);
+ $self->{storecfg}, $fleecing_storeid, $vmid, $format, $size, {
+ name => $name,
+ });
$n++;
} else {
--git a/qmextract b/qmextract
index 1466a586..0500c0b8 100755
--- a/qmextract
+++ b/qmextract
@@ -174,8 +174,7 @@ sub extract_archive {
if $format ne 'raw';
}
- my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid,
- $format, undef, $alloc_size);
+ my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, $alloc_size);
print STDERR "new volume ID is '$volid'\n";
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index fb94c58b..a3e6cd1a 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -84,10 +84,10 @@ my $disk_counter = 10;
$MigrationTest::Shared::storage_module->mock(
vdisk_alloc => sub {
- my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
+ my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
die "vdisk_alloc (mocked) - name is not expected to be set - implement me\n"
- if defined($name);
+ if defined($opts->{name});
my $name_without_extension = "vm-${vmid}-disk-${disk_counter}";
$disk_counter++;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature Daniel Kral
@ 2025-02-20 14:36 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:36 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> @@ -552,9 +554,9 @@ my sub create_disks : prototype($$$$$$$$$$$) {
> } elsif ($ds eq 'tpmstate0') {
> # swtpm can only use raw volumes, and uses a fixed size
> $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, 'raw', $size);
Nit: AFAIK, we don't have a clear policy when using 'string' vs "string"
so would be better to avoid mixing in that as an unrelated change.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 09/15] cfg2cmd: improve error message for invalid volume content type
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (12 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 10/15] api: {clone, move}_vm: use volume content type assertion helpers Daniel Kral
` (18 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Improve the error message when a VM start fails due to a volume storage
not supporting the volume's required content type by prefixing it with
the disk handle (e.g. scsi0).
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- none except relocation of helper
PVE/QemuServer.pm | 17 +++--------------
.../unsupported-storage-content-type.conf | 2 +-
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2f14bb52..07b7785f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3827,7 +3827,9 @@ sub config_to_command {
my ($ds, $drive) = @_;
if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
- check_volume_storage_type($storecfg, $drive->{file});
+ eval { PVE::Storage::assert_volume_type_supported($storecfg, $drive->{file}) };
+ die "$ds: $@" if $@;
+
push @$vollist, $drive->{file};
}
@@ -8708,19 +8710,6 @@ sub vm_is_paused {
);
}
-sub check_volume_storage_type {
- my ($storecfg, $vol) = @_;
-
- my ($storeid, $volname) = PVE::Storage::parse_volume_id($vol);
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
- my ($vtype) = PVE::Storage::parse_volname($storecfg, $vol);
-
- die "storage '$storeid' does not support content-type '$vtype'\n"
- if !$scfg->{content}->{$vtype};
-
- return 1;
-}
-
sub add_nets_bridge_fdb {
my ($conf, $vmid) = @_;
diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
index e33165a8..e0903eff 100644
--- a/test/cfg2cmd/unsupported-storage-content-type.conf
+++ b/test/cfg2cmd/unsupported-storage-content-type.conf
@@ -1,3 +1,3 @@
# TEST: Unsupported storage content type in a volume disk
-# EXPECT_ERROR: storage 'noimages' does not support content-type 'images'
+# EXPECT_ERROR: scsi0: storage 'noimages' does not support content type 'images'
scsi0: noimages:8006/vm-8006-disk-0.raw,iothread=1,size=32G
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH qemu-server v2 10/15] api: {clone, move}_vm: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (13 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 09/15] cfg2cmd: improve error message for invalid volume content type Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 11/15] api: {create, update}_vm: " Daniel Kral
` (17 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for cloning VMs to another storage and moving a VM disk to
another storage with the newly introduced helper.
Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- split bug fix from refactoring (here)
- this was patch #3 and #4 before, but squashed them here as they are
very related to each other
PVE/API2/Qemu.pm | 10 +++++-----
PVE/QemuServer.pm | 1 +
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 92d24443..8ce266a3 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3853,8 +3853,8 @@ __PACKAGE__->register_method({
if ($storage) {
# check if storage is enabled on local node and supports vm images
my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
- raise_param_exc({ storage => "storage '$storage' does not support vm images" })
- if !$scfg->{content}->{images};
+ eval { PVE::Storage::assert_content_type_supported($storecfg, $storage, 'images') };
+ raise_param_exc({ storage => "$@" }) if $@;
if ($target) {
# check if storage is available on target node
@@ -4235,9 +4235,9 @@ __PACKAGE__->register_method({
die "you can't move to the same storage with same format\n"
if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
- my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
- raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
- if !$scfg->{content}->{images};
+ PVE::Storage::storage_check_enabled($storecfg, $storeid);
+ eval { PVE::Storage::assert_content_type_supported($storecfg, $storeid, 'images') };
+ raise_param_exc({ storage => "$@" }) if $@;
# this only checks snapshots because $disk is passed!
my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 07b7785f..0cc3285f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8248,6 +8248,7 @@ sub clone_disk {
}
$newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $newvmid, $dst_format, ($size/1024), {
name => $name,
+ vtype => 'images',
});
push @$newvollist, $newvolid;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH qemu-server v2 11/15] api: {create, update}_vm: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (14 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 10/15] api: {clone, move}_vm: use volume content type assertion helpers Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 12/15] api: import{disk, ovf}: " Daniel Kral
` (16 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for creating a VM or updating a VM with newly allocated VM,
EFI, TPM and/or cloudinit images.
Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- squashed previous rfc patch #5 and #6 as they are very related to
another and also made explicit notice of changes to `update_vm` api
PVE/API2/Qemu.pm | 14 +++++++++-----
PVE/QemuServer.pm | 4 +++-
PVE/QemuServer/Cloudinit.pm | 1 +
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8ce266a3..ea164eea 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -150,9 +150,8 @@ my $check_storage_access = sub {
$rpcenv->check($authuser, "/", ['Sys.Console']);
} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
$rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
- raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
- if !$scfg->{content}->{images};
+ eval { PVE::Storage::assert_content_type_supported($storecfg, $storeid, 'images') };
+ raise_param_exc({ storage => "$@" }) if $@;
} else {
PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
if ($storeid) {
@@ -416,6 +415,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $ci_size/1024, {
name => $name,
+ vtype => 'images',
});
$disk->{file} = $volid;
$disk->{media} = 'cdrom';
@@ -554,9 +554,13 @@ my sub create_disks : prototype($$$$$$$$$$$) {
} elsif ($ds eq 'tpmstate0') {
# swtpm can only use raw volumes, and uses a fixed size
$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, 'raw', $size);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, 'raw', $size, {
+ vtype => 'images',
+ });
} else {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $size);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $size, {
+ vtype => 'images',
+ });
}
# change created disk to a base volume in case the VM is a template
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0cc3285f..f3178655 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8372,7 +8372,9 @@ sub create_efidisk($$$$$$$) {
my $vars_size_b = -s $ovmf_vars;
my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
- my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $vars_size);
+ my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $vars_size, {
+ vtype => 'images',
+ });
PVE::Storage::activate_volumes($storecfg, [$volid]);
qemu_img_convert($ovmf_vars, $volid, $vars_size_b, undef, 0);
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 0dc99e05..a566e811 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -46,6 +46,7 @@ sub commit_cloudinit_disk {
$size = 4 * 1024;
PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size, {
name => $name,
+ vtype => 'images',
});
$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH qemu-server v2 12/15] api: import{disk, ovf}: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (15 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 11/15] api: {create, update}_vm: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 13/15] api: qmrestore: " Daniel Kral
` (15 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for importing VM disks or importing OVF manifests as VMs.
Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- made it more apparent that this also affects importovf and fixed a bug
here
PVE/CLI/qm.pm | 10 +++-------
PVE/QemuServer/ImportDisk.pm | 5 +++--
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 58167050..069d3e30 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -607,10 +607,7 @@ __PACKAGE__->register_method ({
my $storecfg = PVE::Storage::config();
PVE::Storage::storage_check_enabled($storecfg, $storeid);
-
- my $target_storage_config = PVE::Storage::storage_config($storecfg, $storeid);
- die "storage $storeid does not support vm images\n"
- if !$target_storage_config->{content}->{images};
+ PVE::Storage::assert_content_type_supported($storecfg, $storeid, 'images');
print "importing disk '$source' to VM $vmid ...\n";
@@ -756,9 +753,8 @@ __PACKAGE__->register_method ({
die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
my $storecfg = PVE::Storage::config();
- my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
- raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
- if !$scfg->{content}->{images};
+ PVE::Storage::storage_check_enabled($storecfg, $storeid);
+ PVE::Storage::assert_content_type_supported($storecfg, $storeid, 'images');
my $parsed = PVE::GuestImport::OVF::parse_ovf($ovf_file);
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 22dbd4fc..1d28d2e1 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -28,8 +28,9 @@ sub do_import {
warn "format '$format' is not supported by the target storage - using '$dst_format' instead\n"
if $format && $format ne $dst_format;
- my $dst_volid = PVE::Storage::vdisk_alloc(
- $storecfg, $storage_id, $vmid, $dst_format, $src_size / 1024);
+ my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid, $dst_format, $src_size / 1024, {
+ vtype => 'images',
+ });
my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH qemu-server v2 13/15] api: qmrestore: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (16 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 12/15] api: import{disk, ovf}: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: " Daniel Kral
` (14 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API and
CLI handler for restoring VMs to a specified target storage.
Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- removed changes to fleecing images and vm state files
PVE/QemuServer.pm | 12 +++++-------
| 4 +++-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f3178655..7025501f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6601,11 +6601,10 @@ my $parse_backup_hints = sub {
my ($rpcenv, $user, $storecfg, $fh, $devinfo, $options) = @_;
my $check_storage = sub { # assert if an image can be allocate
- my ($storeid, $scfg) = @_;
- die "Content type 'images' is not available on storage '$storeid'\n"
- if !$scfg->{content}->{images};
+ my ($storeid) = @_;
$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
if $user ne 'root@pam';
+ PVE::Storage::assert_content_type_supported($storecfg, $storeid, 'images');
};
my $virtdev_hash = {};
@@ -6626,8 +6625,7 @@ my $parse_backup_hints = sub {
$devinfo->{$devname}->{format} = $format;
$devinfo->{$devname}->{storeid} = $storeid;
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
- $check_storage->($storeid, $scfg); # permission and content type check
+ $check_storage->($storeid); # permission and content type check
$virtdev_hash->{$virtdev} = $devinfo->{$devname};
} elsif ($line =~ m/^((?:ide|sata|scsi)\d+):\s*(.*)\s*$/) {
@@ -6637,10 +6635,9 @@ my $parse_backup_hints = sub {
if (drive_is_cloudinit($drive)) {
my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
$storeid = $options->{storage} if defined ($options->{storage});
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
my $format = eval { checked_volume_format($storecfg, $drive->{file}) } // 'raw';
- $check_storage->($storeid, $scfg); # permission and content type check
+ $check_storage->($storeid); # permission and content type check
$virtdev_hash->{$virtdev} = {
format => $format,
@@ -6688,6 +6685,7 @@ my $restore_allocate_devices = sub {
my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $d->{format}, $alloc_size, {
name => $name,
+ vtype => 'images',
});
print STDERR "new volume ID is '$volid'\n";
--git a/qmextract b/qmextract
index 0500c0b8..999fad9e 100755
--- a/qmextract
+++ b/qmextract
@@ -174,7 +174,9 @@ sub extract_archive {
if $format ne 'raw';
}
- my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, $alloc_size);
+ my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, $alloc_size, {
+ vtype => 'images',
+ });
print STDERR "new volume ID is '$volid'\n";
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (17 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 13/15] api: qmrestore: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:46 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 15/15] tree-wide: add todos for breaking content type assertions Daniel Kral
` (13 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handler for migrating VMs (and its subcommands and `mtunnel` endpoint).
Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- preserve `check_storage_access_migrate` and make it a wrapper instead
of replacing each occurrence with the new helper
PVE/API2/Qemu.pm | 8 ++------
PVE/QemuMigrate.pm | 16 +++++++---------
PVE/QemuServer.pm | 19 ++++++++++---------
3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index ea164eea..52e6a3d7 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -244,13 +244,9 @@ my $check_storage_access_clone = sub {
my $check_storage_access_migrate = sub {
my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
- PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
-
$rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
-
- my $scfg = PVE::Storage::storage_config($storecfg, $storage);
- die "storage '$storage' does not support vm images\n"
- if !$scfg->{content}->{images};
+ PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
+ PVE::Storage::assert_content_type_supported($storecfg, $storage, 'images', $node);
};
my $import_from_volid = sub {
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index ed5ede30..827e54b7 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -158,15 +158,13 @@ sub target_storage_check_available {
my ($self, $storecfg, $targetsid, $volid) = @_;
if (!$self->{opts}->{remote}) {
- # check if storage is available on target node
- my $target_scfg = PVE::Storage::storage_check_enabled(
- $storecfg,
- $targetsid,
- $self->{node},
- );
- my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
- die "$volid: content type '$vtype' is not available on storage '$targetsid'\n"
- if !$target_scfg->{content}->{$vtype};
+ # check if storage is available on target node and supports the volume's content type
+ eval {
+ PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
+ my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
+ PVE::Storage::assert_content_type_supported($storecfg, $targetsid, $vtype);
+ };
+ die "$volid: $@" if $@;
}
}
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 7025501f..6678e2f7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2541,14 +2541,13 @@ sub check_storage_availability {
my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
return if !$sid;
- # check if storage is available on both nodes
- my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
- PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
-
- my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
-
- die "$volid: content type '$vtype' is not available on storage '$sid'\n"
- if !$scfg->{content}->{$vtype};
+ # check if storage is available on both nodes and supports the volume's content type
+ eval {
+ PVE::Storage::storage_check_enabled($storecfg, $sid);
+ PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
+ PVE::Storage::assert_volume_type_supported($storecfg, $volid);
+ };
+ die "$volid: $@" if $@;
});
}
@@ -5473,7 +5472,9 @@ sub vm_migrate_alloc_nbd_disks {
$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
my $size = $drive->{size} / 1024;
- my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size);
+ my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size, {
+ vtype => 'images',
+ });
my $newdrive = $drive;
$newdrive->{format} = $format;
$newdrive->{file} = $newvolid;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: use volume content type assertion helpers
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: " Daniel Kral
@ 2025-02-20 14:46 ` Fiona Ebner
2025-02-20 17:50 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 14:46 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index ed5ede30..827e54b7 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -158,15 +158,13 @@ sub target_storage_check_available {
> my ($self, $storecfg, $targetsid, $volid) = @_;
>
> if (!$self->{opts}->{remote}) {
> - # check if storage is available on target node
> - my $target_scfg = PVE::Storage::storage_check_enabled(
> - $storecfg,
> - $targetsid,
> - $self->{node},
> - );
> - my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
> - die "$volid: content type '$vtype' is not available on storage '$targetsid'\n"
> - if !$target_scfg->{content}->{$vtype};
> + # check if storage is available on target node and supports the volume's content type
> + eval {
> + PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
> + my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
> + PVE::Storage::assert_content_type_supported($storecfg, $targetsid, $vtype);
(In v3) this needs to pass along the node. And could use
assert_volume_type_supported() or?
> + };
> + die "$volid: $@" if $@;
> }
> }
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: use volume content type assertion helpers
2025-02-20 14:46 ` Fiona Ebner
@ 2025-02-20 17:50 ` Daniel Kral
2025-02-21 9:45 ` Fiona Ebner
0 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 17:50 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 15:46, Fiona Ebner wrote:
> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index ed5ede30..827e54b7 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -158,15 +158,13 @@ sub target_storage_check_available {
>> my ($self, $storecfg, $targetsid, $volid) = @_;
>>
>> if (!$self->{opts}->{remote}) {
>> - # check if storage is available on target node
>> - my $target_scfg = PVE::Storage::storage_check_enabled(
>> - $storecfg,
>> - $targetsid,
>> - $self->{node},
>> - );
>> - my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
>> - die "$volid: content type '$vtype' is not available on storage '$targetsid'\n"
>> - if !$target_scfg->{content}->{$vtype};
>> + # check if storage is available on target node and supports the volume's content type
>> + eval {
>> + PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
>> + my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
>> + PVE::Storage::assert_content_type_supported($storecfg, $targetsid, $vtype);
>
> (In v3) this needs to pass along the node. And could use
> assert_volume_type_supported() or?
Except I'm missing something, in this case not, because we do want to
check whether the target storage $targetsid supports the content type,
but `assert_volume_type_supported` would check whether the current
storage of the volume supports the content type, not the target one.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: use volume content type assertion helpers
2025-02-20 17:50 ` Daniel Kral
@ 2025-02-21 9:45 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-21 9:45 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 20.02.25 um 18:50 schrieb Daniel Kral:
> On 2/20/25 15:46, Fiona Ebner wrote:
>> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>>> index ed5ede30..827e54b7 100644
>>> --- a/PVE/QemuMigrate.pm
>>> +++ b/PVE/QemuMigrate.pm
>>> @@ -158,15 +158,13 @@ sub target_storage_check_available {
>>> my ($self, $storecfg, $targetsid, $volid) = @_;
>>> if (!$self->{opts}->{remote}) {
>>> - # check if storage is available on target node
>>> - my $target_scfg = PVE::Storage::storage_check_enabled(
>>> - $storecfg,
>>> - $targetsid,
>>> - $self->{node},
>>> - );
>>> - my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
>>> - die "$volid: content type '$vtype' is not available on storage
>>> '$targetsid'\n"
>>> - if !$target_scfg->{content}->{$vtype};
>>> + # check if storage is available on target node and supports the
>>> volume's content type
>>> + eval {
>>> + PVE::Storage::storage_check_enabled($storecfg, $targetsid,
>>> $self->{node});
>>> + my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
>>> + PVE::Storage::assert_content_type_supported($storecfg,
>>> $targetsid, $vtype);
>>
>> (In v3) this needs to pass along the node. And could use
>> assert_volume_type_supported() or?
>
> Except I'm missing something, in this case not, because we do want to
> check whether the target storage $targetsid supports the content type,
> but `assert_volume_type_supported` would check whether the current
> storage of the volume supports the content type, not the target one.
Right :) Would be nice to have a short code comment for this to avoid
the question in the future.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH qemu-server v2 15/15] tree-wide: add todos for breaking content type assertions
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (18 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 14:47 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables Daniel Kral
` (12 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Add TODO comments to calls to `vdisk_alloc`, which should be done for
consistency, but would be breaking in the current release since both
snapshot state files and fleecing images are allowed on storages, which
do not support VM images and work without errors currently.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- made this TODO statements instead of changes as these would break
the existing API as both actions are currently totally allowed
PVE/QemuConfig.pm | 1 +
PVE/VZDump/QemuServer.pm | 1 +
2 files changed, 2 insertions(+)
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 45660abb..28f31b6d 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -226,6 +226,7 @@ sub __snapshot_save_vmstate {
my $name = "vm-$vmid-state-$snapname";
$name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
+ # TODO PVE 9: add assertion for supported content type
my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $size*1024, {
name => $name,
});
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index c5c94a91..42e91bc2 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -560,6 +560,7 @@ my sub allocate_fleecing_images {
my $size = PVE::Tools::convert_size($di->{size}, 'b' => 'kb');
+ # TODO PVE 9: add assertion for supported content type
$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
$self->{storecfg}, $fleecing_storeid, $vmid, $format, $size, {
name => $name,
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (19 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 15/15] tree-wide: add todos for breaking content type assertions Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 9:20 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper Daniel Kral
` (11 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Factor out the common read-only variables to allow the second call to
`storage_check_enabled` to be below 100 characters.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC/Migrate.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index e1e6cab..f06a10a 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -71,7 +71,8 @@ sub prepare {
die "can't determine assigned storage for mount point '$ms'\n" if !$storage;
# check if storage is available on both nodes
- my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $storage);
+ my ($storecfg, $node) = $self->@{qw(storecfg node)};
+ my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
my $targetsid = $storage;
@@ -92,7 +93,7 @@ sub prepare {
}
if (!$remote) {
- my $target_scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
+ my $target_scfg = PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
die "$volid: content type 'rootdir' is not available on storage '$targetsid'\n"
if !$target_scfg->{content}->{rootdir};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables Daniel Kral
@ 2025-02-20 9:20 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 9:20 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Factor out the common read-only variables to allow the second call to
> `storage_check_enabled` to be below 100 characters.
>
Just wanted to say in general, there is nothing wrong with using
$self->{node} either, especially if it's only used a single time,
because having an additional variable is more overhead then.
Still, can be fine in this case. But there are other usages of
$self->{node} further below in the branch for remote migration. And the
readability there would benefit more noticeably, because the variable is
used inside a string. So I'd rather go all the way, have the $node
variable be defined at the top level and used throughout the function.
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
> - new!
>
> src/PVE/LXC/Migrate.pm | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
> index e1e6cab..f06a10a 100644
> --- a/src/PVE/LXC/Migrate.pm
> +++ b/src/PVE/LXC/Migrate.pm
> @@ -71,7 +71,8 @@ sub prepare {
> die "can't determine assigned storage for mount point '$ms'\n" if !$storage;
>
> # check if storage is available on both nodes
> - my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $storage);
> + my ($storecfg, $node) = $self->@{qw(storecfg node)};
> + my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
>
> my $targetsid = $storage;
>
> @@ -92,7 +93,7 @@ sub prepare {
> }
>
> if (!$remote) {
> - my $target_scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
> + my $target_scfg = PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
>
> die "$volid: content type 'rootdir' is not available on storage '$targetsid'\n"
> if !$target_scfg->{content}->{rootdir};
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (20 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 10:21 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir Daniel Kral
` (10 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Adds test cases for the alloc_disk wrapper subroutine to ensure that:
- zero-sized volumes are allocated as subvols on path-based storages
- non-zero-sized volumes are allocated as raw on path-based storages
- volumes are allocated as raw on btrfs storages without quotas
- volumes are allocated as subvols on btrfs storages with quotas
- volumes are allocated as subvols on zfs storages
- volumes cannot be allocated on storages that do not support rootdir
These test cases should allow to catch regressions in following changes
to the alloc_disk wrapper.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/test/Makefile | 5 +-
src/test/run_alloc_disk_tests.pl | 149 +++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+), 1 deletion(-)
create mode 100755 src/test/run_alloc_disk_tests.pl
diff --git a/src/test/Makefile b/src/test/Makefile
index 91ae6ff..729f981 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -2,7 +2,7 @@ RUN_USERNS := lxc-usernsexec -m "u:0:`id -u`:1" -m "g:0:`id -g`:1" --
all: test
-test: test_setup test_snapshot test_bindmount test_idmap
+test: test_setup test_snapshot test_bindmount test_idmap test_alloc_disk
test_setup: run_setup_tests.pl
if test -e /run/lock/sbuild; then \
@@ -24,5 +24,8 @@ test_bindmount: bindmount_test.pl
test_idmap: run_idmap_tests.pl
./run_idmap_tests.pl
+test_alloc_disk: run_alloc_disk_tests.pl
+ ./run_alloc_disk_tests.pl
+
clean:
rm -rf tmprootfs
diff --git a/src/test/run_alloc_disk_tests.pl b/src/test/run_alloc_disk_tests.pl
new file mode 100755
index 0000000..b13f5a2
--- /dev/null
+++ b/src/test/run_alloc_disk_tests.pl
@@ -0,0 +1,149 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::Tools;
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::LXC;
+
+my $test_vmid = 100;
+my $test_root_uid = 100000;
+my $test_root_gid = 100000;
+
+my $storage_config = {
+ ids => {
+ local => {
+ content => {
+ rootdir => 1,
+ },
+ path => "/var/lib/vz",
+ type => "dir",
+ shared => 0,
+ },
+ norootdirs => {
+ path => '/var/lib/vz',
+ type => 'dir',
+ },
+ btrfsstore => {
+ content => {
+ rootdir => 1,
+ },
+ path => '/butter/bread',
+ type => 'btrfs',
+ },
+ btrfsquotas => {
+ content => {
+ rootdir => 1,
+ },
+ path => '/butter/bread',
+ type => 'btrfs',
+ quotas => 1,
+ },
+ zfspool0 => {
+ type => 'zfspool',
+ content => {
+ rootdir => 1,
+ },
+ pool => 'rpool0',
+ mountpoint => '/zfspool0',
+ },
+ },
+};
+
+my $storage_module = Test::MockModule->new("PVE::Storage");
+$storage_module->redefine(
+ vdisk_alloc => sub {
+ my ($storecfg, $storage, $vmid, $fmt, $name, $size_kb) = @_;
+
+ $fmt //= '';
+ my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
+
+ return "$storage:$prefix-$vmid-disk-0";
+ },
+);
+
+my $format_disk_called = 0;
+
+my $lxc_module = Test::MockModule->new("PVE::LXC");
+$lxc_module->redefine(
+ format_disk => sub {
+ $format_disk_called = 1;
+ },
+);
+
+my $tests = [
+ {
+ description => 'allocate zero-sized volume on path-based storage',
+ storage => 'local',
+ size_kb => 0,
+ result => ["local:subvol-$test_vmid-disk-0", 1],
+ },
+ {
+ description => 'allocate non-zero-sized volume on path-based storage',
+ should_format_disk => 1,
+ storage => 'local',
+ size_kb => 1024 * 1024,
+ result => ["local:vm-$test_vmid-disk-0", 0],
+ },
+ {
+ description => 'allocate volume on btrfs with quotas disabled',
+ should_format_disk => 1,
+ storage => 'btrfsstore',
+ size_kb => 1024 * 1024,
+ result => ["btrfsstore:vm-$test_vmid-disk-0", 0],
+ },
+ {
+ description => 'allocate volume on btrfs with quotas enabled',
+ storage => 'btrfsquotas',
+ size_kb => 1024 * 1024,
+ result => ["btrfsquotas:subvol-$test_vmid-disk-0", 1],
+ },
+ {
+ description => 'allocate volume on zfspool',
+ storage => 'zfspool0',
+ size_kb => 1024 * 1024,
+ result => ["zfspool0:subvol-$test_vmid-disk-0", 1],
+ },
+ {
+ description => 'allocate volume on storage without rootdir content type support',
+ should_fail => 1,
+ storage => 'norootdirs',
+ size_kb => 1024 * 1024,
+ },
+];
+
+# multiply by 2 because of format_disk test
+plan(tests => 2 * scalar($tests->@*));
+
+for my $case ($tests->@*) {
+ my $should_format_disk = exists($case->{should_format_disk}) ? $case->{should_format_disk} : 0;
+ $format_disk_called = 0;
+
+ my @result = eval {
+ PVE::LXC::alloc_disk(
+ $storage_config,
+ $test_vmid,
+ $case->{storage},
+ $case->{size_kb},
+ $test_root_uid,
+ $test_root_gid
+ )
+ };
+
+ if ($@) {
+ my $should_fail = exists($case->{should_fail}) ? $case->{should_fail} : 0;
+ is(defined($@), $should_fail, "should fail: $case->{description}") || diag explain $@;
+ } else {
+ is_deeply(\@result, $case->{result}, $case->{description});
+ }
+
+ is($format_disk_called, $should_format_disk, "should format_disk: $case->{description}");
+}
+
+done_testing();
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper Daniel Kral
@ 2025-02-20 10:21 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 10:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Adds test cases for the alloc_disk wrapper subroutine to ensure that:
>
> - zero-sized volumes are allocated as subvols on path-based storages
> - non-zero-sized volumes are allocated as raw on path-based storages
> - volumes are allocated as raw on btrfs storages without quotas
> - volumes are allocated as subvols on btrfs storages with quotas
> - volumes are allocated as subvols on zfs storages
> - volumes cannot be allocated on storages that do not support rootdir
>
> These test cases should allow to catch regressions in following changes
> to the alloc_disk wrapper.
Always nice to have such tests up-front :)
> diff --git a/src/test/run_alloc_disk_tests.pl b/src/test/run_alloc_disk_tests.pl
> new file mode 100755
> index 0000000..b13f5a2
> --- /dev/null
> +++ b/src/test/run_alloc_disk_tests.pl
> @@ -0,0 +1,149 @@
> +#!/usr/bin/env perl
> +
> +use strict;
> +use warnings;
> +
> +use lib qw(..);
> +
> +use PVE::Tools;
> +
> +use Test::More;
> +use Test::MockModule;
> +
> +use PVE::LXC;
> +
> +my $test_vmid = 100;
> +my $test_root_uid = 100000;
> +my $test_root_gid = 100000;
> +
> +my $storage_config = {
> + ids => {
> + local => {
> + content => {
> + rootdir => 1,
> + },
> + path => "/var/lib/vz",
> + type => "dir",
> + shared => 0,
> + },
> + norootdirs => {
Similar to the qemu-server patch: I'd add some other content to the
hash, just so it's slightly more realistic.
> + path => '/var/lib/vz',
> + type => 'dir',> + },
> + btrfsstore => {
> + content => {
> + rootdir => 1,
> + },
> + path => '/butter/bread',
> + type => 'btrfs',
> + },
> + btrfsquotas => {
> + content => {
> + rootdir => 1,
> + },
> + path => '/butter/bread',
> + type => 'btrfs',
> + quotas => 1,
> + },
> + zfspool0 => {
> + type => 'zfspool',
> + content => {
> + rootdir => 1,
> + },
> + pool => 'rpool0',
> + mountpoint => '/zfspool0',
> + },
> + },
> +};
> +
> +my $storage_module = Test::MockModule->new("PVE::Storage");
> +$storage_module->redefine(
> + vdisk_alloc => sub {
> + my ($storecfg, $storage, $vmid, $fmt, $name, $size_kb) = @_;
> +
> + $fmt //= '';
> + my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
> +
> + return "$storage:$prefix-$vmid-disk-0";
Nit: To have the tests be slightly more complete, you could have
additional global variables for size and format, similar to how you keep
track of $format_disk_called. But not too important.
> + },
> +);
> +
> +my $format_disk_called = 0;
> +
> +my $lxc_module = Test::MockModule->new("PVE::LXC");
> +$lxc_module->redefine(
> + format_disk => sub {
> + $format_disk_called = 1;
> + },
> +);
> +
> +my $tests = [
> + {
> + description => 'allocate zero-sized volume on path-based storage',
> + storage => 'local',
> + size_kb => 0,
> + result => ["local:subvol-$test_vmid-disk-0", 1],
> + },
> + {
> + description => 'allocate non-zero-sized volume on path-based storage',
> + should_format_disk => 1,
> + storage => 'local',
> + size_kb => 1024 * 1024,
> + result => ["local:vm-$test_vmid-disk-0", 0],
> + },
> + {
> + description => 'allocate volume on btrfs with quotas disabled',
> + should_format_disk => 1,
> + storage => 'btrfsstore',
> + size_kb => 1024 * 1024,
> + result => ["btrfsstore:vm-$test_vmid-disk-0", 0],
> + },
> + {
> + description => 'allocate volume on btrfs with quotas enabled',
> + storage => 'btrfsquotas',
> + size_kb => 1024 * 1024,
> + result => ["btrfsquotas:subvol-$test_vmid-disk-0", 1],
> + },
> + {
> + description => 'allocate volume on zfspool',
> + storage => 'zfspool0',
> + size_kb => 1024 * 1024,
> + result => ["zfspool0:subvol-$test_vmid-disk-0", 1],
> + },
> + {
> + description => 'allocate volume on storage without rootdir content type support',
> + should_fail => 1,
> + storage => 'norootdirs',
> + size_kb => 1024 * 1024,
> + },
> +];
> +
> +# multiply by 2 because of format_disk test
> +plan(tests => 2 * scalar($tests->@*));
> +
> +for my $case ($tests->@*) {
> + my $should_format_disk = exists($case->{should_format_disk}) ? $case->{should_format_disk} : 0;
> + $format_disk_called = 0;
> +
> + my @result = eval {
I'd prefer to be explicit: my ($volid, $needs_chown) = eval {
> + PVE::LXC::alloc_disk(
> + $storage_config,
> + $test_vmid,
> + $case->{storage},
> + $case->{size_kb},
> + $test_root_uid,
> + $test_root_gid
> + )
> + };
> +
Style nit: I'd avoid the blank line here to make it less likely for
future code to sneak in
> + if ($@) {
Style nit: if (my $err = $@) { and then use $err. Because what if
somebody changes the $should_fail assignment to a function call in the
future or adds another line in between, then you might suddenly have a
different $@ in the check below.
> + my $should_fail = exists($case->{should_fail}) ? $case->{should_fail} : 0;
> + is(defined($@), $should_fail, "should fail: $case->{description}") || diag explain $@;
I'd just write 1 instead of defined($@), because we are already in the
failure branch. Note that defined($@) would otherwise return 'undef', so
comparing against 0 wouldn't work either. And note this requires
$should_fail to be exactly 1 and not some other true value.
So I'd prefer using something like:
if ($should_fail) {
pass(...)
} else {
diag ...
fail(...)
}
> + } else {
> + is_deeply(\@result, $case->{result}, $case->{description});
> + }
> +
> + is($format_disk_called, $should_format_disk, "should format_disk: $case->{description}");
> +}
> +
> +done_testing();
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (21 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 12:15 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc Daniel Kral
` (9 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 44e28fc..51457ec 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2158,7 +2158,11 @@ sub alloc_disk {
eval {
my $do_format = 0;
- if ($scfg->{content}->{rootdir} && $scfg->{path}) {
+
+ die "storage '$storage' does not support content type 'rootdir'\n"
+ if !$scfg->{content}->{rootdir};
+
+ if ($scfg->{path}) {
if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
$do_format = 1;
@@ -2169,11 +2173,9 @@ sub alloc_disk {
} elsif ($scfg->{type} eq 'zfspool') {
$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
$needs_chown = 1;
- } elsif ($scfg->{content}->{rootdir}) {
+ } else {
$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
$do_format = 1;
- } else {
- die "content type 'rootdir' is not available or configured on storage '$storage'\n";
}
format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir Daniel Kral
@ 2025-02-20 12:15 ` Fiona Ebner
2025-02-20 17:52 ` Daniel Kral
0 siblings, 1 reply; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 12:15 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
I'd change the title to "alloc disk: fix content type check for ZFS
storages" because the check was missing for that branch ;)
The commit message should mention that there are already earlier checks
for the create operations. Moving a volume to a ZFS storage without
'rootdir' content type was still possible however, which this change
prohibits.
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
With the above:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
I also noticed that we have no check against starting a container with
volumes on a storage that does not support 'rootdir'. We have such a
check for VMs IIRC. Prohibiting that would also be good, but maybe
something for PVE 9 where we can also check for misconfigured
containers/storages via the pve8to9 script up front so users can adapt.
> ---
> changes since v1:
> - new!
>
> src/PVE/LXC.pm | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 44e28fc..51457ec 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2158,7 +2158,11 @@ sub alloc_disk {
>
> eval {
> my $do_format = 0;
> - if ($scfg->{content}->{rootdir} && $scfg->{path}) {
> +
> + die "storage '$storage' does not support content type 'rootdir'\n"
> + if !$scfg->{content}->{rootdir};
> +
> + if ($scfg->{path}) {
> if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
> $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> $do_format = 1;
> @@ -2169,11 +2173,9 @@ sub alloc_disk {
> } elsif ($scfg->{type} eq 'zfspool') {
> $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
> $needs_chown = 1;
> - } elsif ($scfg->{content}->{rootdir}) {
> + } else {
> $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> $do_format = 1;
> - } else {
> - die "content type 'rootdir' is not available or configured on storage '$storage'\n";
> }
> format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
> };
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir
2025-02-20 12:15 ` Fiona Ebner
@ 2025-02-20 17:52 ` Daniel Kral
0 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-20 17:52 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/20/25 13:15, Fiona Ebner wrote:
> I'd change the title to "alloc disk: fix content type check for ZFS
> storages" because the check was missing for that branch ;)
>
> The commit message should mention that there are already earlier checks
> for the create operations. Moving a volume to a ZFS storage without
> 'rootdir' content type was still possible however, which this change
> prohibits.
Nice catch, thanks for pointing that out! Haven't noticed that while
doing the refactor and will adapt the patch message as suggested for the
v3 :).
On 2/20/25 13:15, Fiona Ebner wrote:
> I also noticed that we have no check against starting a container with
> volumes on a storage that does not support 'rootdir'. We have such a
> check for VMs IIRC. Prohibiting that would also be good, but maybe
> something for PVE 9 where we can also check for misconfigured
> containers/storages via the pve8to9 script up front so users can adapt.
Sounds good to have them both act the same in that regard! Put it on my
pve-9 todo list for now.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (22 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 13:11 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments Daniel Kral
` (8 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 51457ec..1152131 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2157,6 +2157,7 @@ sub alloc_disk {
# fixme: use better naming ct-$vmid-disk-X.raw?
eval {
+ my $format = 'raw';
my $do_format = 0;
die "storage '$storage' does not support content type 'rootdir'\n"
@@ -2164,19 +2165,20 @@ sub alloc_disk {
if ($scfg->{path}) {
if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
$do_format = 1;
} else {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
+ $format = 'subvol';
$needs_chown = 1;
}
} elsif ($scfg->{type} eq 'zfspool') {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
+ $format = 'subvol';
$needs_chown = 1;
} else {
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
$do_format = 1;
}
+
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
+
format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
};
if (my $err = $@) {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc Daniel Kral
@ 2025-02-20 13:11 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 13:11 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new!
>
> src/PVE/LXC.pm | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 51457ec..1152131 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2157,6 +2157,7 @@ sub alloc_disk {
> # fixme: use better naming ct-$vmid-disk-X.raw?
>
> eval {
> + my $format = 'raw';
> my $do_format = 0;
>
> die "storage '$storage' does not support content type 'rootdir'\n"
> @@ -2164,19 +2165,20 @@ sub alloc_disk {
>
> if ($scfg->{path}) {
> if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> $do_format = 1;
> } else {
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
> + $format = 'subvol';
> $needs_chown = 1;
> }
> } elsif ($scfg->{type} eq 'zfspool') {
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
> + $format = 'subvol';
> $needs_chown = 1;
> } else {
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> $do_format = 1;
> }
> +
> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
> +
> format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
> };
> if (my $err = $@) {
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (23 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 13:22 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature Daniel Kral
` (7 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Since there are only two distinct outcomes, i.e. raw format + do_format
and subvol format + needs_chown, break down the conditions to reduce the
branching depth.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 1152131..c634b70 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2163,14 +2163,11 @@ sub alloc_disk {
die "storage '$storage' does not support content type 'rootdir'\n"
if !$scfg->{content}->{rootdir};
- if ($scfg->{path}) {
- if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
- $do_format = 1;
- } else {
- $format = 'subvol';
- $needs_chown = 1;
- }
- } elsif ($scfg->{type} eq 'zfspool') {
+ my $is_unsized_on_path = $scfg->{path} && $size_kb <= 0;
+ my $is_btrfs_quotas = $scfg->{path} && $scfg->{type} eq 'btrfs' && $scfg->{quotas};
+ my $is_zfs_pool = !$scfg->{path} && $scfg->{type} eq 'zfspool';
+
+ if ($is_unsized_on_path || $is_btrfs_quotas || $is_zfs_pool) {
$format = 'subvol';
$needs_chown = 1;
} else {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments Daniel Kral
@ 2025-02-20 13:22 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 13:22 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Since there are only two distinct outcomes, i.e. raw format + do_format
> and subvol format + needs_chown, break down the conditions to reduce the
> branching depth.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new!
>
> src/PVE/LXC.pm | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 1152131..c634b70 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2163,14 +2163,11 @@ sub alloc_disk {
> die "storage '$storage' does not support content type 'rootdir'\n"
> if !$scfg->{content}->{rootdir};
>
> - if ($scfg->{path}) {
> - if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
> - $do_format = 1;
> - } else {
> - $format = 'subvol';
> - $needs_chown = 1;
> - }
> - } elsif ($scfg->{type} eq 'zfspool') {
> + my $is_unsized_on_path = $scfg->{path} && $size_kb <= 0;
> + my $is_btrfs_quotas = $scfg->{path} && $scfg->{type} eq 'btrfs' && $scfg->{quotas};
Nit: since the btrfs plugin always has a path, that part can be dropped
> + my $is_zfs_pool = !$scfg->{path} && $scfg->{type} eq 'zfspool';
Nit: since the zfspool plugin never has a path, that part can be dropped
> +
> + if ($is_unsized_on_path || $is_btrfs_quotas || $is_zfs_pool) {
> $format = 'subvol';
> $needs_chown = 1;
> } else {
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (24 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 13:24 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 07/11] alloc_disk: use volume content type assertion helpers Daniel Kral
` (6 subsequent siblings)
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Update the call to `PVE::Storage::vdisk_alloc` to the updated function
signature, which moves the optional argument `$name` (as it may be
passed as undefined), to the optional hash argument at the end of the
argument list.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index c634b70..32a54a9 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2174,7 +2174,7 @@ sub alloc_disk {
$do_format = 1;
}
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb);
format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature Daniel Kral
@ 2025-02-20 13:24 ` Fiona Ebner
0 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-20 13:24 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:08 schrieb Daniel Kral:
> Update the call to `PVE::Storage::vdisk_alloc` to the updated function
> signature, which moves the optional argument `$name` (as it may be
> passed as undefined), to the optional hash argument at the end of the
> argument list.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> changes since v1:
> - new!
>
> src/PVE/LXC.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index c634b70..32a54a9 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2174,7 +2174,7 @@ sub alloc_disk {
> $do_format = 1;
> }
>
> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb);
>
> format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
> };
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread
* [pve-devel] [PATCH container v2 07/11] alloc_disk: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (25 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 08/11] api: create: " Daniel Kral
` (5 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers in the
precondition check path and the call to `vdisk_alloc` in the wrapper for
allocating container's disks.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 32a54a9..1631f82 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2160,8 +2160,7 @@ sub alloc_disk {
my $format = 'raw';
my $do_format = 0;
- die "storage '$storage' does not support content type 'rootdir'\n"
- if !$scfg->{content}->{rootdir};
+ PVE::Storage::assert_content_type_supported($storecfg, $storage, 'rootdir');
my $is_unsized_on_path = $scfg->{path} && $size_kb <= 0;
my $is_btrfs_quotas = $scfg->{path} && $scfg->{type} eq 'btrfs' && $scfg->{quotas};
@@ -2174,7 +2173,9 @@ sub alloc_disk {
$do_format = 1;
}
- $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb);
+ $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb, {
+ vtype => 'rootdir',
+ });
format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH container v2 08/11] api: create: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (26 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 07/11] alloc_disk: use volume content type assertion helpers Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 09/11] migration: prepare: " Daniel Kral
` (4 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of newly introduced content type assertion helpers in the
preconditions check path in the API handler for creating a container.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/API2/LXC.pm | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 7cb5122..8e5e6a6 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -306,10 +306,9 @@ __PACKAGE__->register_method({
my $check_and_activate_storage = sub {
my ($sid) = @_;
- my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $sid, $node);
-
- raise_param_exc({ storage => "storage '$sid' does not support container directories"})
- if !$scfg->{content}->{rootdir};
+ PVE::Storage::storage_check_enabled($storage_cfg, $sid, $node);
+ eval { PVE::Storage::assert_content_type_supported($storage_cfg, $sid, 'rootdir', $node) };
+ raise_param_exc({ storage => "$@" }) if $@;
$rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH container v2 09/11] migration: prepare: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (27 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 08/11] api: create: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 10/11] api: update_vm: " Daniel Kral
` (3 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of newly introduced content type assertion helpers in the
preconditions check and preparation path of the migration API handlers.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/API2/LXC.pm | 8 ++------
src/PVE/LXC/Migrate.pm | 13 ++++++-------
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 8e5e6a6..a945a08 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -43,13 +43,9 @@ BEGIN {
my $check_storage_access_migrate = sub {
my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
- PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
-
$rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
-
- my $scfg = PVE::Storage::storage_config($storecfg, $storage);
- die "storage '$storage' does not support CT rootdirs\n"
- if !$scfg->{content}->{rootdir};
+ PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
+ PVE::Storage::assert_content_type_supported($storecfg, $storage, 'rootdir', $node);
};
__PACKAGE__->register_method ({
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index f06a10a..61d241a 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -73,12 +73,10 @@ sub prepare {
# check if storage is available on both nodes
my ($storecfg, $node) = $self->@{qw(storecfg node)};
my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
+ PVE::Storage::assert_content_type_supported($storecfg, $storage, 'rootdir');
my $targetsid = $storage;
- die "content type 'rootdir' is not available on storage '$storage'\n"
- if !$scfg->{content}->{rootdir};
-
if ($scfg->{shared} && !$remote) {
# PVE::Storage::activate_storage checks this for non-shared storages
my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
@@ -93,10 +91,11 @@ sub prepare {
}
if (!$remote) {
- my $target_scfg = PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
-
- die "$volid: content type 'rootdir' is not available on storage '$targetsid'\n"
- if !$target_scfg->{content}->{rootdir};
+ PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
+ eval {
+ PVE::Storage::assert_content_type_supported($storecfg, $targetsid, 'rootdir', $node)
+ };
+ die "$volid: $@" if $@;
}
$storages->{$targetsid} = 1;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH container v2 10/11] api: update_vm: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (28 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 09/11] migration: prepare: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 11/11] mount: raw/iso: " Daniel Kral
` (2 subsequent siblings)
32 siblings, 0 replies; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers at
verifying the added and/or changed container config values, which is
called e.g. in the API handler for updating the container config.
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC/Config.pm | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0740e8c..83b6b47 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1176,9 +1176,7 @@ sub update_pct_config {
my $check_content_type = sub {
my ($mp) = @_;
my $sid = PVE::Storage::parse_volume_id($mp->{volume});
- my $storage_config = PVE::Storage::storage_config($storage_cfg, $sid);
- die "storage '$sid' does not allow content type 'rootdir' (Container)\n"
- if !$storage_config->{content}->{rootdir};
+ PVE::Storage::assert_content_type_supported($storage_cfg, $sid, 'rootdir');
};
foreach my $opt (sort keys %$param) { # add/change
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] [PATCH container v2 11/11] mount: raw/iso: use volume content type assertion helpers
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (29 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 10/11] api: update_vm: " Daniel Kral
@ 2025-02-11 16:08 ` Daniel Kral
2025-02-20 13:29 ` Fiona Ebner
2025-02-21 9:50 ` [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Fiona Ebner
2025-02-28 8:46 ` [pve-devel] partially-applied: " Fiona Ebner
32 siblings, 1 reply; 75+ messages in thread
From: Daniel Kral @ 2025-02-11 16:08 UTC (permalink / raw)
To: pve-devel
Make use of the newly introduced content type assertion helpers at
verifying the content type when mounting mountpoints, which have the
format "raw" or "iso".
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- new!
src/PVE/LXC.pm | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 1631f82..9cbe8b2 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1965,17 +1965,17 @@ sub __mountpoint_mount {
}
};
my $use_loopdev = 0;
- if ($scfg->{content}->{rootdir}) {
- if ($scfg->{path}) {
- $mounted_dev = run_with_loopdev($domount, $path, $readonly);
- $use_loopdev = 1;
- } else {
- $mounted_dev = $path;
- &$domount($path);
- }
+
+ PVE::Storage::assert_content_type_supported($storage_cfg, $storage, 'rootdir');
+
+ if ($scfg->{path}) {
+ $mounted_dev = run_with_loopdev($domount, $path, $readonly);
+ $use_loopdev = 1;
} else {
- die "storage '$storage' does not support containers\n";
+ $mounted_dev = $path;
+ &$domount($path);
}
+
return wantarray ? ($path, $use_loopdev, $mounted_dev) : $path;
} else {
die "unsupported image format '$format'\n";
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* Re: [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (30 preceding siblings ...)
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 11/11] mount: raw/iso: " Daniel Kral
@ 2025-02-21 9:50 ` Fiona Ebner
2025-02-28 8:46 ` [pve-devel] partially-applied: " Fiona Ebner
32 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-21 9:50 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> == Possible squash ==
>
> I was unsure how granular the patches here should be, so I left it at
> the finest granularity to make reviewing easier, but there's room for
> squashing patches together:
>
> - qemu-server #10-#14,
> - pve-container #3-#5, and
> - pve-container #7-#11.
Personally, I'm totally fine with having them separate in such a case.
The modifications touch different parts of the code and have no real
interdependency, so it will only be easier to track back a problematic
change when bisecting should the need really arise. Also makes it easier
to keep track of what is affected by each change when looking over
changelog/history.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread* [pve-devel] partially-applied: [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
` (31 preceding siblings ...)
2025-02-21 9:50 ` [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Fiona Ebner
@ 2025-02-28 8:46 ` Fiona Ebner
32 siblings, 0 replies; 75+ messages in thread
From: Fiona Ebner @ 2025-02-28 8:46 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 11.02.25 um 17:07 schrieb Daniel Kral:
> qemu-server:
>
> Daniel Kral (15):
> test: cfg2cmd: expect error for invalid volume's storage content type
> fix #5284: api: move-disk: assert content type support for target storage
> fix #5284: api: clone_vm: assert content type support for target storage
> api: remove unused size variable in check_storage_access
Applied the above 4 patches now, squashing in adding a 'content' key for
the test case. Thanks!
> api: remove unusable default storage parameter in check_storage_access
> fix #5284: api: update-vm: assert content type support for cloudinit images
> fix #5284: cli: importovf: assert content type support for target storage
> tree-wide: update vdisk_alloc optional arguments signature
> cfg2cmd: improve error message for invalid volume content type
> api: {clone,move}_vm: use volume content type assertion helpers
> api: {create,update}_vm: use volume content type assertion helpers
> api: import{disk,ovf}: use volume content type assertion helpers
> api: qmrestore: use volume content type assertion helpers
> api: migrate_vm: use volume content type assertion helpers
> tree-wide: add todos for breaking content type assertions
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 75+ messages in thread