[pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist

[pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/QemuServer.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b0fe257..0d8affa 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6381,13 +6381,13 @@ sub restore_proxmox_backup_archive {
 	    my $d = $virtdev_hash->{$virtdev};
 	    next if $d->{is_cloudinit}; # no need to restore cloudinit
 
+	    # this fails if storage is unavailable
+	    my $volid = $d->{volid};
+	    my $path = PVE::Storage::path($storecfg, $volid);
+
 	    # for live-restore we only want to preload the efidisk
 	    next if $options->{live} && $virtdev ne 'efidisk0';
 
-	    my $volid = $d->{volid};
-
-	    my $path = PVE::Storage::path($storecfg, $volid);
-
 	    my $pbs_restore_cmd = [
 		'/usr/bin/pbs-restore',
 		'--repository', $repo,
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image

[Stefan Reiter]

...instead of hard failing. Requires a RPCEnvironment to issue the
warnings, so init a dummy one in run_config2command_tests.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/QemuServer.pm                | 10 ++++++++++
 test/run_config2command_tests.pl |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0d8affa..4082e69 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3195,6 +3195,8 @@ sub config_to_command {
     my $kvm = $conf->{kvm};
     my $nodename = nodename();
 
+    my $rpcenv = PVE::RPCEnvironment::get();
+
     my $arch = get_vm_arch($conf);
     my $kvm_binary = get_command_for_arch($arch);
     my $kvmver = kvm_user_version($kvm_binary);
@@ -3635,6 +3637,14 @@ sub config_to_command {
     PVE::QemuConfig->foreach_volume($conf, sub {
 	my ($ds, $drive) = @_;
 
+	if (drive_is_cdrom($drive)) {
+	    my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});
+	    if ($iso_path ne '' && ! -e $iso_path) {
+		$rpcenv->warn("$ds: image '$iso_path' doesn't exist, ignoring for CDROM");
+		$drive->{file} = 'none';
+	    }
+	}
+
 	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
 	    check_volume_storage_type($storecfg, $drive->{file});
 	    push @$vollist, $drive->{file};
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 04df9d9..a2c3be3 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -404,6 +404,8 @@ sub do_test($) {
 
 print "testing config to command stabillity\n";
 
+PVE::RPCEnvironment->init('cli');
+
 # exec tests
 if (my $file = shift) {
     do_test $file;
-- 
2.30.2

[pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk

[Stefan Reiter]

Don't attempt to restore a configured efidisk for SeaBIOS VMs when it is
missing. This can happen, as configured efidisks do not get backed up if
the BIOS setting is not OVMF.

Note that in case of OVMF VMs a missing efidisk is still treated as an
error, as that indicates a broken backup in general.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/QemuServer.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 4082e69..a86a2a4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6471,6 +6471,13 @@ sub restore_proxmox_backup_archive {
 	my $conf = PVE::QemuConfig->load_config($vmid);
 	die "cannot do live-restore for template\n" if PVE::QemuConfig->is_template($conf);
 
+	if ((!$conf->{bios} || $conf->{bios} ne 'ovmf') && !exists($devinfo->{'drive-efidisk0'})) {
+	    # VMs with SeaBIOS don't include efidisks in the backup, so ignore
+	    # it here for starting
+	    delete $conf->{efidisk0};
+	    $rpcenv->warn("efidisk0 in config, but not backed up - ignoring for live-restore");
+	}
+
 	delete $devinfo->{'drive-efidisk0'}; # this special drive is already restored before start
 	pbs_live_restore($vmid, $conf, $storecfg, $devinfo, $repo, $keyfile, $pbs_backup_name);
 
-- 
2.30.2

[pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up

[Stefan Reiter]

If it doesn't exist, there's no need to keep it around at all. Such a
backup might easily be created if an efidisk is configured with BIOS
set to anything but 'OVMF'.

Unbreaks live-restore for such cases too.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v1->v2:
* Actually remove efidisk from config, not just from launched version for
  live-restore - first, this allows one to restart the VM after the live-restore
  is done without errors, and second this way it also applies for normal
  restores (no reason to keep an efidisk configured that doesn't exist).
  After testing both I think I like this approach better, but feel free to keep
  the v1 version too...

 PVE/QemuServer.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 4082e69..9af0ad3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6044,7 +6044,7 @@ my $restore_allocate_devices = sub {
 };
 
 sub restore_update_config_line {
-    my ($cookie, $map, $line, $unique) = @_;
+    my ($cookie, $map, $line, $unique, $rpcenv) = @_;
 
     return '' if $line =~ m/^\#qmdump\#/;
     return '' if $line =~ m/^\#vzdump\#/;
@@ -6088,6 +6088,11 @@ sub restore_update_config_line {
 	    $di->{file} = $map->{$virtdev};
 	    $value = print_drive($di);
 	    $res .= "$virtdev: $value\n";
+	} elsif ($line =~ m/^efidisk0/) {
+	    # ignore efidisk, this can happen when backing up a SeaBIOS VM with
+	    # an efidisk configured
+	    $rpcenv->warn("efidisk in config, but not backed up - removing from config")
+		if $rpcenv;
 	} else {
 	    $res .= $line;
 	}
@@ -6428,6 +6433,7 @@ sub restore_proxmox_backup_archive {
 		$map,
 		$line,
 		$options->{unique},
+		$rpcenv,
 	    );
 	}
 
-- 
2.30.2

Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up

[Fabian Ebner]

Am 08.07.21 um 13:46 schrieb Stefan Reiter:
> If it doesn't exist, there's no need to keep it around at all. Such a
> backup might easily be created if an efidisk is configured with BIOS
> set to anything but 'OVMF'.
> 
> Unbreaks live-restore for such cases too.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v1->v2:
> * Actually remove efidisk from config, not just from launched version for
>    live-restore - first, this allows one to restart the VM after the live-restore
>    is done without errors, and second this way it also applies for normal
>    restores (no reason to keep an efidisk configured that doesn't exist).
>    After testing both I think I like this approach better, but feel free to keep
>    the v1 version too...
> 
>   PVE/QemuServer.pm | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 4082e69..9af0ad3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6044,7 +6044,7 @@ my $restore_allocate_devices = sub {
>   };
>   
>   sub restore_update_config_line {
> -    my ($cookie, $map, $line, $unique) = @_;
> +    my ($cookie, $map, $line, $unique, $rpcenv) = @_;
>

General question, because this will pop up often with the new task 
warnings: should using RPCEnvironment::get() within each function that 
needs it be preferred over passing it around as a parameter?


>       return '' if $line =~ m/^\#qmdump\#/;
>       return '' if $line =~ m/^\#vzdump\#/;
> @@ -6088,6 +6088,11 @@ sub restore_update_config_line {
>   	    $di->{file} = $map->{$virtdev};
>   	    $value = print_drive($di);
>   	    $res .= "$virtdev: $value\n";
> +	} elsif ($line =~ m/^efidisk0/) {
> +	    # ignore efidisk, this can happen when backing up a SeaBIOS VM with
> +	    # an efidisk configured
> +	    $rpcenv->warn("efidisk in config, but not backed up - removing from config")
> +		if $rpcenv;
>   	} else {
>   	    $res .= $line;
>   	}
> @@ -6428,6 +6433,7 @@ sub restore_proxmox_backup_archive {
>   		$map,
>   		$line,
>   		$options->{unique},
> +		$rpcenv,
>   	    );
>   	}
>   
> 

Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up

[Thomas Lamprecht]

On 09.07.21 09:11, Fabian Ebner wrote:
> Am 08.07.21 um 13:46 schrieb Stefan Reiter:
>> If it doesn't exist, there's no need to keep it around at all. Such a
>> backup might easily be created if an efidisk is configured with BIOS
>> set to anything but 'OVMF'.
>>
>> Unbreaks live-restore for such cases too.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> v1->v2:
>> * Actually remove efidisk from config, not just from launched version for
>>    live-restore - first, this allows one to restart the VM after the live-restore
>>    is done without errors, and second this way it also applies for normal
>>    restores (no reason to keep an efidisk configured that doesn't exist).
>>    After testing both I think I like this approach better, but feel free to keep
>>    the v1 version too...
>>
>>   PVE/QemuServer.pm | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 4082e69..9af0ad3 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6044,7 +6044,7 @@ my $restore_allocate_devices = sub {
>>   };
>>     sub restore_update_config_line {
>> -    my ($cookie, $map, $line, $unique) = @_;
>> +    my ($cookie, $map, $line, $unique, $rpcenv) = @_;
>>
> 
> General question, because this will pop up often with the new task warnings: should using RPCEnvironment::get() within each function that needs it be preferred over passing it around as a parameter?

Good question I asked myself recently.

FWIW, status quo is that we normally only get the RPCEnv in the API and then pass
it along.

I broke that recently once when adding the better warn for the CT start, but
explicitly made it such that it won't fail if a RPCEnv singleton isn't instantiated,
and I was not too happy with it, is was rather done out of time pressure for the
release.

In general I'd rather try to avoid using side-effects in "pure" methods, that
are only helpers or "bussiness-logic", and try to contain RPCEnv to the API/CLI if
possible. If one would need to add a new $rpcenv param to 3+ methods to chain it
through it may be a sign that there's a general design issue with the approach taken
though.

Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up

[Thomas Lamprecht]

On 08.07.21 13:46, Stefan Reiter wrote:
> If it doesn't exist, there's no need to keep it around at all. Such a
> backup might easily be created if an efidisk is configured with BIOS
> set to anything but 'OVMF'.
> 
> Unbreaks live-restore for such cases too.
> 

the issue here is that we start the VM without efi disk if SeaBIOS is used,
so it cannot be live-restored, but couldn't we just restore that separately
so that all data the backup saved gets also restored?

> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v1->v2:
> * Actually remove efidisk from config, not just from launched version for
>   live-restore - first, this allows one to restart the VM after the live-restore
>   is done without errors, and second this way it also applies for normal
>   restores (no reason to keep an efidisk configured that doesn't exist).
>   After testing both I think I like this approach better, but feel free to keep
>   the v1 version too...
> 
>  PVE/QemuServer.pm | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 4082e69..9af0ad3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6044,7 +6044,7 @@ my $restore_allocate_devices = sub {
>  };
>  
>  sub restore_update_config_line {
> -    my ($cookie, $map, $line, $unique) = @_;
> +    my ($cookie, $map, $line, $unique, $rpcenv) = @_;
>  
>      return '' if $line =~ m/^\#qmdump\#/;
>      return '' if $line =~ m/^\#vzdump\#/;
> @@ -6088,6 +6088,11 @@ sub restore_update_config_line {
>  	    $di->{file} = $map->{$virtdev};
>  	    $value = print_drive($di);
>  	    $res .= "$virtdev: $value\n";
> +	} elsif ($line =~ m/^efidisk0/) {
> +	    # ignore efidisk, this can happen when backing up a SeaBIOS VM with
> +	    # an efidisk configured
> +	    $rpcenv->warn("efidisk in config, but not backed up - removing from config")
> +		if $rpcenv;
>  	} else {
>  	    $res .= $line;
>  	}
> @@ -6428,6 +6433,7 @@ sub restore_proxmox_backup_archive {
>  		$map,
>  		$line,
>  		$options->{unique},
> +		$rpcenv,
>  	    );
>  	}
>  
> 

[pve-devel] applied: [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist

[Thomas Lamprecht]

On 08.07.21 13:25, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/QemuServer.pm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up

[Stefan Reiter]

On 7/23/21 11:02 AM, Thomas Lamprecht wrote:
> On 08.07.21 13:46, Stefan Reiter wrote:
>> If it doesn't exist, there's no need to keep it around at all. Such a
>> backup might easily be created if an efidisk is configured with BIOS
>> set to anything but 'OVMF'.
>>
>> Unbreaks live-restore for such cases too.
>>
> 
> the issue here is that we start the VM without efi disk if SeaBIOS is used,
> so it cannot be live-restored, but couldn't we just restore that separately
> so that all data the backup saved gets also restored?
> 

No, other way around, the issue occurs if there exists no efidisk image in the
backup, but only in the config. We already restore efidisks before live-restore,
so SeaBIOS or not doesn't matter, it's at backup time where it makes a problem.

>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> v1->v2:
>> * Actually remove efidisk from config, not just from launched version for
>>    live-restore - first, this allows one to restart the VM after the live-restore
>>    is done without errors, and second this way it also applies for normal
>>    restores (no reason to keep an efidisk configured that doesn't exist).
>>    After testing both I think I like this approach better, but feel free to keep
>>    the v1 version too...
>>
>>   PVE/QemuServer.pm | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 4082e69..9af0ad3 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6044,7 +6044,7 @@ my $restore_allocate_devices = sub {
>>   };
>>   
>>   sub restore_update_config_line {
>> -    my ($cookie, $map, $line, $unique) = @_;
>> +    my ($cookie, $map, $line, $unique, $rpcenv) = @_;
>>   
>>       return '' if $line =~ m/^\#qmdump\#/;
>>       return '' if $line =~ m/^\#vzdump\#/;
>> @@ -6088,6 +6088,11 @@ sub restore_update_config_line {
>>   	    $di->{file} = $map->{$virtdev};
>>   	    $value = print_drive($di);
>>   	    $res .= "$virtdev: $value\n";
>> +	} elsif ($line =~ m/^efidisk0/) {
>> +	    # ignore efidisk, this can happen when backing up a SeaBIOS VM with
>> +	    # an efidisk configured
>> +	    $rpcenv->warn("efidisk in config, but not backed up - removing from config")
>> +		if $rpcenv;
>>   	} else {
>>   	    $res .= $line;
>>   	}
>> @@ -6428,6 +6433,7 @@ sub restore_proxmox_backup_archive {
>>   		$map,
>>   		$line,
>>   		$options->{unique},
>> +		$rpcenv,
>>   	    );
>>   	}
>>   
>>
>