* [pve-devel] [PATCH qemu-server 1/2] migration: alloc nbd disks: base format hint off source storage
2023-07-17 14:00 [pve-devel] [PATCH-SERIES qemu-server] migration: nbd alloc: improve format fallback Fiona Ebner
@ 2023-07-17 14:00 ` Fiona Ebner
2023-07-17 14:00 ` [pve-devel] [PATCH qemu-server 2/2] migration: alloc nbd disks: fix fall-back for remote live migration Fiona Ebner
2023-07-27 8:29 ` [pve-devel] applied: [PATCH-SERIES qemu-server] migration: nbd alloc: improve format fallback Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Fiona Ebner @ 2023-07-17 14:00 UTC (permalink / raw)
To: pve-devel
Previously, qemu_img_format() was called with the target storage's
$scfg and the source storage's volume name.
This mismatch should only be relevant for certain special kinds of
storage plugins:
- no path, but does support an additional QEMU image format besides
'raw', in short NPAF.
- no path, volume name can match QEMU_FORMAT_RE, in short NPVM.
Note that all integrated plugins are neither NPAF nor NPVM.
Note that for NPAF plugins, qemu_img_format() already always returns
'raw' because there is no path. It's a bit unlikely such a plugin
exists, because there were no bug reports about qemu_img_format()
misbehaving there yet.
Let's go through the cases:
- If source and target storage both have or don't have a path,
qemu_img_format($scfg, $volname) returns the same for both $scfg's.
- If source storage has a path, but target storage does not, the
format hint was previously 'raw', but can only be more correct now
(being what the source image actually is):
- For non-NPAF targets, since we know there is no path, it follows
that 'raw' is the only supported QEMU image format.
- For NPAF targets, the format will be preserved now (if actually
supported).
- If source storage does not have a path, but target storage does, the
format hint will be 'raw' now.
- For non-NPVM sources, QEMU_FORMAT_RE didn't match when
qemu_img_format() was called with the target storage's $scfg, so
the hint also was 'raw' before this commit.
- For NPVM sources, qemu_img_format() might've guessed a format from
the source volume name when called with the target's $scfg before
this commit. If the target storage supports the previously guessed
format, it was preserved before this commit, but will not be
anymore. In theory, the guess might've also been wrong, and in
this case, this commit avoids the wrong guess.
To summarize, there is only one edge case with an exotic kind of third
party storage plugin where format preservation would be lost and in
another edge case, format preservation is gained.
In preparation to simplify the format fallback logic implementation.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/QemuServer.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 430661a7..a692e32e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5583,12 +5583,12 @@ sub vm_migrate_alloc_nbd_disks {
# 2. format of current volume
# 3. default format of storage
if (!$storagemap->{identity}) {
+ my $source_scfg = PVE::Storage::storage_config($storecfg, $storeid);
$storeid = PVE::JSONSchema::map_id($storagemap, $storeid);
my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
if (!$format || !grep { $format eq $_ } @$validFormats) {
if ($volname) {
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
- my $fileFormat = qemu_img_format($scfg, $volname);
+ my $fileFormat = qemu_img_format($source_scfg, $volname);
$format = $fileFormat
if grep { $fileFormat eq $_ } @$validFormats;
}
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread* [pve-devel] [PATCH qemu-server 2/2] migration: alloc nbd disks: fix fall-back for remote live migration
2023-07-17 14:00 [pve-devel] [PATCH-SERIES qemu-server] migration: nbd alloc: improve format fallback Fiona Ebner
2023-07-17 14:00 ` [pve-devel] [PATCH qemu-server 1/2] migration: alloc nbd disks: base format hint off source storage Fiona Ebner
@ 2023-07-17 14:00 ` Fiona Ebner
2023-07-27 8:29 ` [pve-devel] applied: [PATCH-SERIES qemu-server] migration: nbd alloc: improve format fallback Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Fiona Ebner @ 2023-07-17 14:00 UTC (permalink / raw)
To: pve-devel
While the comment sated
> # order of precedence, filtered by whether storage supports it:
> # 1. explicit requested format
> # 2. format of current volume
> # 3. default format of storage
the code did not fall back to the default format in the case of remote
migration, because the format was already set and the code used
> $format //= $defFormat;
This made remote migration from dir with qcow2 to e.g. LVM-thin fail.
Move extracting the format from the volume name to the call side for
local migration. This allows the logic here to be much simpler.
Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/API2/Qemu.pm | 1 -
PVE/QemuServer.pm | 32 +++++++++-----------------------
2 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 59307133..9c3f7216 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -5634,7 +5634,6 @@ __PACKAGE__->register_method({
'disk' => [
undef,
$storeid,
- undef,
$drive,
0,
$format,
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a692e32e..4767bad4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5554,9 +5554,11 @@ sub vm_migrate_get_nbd_disks {
my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
return if $scfg->{shared};
+ my $format = qemu_img_format($scfg, $volname);
+
# replicated disks re-use existing state via bitmap
my $use_existing = $replicated_volumes->{$volid} ? 1 : 0;
- $local_volumes->{$ds} = [$volid, $storeid, $volname, $drive, $use_existing];
+ $local_volumes->{$ds} = [$volid, $storeid, $drive, $use_existing, $format];
});
return $local_volumes;
}
@@ -5567,7 +5569,7 @@ sub vm_migrate_alloc_nbd_disks {
my $nbd = {};
foreach my $opt (sort keys %$source_volumes) {
- my ($volid, $storeid, $volname, $drive, $use_existing, $format) = @{$source_volumes->{$opt}};
+ my ($volid, $storeid, $drive, $use_existing, $format) = @{$source_volumes->{$opt}};
if ($use_existing) {
$nbd->{$opt}->{drivestr} = print_drive($drive);
@@ -5576,29 +5578,13 @@ sub vm_migrate_alloc_nbd_disks {
next;
}
- # storage mapping + volname = regular migration
- # storage mapping + format = remote migration
+ $storeid = PVE::JSONSchema::map_id($storagemap, $storeid);
+
# order of precedence, filtered by whether storage supports it:
# 1. explicit requested format
- # 2. format of current volume
- # 3. default format of storage
- if (!$storagemap->{identity}) {
- my $source_scfg = PVE::Storage::storage_config($storecfg, $storeid);
- $storeid = PVE::JSONSchema::map_id($storagemap, $storeid);
- my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
- if (!$format || !grep { $format eq $_ } @$validFormats) {
- if ($volname) {
- my $fileFormat = qemu_img_format($source_scfg, $volname);
- $format = $fileFormat
- if grep { $fileFormat eq $_ } @$validFormats;
- }
- $format //= $defFormat;
- }
- } else {
- # can't happen for remote migration, so $volname is always defined
- my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
- $format = qemu_img_format($scfg, $volname);
- }
+ # 2. default format of storage
+ my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
+ $format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
my $size = $drive->{size} / 1024;
my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, undef, $size);
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread