all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls
@ 2025-12-05  9:14 Dominik Csapak
  2025-12-05  9:57 ` Fiona Ebner
  2025-12-05 10:04 ` [pve-devel] superseded: " Dominik Csapak
  0 siblings, 2 replies; 4+ messages in thread
From: Dominik Csapak @ 2025-12-05  9:14 UTC (permalink / raw)
  To: pve-devel

It seems, that 'alias' schema does not properly work currently for
normal schema definitions (only for property-string), since it does not
verification of the actual schema (verified by calling the api with
maxworkers=1000, which throws no errors). Luckily, this is the only use
as api parameters that I could find, and here it did not do anything,
since we did not actually use `$param->{maxworkers}` anymore.

In the long term, we should fix it in JSONSchema, but as a stop-gap,
restore the old 'maxworkers' parameters to a 'real' property and mark it
deprecated.

Also actually use it in the code and fix the error message to the new
variant.

Reported in the forums for the node-specific one (since there the gui
uses still the old variant):
https://forum.proxmox.com/threads/bulk-migrate-error-in-9-1-2.177326/

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Cluster/BulkAction/Guest.pm | 33 ++++++++++++++++++++++++----
 PVE/API2/Nodes.pm                    | 14 ++++++++----
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Cluster/BulkAction/Guest.pm b/PVE/API2/Cluster/BulkAction/Guest.pm
index 3a53ca60..9ed7dbb6 100644
--- a/PVE/API2/Cluster/BulkAction/Guest.pm
+++ b/PVE/API2/Cluster/BulkAction/Guest.pm
@@ -251,6 +251,7 @@ sub get_max_workers {
     my ($param, $default) = @_;
 
     return $param->{'max-workers'} if $param->{'max-workers'};
+    return $param->{maxworkers} if $param->{maxworkers}; # alias
 
     my $datacenter_config = PVE::Cluster::cfs_read_file('datacenter.cfg');
     return $datacenter_config->{max_workers} if $datacenter_config->{max_workers};
@@ -286,7 +287,13 @@ __PACKAGE__->register_method({
                 optional => 1,
             },
             maxworkers => {
-                alias => 'max-workers',
+                description => "Defines the maximum number of tasks running concurrently."
+                    . " Deprecated, use 'max-workers' instead.",
+                optional => 1,
+                default => 4,
+                type => 'integer',
+                minimum => 1,
+                maximum => 64,
             },
             'max-workers' => {
                 description => "Defines the maximum number of tasks running concurrently.",
@@ -422,7 +429,13 @@ __PACKAGE__->register_method({
                 optional => 1,
             },
             maxworkers => {
-                alias => 'max-workers',
+                description => "Defines the maximum number of tasks running concurrently."
+                    . " Deprecated, use 'max-workers' instead.",
+                optional => 1,
+                default => 4,
+                type => 'integer',
+                minimum => 1,
+                maximum => 64,
             },
             'max-workers' => {
                 description => "Defines the maximum number of tasks running concurrently.",
@@ -567,7 +580,13 @@ __PACKAGE__->register_method({
                 optional => 1,
             },
             maxworkers => {
-                alias => 'max-workers',
+                description => "Defines the maximum number of tasks running concurrently."
+                    . " Deprecated, use 'max-workers' instead.",
+                optional => 1,
+                default => 4,
+                type => 'integer',
+                minimum => 1,
+                maximum => 64,
             },
             'max-workers' => {
                 description => "Defines the maximum number of tasks running concurrently.",
@@ -704,7 +723,13 @@ __PACKAGE__->register_method({
                 optional => 1,
             },
             maxworkers => {
-                alias => 'max-workers',
+                description => "Defines the maximum number of tasks running concurrently."
+                    . " Deprecated, use 'max-workers' instead.",
+                optional => 1,
+                default => 4,
+                type => 'integer',
+                minimum => 1,
+                maximum => 64,
             },
             'max-workers' => {
                 description => "Defines the maximum number of tasks running concurrently.",
diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 3ca1c319..6a6465b6 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -2559,7 +2559,13 @@ __PACKAGE__->register_method({
             node => get_standard_option('pve-node'),
             target => get_standard_option('pve-node', { description => "Target node." }),
             maxworkers => {
-                alias => 'max-workers',
+                description => "Maximal number of parallel migration job. If not set, uses"
+                    . "'max_workers' from datacenter.cfg. One of both must be set!"
+                    . "Deprecated, use 'max-workers' instead.",
+                optional => 1,
+                type => 'integer',
+                minimum => 1,
+                maximum => 64,
             },
             'max-workers' => {
                 description => "Maximal number of parallel migration job. If not set, uses"
@@ -2613,10 +2619,10 @@ __PACKAGE__->register_method({
 
         my $datacenterconfig = cfs_read_file('datacenter.cfg');
         # prefer parameter over datacenter cfg settings
-        my $max_workers =
-            $param->{'max-workers'}
+        my $max_workers = $param->{'max-workers'}
+            || $param->{'maxworkers'} # alias
             || $datacenterconfig->{max_workers}
-            || die "either 'maxworkers' parameter or max_workers in datacenter.cfg must be set!\n";
+            || die "either 'max-workers' parameter or max_workers in datacenter.cfg must be set!\n";
 
         my $code = sub {
             $rpcenv->{type} = 'priv'; # to start tasks in background
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls
  2025-12-05  9:14 [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls Dominik Csapak
@ 2025-12-05  9:57 ` Fiona Ebner
  2025-12-05  9:59   ` Dominik Csapak
  2025-12-05 10:04 ` [pve-devel] superseded: " Dominik Csapak
  1 sibling, 1 reply; 4+ messages in thread
From: Fiona Ebner @ 2025-12-05  9:57 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

Lot's of "work" in the commit title :P

Am 05.12.25 um 10:15 AM schrieb Dominik Csapak:
> It seems, that 'alias' schema does not properly work currently for
> normal schema definitions (only for property-string), since it does not

Missing "do" here

> verification of the actual schema (verified by calling the api with

Nit: s/verfified/checked/ sounds a bit nicer to avoid re-using "verify" ;)

> maxworkers=1000, which throws no errors). Luckily, this is the only use
> as api parameters that I could find, and here it did not do anything,

Nit: s/only use as/only use of 'alias' for/

Well, the "not do anything" is rather bad for the node-specific one,
which existed for longer, where it breaks existing API users.

> since we did not actually use `$param->{maxworkers}` anymore.
>
> In the long term, we should fix it in JSONSchema, but as a stop-gap,
> restore the old 'maxworkers' parameters to a 'real' property and mark it
> deprecated.
> 
> Also actually use it in the code and fix the error message to the new
> variant.
> 
> Reported in the forums for the node-specific one (since there the gui
> uses still the old variant):
> https://forum.proxmox.com/threads/bulk-migrate-error-in-9-1-2.177326/
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

With the issue mentioned below fixed:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> diff --git a/PVE/API2/Cluster/BulkAction/Guest.pm b/PVE/API2/Cluster/BulkAction/Guest.pm
> index 3a53ca60..9ed7dbb6 100644
> --- a/PVE/API2/Cluster/BulkAction/Guest.pm
> +++ b/PVE/API2/Cluster/BulkAction/Guest.pm
> @@ -704,7 +723,13 @@ __PACKAGE__->register_method({
>                  optional => 1,
>              },
>              maxworkers => {
> -                alias => 'max-workers',
> +                description => "Defines the maximum number of tasks running concurrently."
> +                    . " Deprecated, use 'max-workers' instead.",
> +                optional => 1,
> +                default => 4,
> +                type => 'integer',
> +                minimum => 1,
> +                maximum => 64,
>              },
>              'max-workers' => {
>                  description => "Defines the maximum number of tasks running concurrently.",

For the migrate one here, the default for 'max-workers' is 1, not 4


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls
  2025-12-05  9:57 ` Fiona Ebner
@ 2025-12-05  9:59   ` Dominik Csapak
  0 siblings, 0 replies; 4+ messages in thread
From: Dominik Csapak @ 2025-12-05  9:59 UTC (permalink / raw)
  To: Fiona Ebner, Proxmox VE development discussion



On 12/5/25 10:56 AM, Fiona Ebner wrote:
> Lot's of "work" in the commit title :P
> 
> Am 05.12.25 um 10:15 AM schrieb Dominik Csapak:
>> It seems, that 'alias' schema does not properly work currently for
>> normal schema definitions (only for property-string), since it does not
> 
> Missing "do" here
> 
>> verification of the actual schema (verified by calling the api with
> 
> Nit: s/verfified/checked/ sounds a bit nicer to avoid re-using "verify" ;)
> 
>> maxworkers=1000, which throws no errors). Luckily, this is the only use
>> as api parameters that I could find, and here it did not do anything,
> 
> Nit: s/only use as/only use of 'alias' for/
> 
> Well, the "not do anything" is rather bad for the node-specific one,
> which existed for longer, where it breaks existing API users.
> 
>> since we did not actually use `$param->{maxworkers}` anymore.
>>
>> In the long term, we should fix it in JSONSchema, but as a stop-gap,
>> restore the old 'maxworkers' parameters to a 'real' property and mark it
>> deprecated.
>>
>> Also actually use it in the code and fix the error message to the new
>> variant.
>>
>> Reported in the forums for the node-specific one (since there the gui
>> uses still the old variant):
>> https://forum.proxmox.com/threads/bulk-migrate-error-in-9-1-2.177326/
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> 
> With the issue mentioned below fixed:
> 
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> 
>> diff --git a/PVE/API2/Cluster/BulkAction/Guest.pm b/PVE/API2/Cluster/BulkAction/Guest.pm
>> index 3a53ca60..9ed7dbb6 100644
>> --- a/PVE/API2/Cluster/BulkAction/Guest.pm
>> +++ b/PVE/API2/Cluster/BulkAction/Guest.pm
>> @@ -704,7 +723,13 @@ __PACKAGE__->register_method({
>>                   optional => 1,
>>               },
>>               maxworkers => {
>> -                alias => 'max-workers',
>> +                description => "Defines the maximum number of tasks running concurrently."
>> +                    . " Deprecated, use 'max-workers' instead.",
>> +                optional => 1,
>> +                default => 4,
>> +                type => 'integer',
>> +                minimum => 1,
>> +                maximum => 64,
>>               },
>>               'max-workers' => {
>>                   description => "Defines the maximum number of tasks running concurrently.",
> 
> For the migrate one here, the default for 'max-workers' is 1, not 4


oops yeah, just copy/pasted from the wrong place v2 incoming ;)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [pve-devel] superseded: [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls
  2025-12-05  9:14 [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls Dominik Csapak
  2025-12-05  9:57 ` Fiona Ebner
@ 2025-12-05 10:04 ` Dominik Csapak
  1 sibling, 0 replies; 4+ messages in thread
From: Dominik Csapak @ 2025-12-05 10:04 UTC (permalink / raw)
  To: pve-devel

superseded by v2:
https://lore.proxmox.com/pve-devel/20251205100317.1101549-1-d.csapak@proxmox.com/T/#u


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-12-05 10:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-05  9:14 [pve-devel] [PATCH manager] api: workaround non-working 'maxworkers' alias in bulk actions api calls Dominik Csapak
2025-12-05  9:57 ` Fiona Ebner
2025-12-05  9:59   ` Dominik Csapak
2025-12-05 10:04 ` [pve-devel] superseded: " Dominik Csapak

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal