* [pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist
@ 2021-07-08 11:25 Stefan Reiter
2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image Stefan Reiter
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Stefan Reiter @ 2021-07-08 11:25 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image 2021-07-08 11:25 [pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Stefan Reiter @ 2021-07-08 11:25 ` Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk Stefan Reiter 2021-07-23 9:06 ` [pve-devel] applied: [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Thomas Lamprecht 2 siblings, 0 replies; 9+ messages in thread From: Stefan Reiter @ 2021-07-08 11:25 UTC (permalink / raw) To: pve-devel ...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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk 2021-07-08 11:25 [pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image Stefan Reiter @ 2021-07-08 11:25 ` Stefan Reiter 2021-07-08 11:46 ` [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up Stefan Reiter 2021-07-23 9:06 ` [pve-devel] applied: [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Thomas Lamprecht 2 siblings, 1 reply; 9+ messages in thread From: Stefan Reiter @ 2021-07-08 11:25 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk Stefan Reiter @ 2021-07-08 11:46 ` Stefan Reiter 2021-07-09 7:11 ` Fabian Ebner 2021-07-23 9:02 ` Thomas Lamprecht 0 siblings, 2 replies; 9+ messages in thread From: Stefan Reiter @ 2021-07-08 11:46 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up 2021-07-08 11:46 ` [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up Stefan Reiter @ 2021-07-09 7:11 ` Fabian Ebner 2021-07-09 13:30 ` Thomas Lamprecht 2021-07-23 9:02 ` Thomas Lamprecht 1 sibling, 1 reply; 9+ messages in thread From: Fabian Ebner @ 2021-07-09 7:11 UTC (permalink / raw) To: pve-devel 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, > ); > } > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up 2021-07-09 7:11 ` Fabian Ebner @ 2021-07-09 13:30 ` Thomas Lamprecht 0 siblings, 0 replies; 9+ messages in thread From: Thomas Lamprecht @ 2021-07-09 13:30 UTC (permalink / raw) To: Proxmox VE development discussion, Fabian Ebner 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up 2021-07-08 11:46 ` [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up Stefan Reiter 2021-07-09 7:11 ` Fabian Ebner @ 2021-07-23 9:02 ` Thomas Lamprecht 2021-08-02 10:15 ` Stefan Reiter 1 sibling, 1 reply; 9+ messages in thread From: Thomas Lamprecht @ 2021-07-23 9:02 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Reiter 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, > ); > } > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up 2021-07-23 9:02 ` Thomas Lamprecht @ 2021-08-02 10:15 ` Stefan Reiter 0 siblings, 0 replies; 9+ messages in thread From: Stefan Reiter @ 2021-08-02 10:15 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion 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, >> ); >> } >> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] applied: [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist 2021-07-08 11:25 [pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk Stefan Reiter @ 2021-07-23 9:06 ` Thomas Lamprecht 2 siblings, 0 replies; 9+ messages in thread From: Thomas Lamprecht @ 2021-07-23 9:06 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Reiter 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-02 10:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-08 11:25 [pve-devel] [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 2/3] cfg2cmd: only warn on non-existant ISO image Stefan Reiter 2021-07-08 11:25 ` [pve-devel] [PATCH qemu-server 3/3] live-restore: ignore missing efidisk Stefan Reiter 2021-07-08 11:46 ` [pve-devel] [PATCH v2 qemu-server 3/3] restore: remove efidisk from config if not backed up Stefan Reiter 2021-07-09 7:11 ` Fabian Ebner 2021-07-09 13:30 ` Thomas Lamprecht 2021-07-23 9:02 ` Thomas Lamprecht 2021-08-02 10:15 ` Stefan Reiter 2021-07-23 9:06 ` [pve-devel] applied: [PATCH qemu-server 1/3] live-restore: fail early if target storage doesn't exist Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox