[pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty

[pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty

[Oguz Bektas]

we also have the same skipping logic when starting vms. this way we
avoid calling 'write_config' when there are no present changes to
commit.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/QemuServer.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 45b704d..1379f33 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
 sub vmconfig_apply_pending {
     my ($vmid, $conf, $storecfg, $errors) = @_;
 
+    return if !scalar(keys %{$conf->{pending}});
+
     my $add_apply_error = sub {
 	my ($opt, $msg) = @_;
 	my $err_msg = "unable to apply pending change $opt : $msg";
@@ -5889,7 +5891,7 @@ sub _do_vm_stop {
 		die "VM quit/powerdown failed - got timeout\n";
 	    }
 	} else {
-	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
 	    return;
 	}
     } else {
@@ -5920,7 +5922,7 @@ sub _do_vm_stop {
 	sleep 1;
     }
 
-    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
 }
 
 # Note: use $nocheck to skip tests if VM configuration file exists.
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty

[Fabian Ebner]

Am 01.12.21 um 13:00 schrieb Oguz Bektas:
> we also have the same skipping logic when starting vms. this way we
> avoid calling 'write_config' when there are no present changes to
> commit.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>   PVE/QemuServer.pm | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 45b704d..1379f33 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
>   sub vmconfig_apply_pending {
>       my ($vmid, $conf, $storecfg, $errors) = @_;
>   
> +    return if !scalar(keys %{$conf->{pending}});

Isn't this part of the patch enough? For the caller while starting a VM 
you mentioned, there is an additional load_config() that is skipped if 
there are no pending changes, but for the two callers below, there is no 
such benefit. They just indicate that any pending changes should be 
applied, which is fine even if there are none.

> +
>       my $add_apply_error = sub {
>   	my ($opt, $msg) = @_;
>   	my $err_msg = "unable to apply pending change $opt : $msg";
> @@ -5889,7 +5891,7 @@ sub _do_vm_stop {
>   		die "VM quit/powerdown failed - got timeout\n";
>   	    }
>   	} else {
> -	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> +	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;

Style nit: >100 characters

>   	    return;
>   	}
>       } else {
> @@ -5920,7 +5922,7 @@ sub _do_vm_stop {
>   	sleep 1;
>       }
>   
> -    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> +    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
>   }
>   
>   # Note: use $nocheck to skip tests if VM configuration file exists.
> 

Re: [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty

[Oguz Bektas]

On Wed, Dec 01, 2021 at 03:10:28PM +0100, Fabian Ebner wrote:
> Am 01.12.21 um 13:00 schrieb Oguz Bektas:
> > we also have the same skipping logic when starting vms. this way we
> > avoid calling 'write_config' when there are no present changes to
> > commit.
> > 
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> >   PVE/QemuServer.pm | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index 45b704d..1379f33 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
> >   sub vmconfig_apply_pending {
> >       my ($vmid, $conf, $storecfg, $errors) = @_;
> > +    return if !scalar(keys %{$conf->{pending}});
> 
> Isn't this part of the patch enough? For the caller while starting a VM you
> mentioned, there is an additional load_config() that is skipped if there are
> no pending changes, but for the two callers below, there is no such benefit.
> They just indicate that any pending changes should be applied, which is fine
> even if there are none.

yes i guess it's alright to keep this part only :) though we could still
avoid the vmconfig_apply_pending call in vm_stop_cleanup completely if
the section is empty anyway. not that it brings much but still it
should be fine..

> 
> > +
> >       my $add_apply_error = sub {
> >   	my ($opt, $msg) = @_;
> >   	my $err_msg = "unable to apply pending change $opt : $msg";
> > @@ -5889,7 +5891,7 @@ sub _do_vm_stop {
> >   		die "VM quit/powerdown failed - got timeout\n";
> >   	    }
> >   	} else {
> > -	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> > +	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
> 
> Style nit: >100 characters
> 
> >   	    return;
> >   	}
> >       } else {
> > @@ -5920,7 +5922,7 @@ sub _do_vm_stop {
> >   	sleep 1;
> >       }
> > -    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> > +    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
> >   }
> >   # Note: use $nocheck to skip tests if VM configuration file exists.
> >