[pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Fabian Grünbichler]

as reported on the forum, this is currently missing, making status
queries via the API impossible:

https://forum.proxmox.com/threads/create-vm-via-api-interface.95942/#post-416084

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 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 ef0d877..0a23525 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4336,7 +4336,10 @@ __PACKAGE__->register_method({
 
 	},
     },
-    returns => { type => 'null'},
+    returns => {
+	type => 'string',
+	description => "the task ID.",
+    },
     code => sub {
 	my ($param) = @_;
 
@@ -4375,8 +4378,7 @@ __PACKAGE__->register_method({
 	    return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
 	};
 
-	PVE::QemuConfig->lock_config($vmid, $updatefn);
-	return;
+	return PVE::QemuConfig->lock_config($vmid, $updatefn);
     }});
 
 __PACKAGE__->register_method({
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] api: template: invert lock and fork

[Fabian Grünbichler]

like for other API calls, repeat the cheap checks done for early abort
before forking and without locks after forking and obtaining the lock,
and only hold the flock in the forked worker instead of across the fork.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
not sure whether we want to switch to the "progress bar -> task viewer"
thing in the GUI for this endpoint? it's usually instant, except when
the storage is very slow/overloaded/..

 PVE/API2/Qemu.pm | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 0a23525..a8fbd9d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4353,8 +4353,7 @@ __PACKAGE__->register_method({
 
 	my $disk = extract_param($param, 'disk');
 
-	my $updatefn =  sub {
-
+	my $load_and_check = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 
 	    PVE::QemuConfig->check_lock($conf);
@@ -4368,17 +4367,23 @@ __PACKAGE__->register_method({
 	    die "you can't convert a VM to template if VM is running\n"
 		if PVE::QemuServer::check_running($vmid);
 
-	    my $realcmd = sub {
-		PVE::QemuServer::template_create($vmid, $conf, $disk);
-	    };
+	    return $conf;
+	};
 
-	    $conf->{template} = 1;
-	    PVE::QemuConfig->write_config($vmid, $conf);
+	$load_and_check->();
 
-	    return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
+	my $realcmd = sub {
+	    PVE::QemuConfig->lock_config($vmid, sub {
+		my $conf = $load_and_check->();
+
+		$conf->{template} = 1;
+		PVE::QemuConfig->write_config($vmid, $conf);
+
+		PVE::QemuServer::template_create($vmid, $conf, $disk);
+	    });
 	};
 
-	return PVE::QemuConfig->lock_config($vmid, $updatefn);
+	return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
     }});
 
 __PACKAGE__->register_method({
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Fabian Ebner]

The same issues are present for the corresponding API call in 
pve-container ;)

Both patches
Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

Am 10.09.21 um 09:48 schrieb Fabian Grünbichler:
> as reported on the forum, this is currently missing, making status
> queries via the API impossible:
> 
> https://forum.proxmox.com/threads/create-vm-via-api-interface.95942/#post-416084
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   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 ef0d877..0a23525 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4336,7 +4336,10 @@ __PACKAGE__->register_method({
>   
>   	},
>       },
> -    returns => { type => 'null'},
> +    returns => {
> +	type => 'string',
> +	description => "the task ID.",
> +    },
>       code => sub {
>   	my ($param) = @_;
>   
> @@ -4375,8 +4378,7 @@ __PACKAGE__->register_method({
>   	    return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
>   	};
>   
> -	PVE::QemuConfig->lock_config($vmid, $updatefn);
> -	return;
> +	return PVE::QemuConfig->lock_config($vmid, $updatefn);
>       }});
>   
>   __PACKAGE__->register_method({
> 

Re: [pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Dominik Csapak]

Isn't this technically a breaking api change?

I.e. if someone relies on the '{data:null}' return for a successfull
template task start, it would now break?

Not that i'm opposed to the patch (on the contrary), just wanted
to clarify

Re: [pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Thomas Lamprecht]

On 10.09.21 11:26, Dominik Csapak wrote:
> Isn't this technically a breaking api change?
> 
> I.e. if someone relies on the '{data:null}' return for a successfull
> template task start, it would now break?
> 
> Not that i'm opposed to the patch (on the contrary), just wanted
> to clarify
> 

We never saw the change from null to some data, especially UPID as
breaking change.

If you argue that one asserted for null, which one can do but we simply
do not care, especially as we often did that and nobody yelled about that
practice yet, then we also must not add new parameters because once could
have asserted that their API work by asserting the return of an error code
by using an unknown parameter, i.e., that arguing is a slippery slope to
absolutely freezing development.

Breaking change in response are basically:
- change from non-null type to something else
- removal of properties without guarding such removal behind a new+
  property or similar opt-in way

Re: [pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Fabian Grünbichler]

On September 10, 2021 11:26 am, Dominik Csapak wrote:
> Isn't this technically a breaking api change?
> 
> I.e. if someone relies on the '{data:null}' return for a successfull
> template task start, it would now break?
> 
> Not that i'm opposed to the patch (on the contrary), just wanted
> to clarify

technically yes, in the same way that adding new fields to a return type 
might trip up a very strict client implementation, and we do that all 
the time ;)

the alternative would be to opt-into the new return type via an optional 
parameter, but that just seems ugly (we don't enforce the return schema, 
so we can have an API endpoint return null or the UPID if we want based 
on whatever condition).

IMHO the fallout risk is minimal, the benefit is there, and not 
returning it in the first place was a bug.

Re: [pve-devel] [PATCH qemu-server 1/2] api: return UPID in template call

[Fabian Grünbichler]

On September 10, 2021 10:32 am, Fabian Ebner wrote:
> The same issues are present for the corresponding API call in 
> pve-container ;)

thanks, should have checked there myself. will send a patch for that as 
well.

> Both patches
> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
> 
> Am 10.09.21 um 09:48 schrieb Fabian Grünbichler:
>> as reported on the forum, this is currently missing, making status
>> queries via the API impossible:
>> 
>> https://forum.proxmox.com/threads/create-vm-via-api-interface.95942/#post-416084
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>   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 ef0d877..0a23525 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -4336,7 +4336,10 @@ __PACKAGE__->register_method({
>>   
>>   	},
>>       },
>> -    returns => { type => 'null'},
>> +    returns => {
>> +	type => 'string',
>> +	description => "the task ID.",
>> +    },
>>       code => sub {
>>   	my ($param) = @_;
>>   
>> @@ -4375,8 +4378,7 @@ __PACKAGE__->register_method({
>>   	    return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
>>   	};
>>   
>> -	PVE::QemuConfig->lock_config($vmid, $updatefn);
>> -	return;
>> +	return PVE::QemuConfig->lock_config($vmid, $updatefn);
>>       }});
>>   
>>   __PACKAGE__->register_method({
>> 
> 

[pve-devel] applied: [PATCH qemu-server 1/2] api: return UPID in template call

[Thomas Lamprecht]

On 10.09.21 09:48, Fabian Grünbichler wrote:
> as reported on the forum, this is currently missing, making status
> queries via the API impossible:
> 
> https://forum.proxmox.com/threads/create-vm-via-api-interface.95942/#post-416084
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH qemu-server 2/2] api: template: invert lock and fork

[Thomas Lamprecht]

On 10.09.21 09:48, Fabian Grünbichler wrote:
> like for other API calls, repeat the cheap checks done for early abort
> before forking and without locks after forking and obtaining the lock,
> and only hold the flock in the forked worker instead of across the fork.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> not sure whether we want to switch to the "progress bar -> task viewer"
> thing in the GUI for this endpoint? it's usually instant, except when
> the storage is very slow/overloaded/..
> 
>  PVE/API2/Qemu.pm | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
>

applied, thanks!