This avoids having the handling for 'discard-no-unref' in two places.
In the tests, rename the relevant target images with a '-target'
suffix to test for them in the mocked volume_snapshot_info() helper.
Suggested-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
---
Changes since last iteration:
* rebase + make tidy
src/PVE/QemuServer/QemuImage.pm | 65 +++++++++++++++++++++++---
src/test/run_qemu_img_convert_tests.pl | 24 +++++++---
2 files changed, 76 insertions(+), 13 deletions(-)
diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index 8fe75e92..f2cadd69 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -5,11 +5,13 @@ use warnings;
use Fcntl qw(S_ISBLK);
use File::stat;
+use JSON;
use PVE::Format qw(render_bytes);
use PVE::Storage;
use PVE::Tools;
+use PVE::QemuServer::Blockdev;
use PVE::QemuServer::Drive qw(checked_volume_format);
use PVE::QemuServer::Helpers;
@@ -31,16 +33,62 @@ sub convert_iscsi_path {
}
my sub qcow2_target_image_opts {
- my ($path, @qcow2_opts) = @_;
+ my ($storecfg, $drive, @qcow2_opts) = @_;
- # FIXME this duplicates logic from qemu_blockdev_options
- my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
+ # There is no machine version, the qemu-img binary version is what's
important.
+ my $version = PVE::QemuServer::Helpers::kvm_user_version();
- my $driver = S_ISBLK($st->mode) ? 'host_device' : 'file';
+ my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
+ $storecfg,
+ $drive,
+ $version,
+ { 'no-throttle' => 1 },
+ );
- my $qcow2_opts_str = ',' . join(',', @qcow2_opts);
+ my $opts = [];
+ my $opt_prefix = '';
+ my $next_child = $blockdev;
+ while ($next_child) {
+ my $current = $next_child;
+ $next_child = delete($current->{file});
- return
"driver=qcow2$qcow2_opts_str,file.driver=$driver,file.filename=$path";
+ # TODO should cache settings be configured here (via appropriate drive
configuration) rather
+ # than via dedicated qemu-img options?
+ delete($current->{cache});
+ # TODO e.g. can't use aio 'native' without cache.direct, just use QEMU
default like for
+ # other targets for now
+ delete($current->{aio});
+
+ # no need for node names
+ delete($current->{'node-name'});
+
+ # it's the write target, while the flag should be 'false' anyways,
remove to be sure
+ delete($current->{'read-only'});
+
+ # TODO should those be set (via appropriate drive configuration)?
+ delete($current->{'detect-zeroes'});
+ delete($current->{'discard'});
+
+ for my $key (sort keys $current->%*) {
+ my $value;
+ if (ref($current->{$key})) {
+ if ($current->{$key} eq JSON::false) {
+ $value = 'false';
+ } elsif ($current->{$key} eq JSON::true) {
+ $value = 'true';
+ } else {
+ die "target image options: unhandled structured key:
$key\n";
+ }
+ } else {
+ $value = $current->{$key};
+ }
+ push $opts->@*, "$opt_prefix$key=$value";
+ }
+
+ $opt_prefix .= 'file.';
+ }
+
+ return join(',', $opts->@*);
}
# The possible options are:
@@ -115,7 +163,10 @@ sub convert {
if ($dst_is_iscsi) {
$dst_path = convert_iscsi_path($dst_path);
} elsif ($dst_needs_discard_no_unref) {
- $dst_path = qcow2_target_image_opts($dst_path,
'discard-no-unref=true');
+ # don't use any other drive options, those are intended for use with a
running VM and just
+ # use scsi0 as a dummy interface+index for now
+ my $dst_drive = { file => $dst_volid, interface => 'scsi', index => 0
};
+ $dst_path = qcow2_target_image_opts($storecfg, $dst_drive,
'discard-no-unref=true');
} else {
push @$cmd, '-O', $dst_format;
}
diff --git a/src/test/run_qemu_img_convert_tests.pl
b/src/test/run_qemu_img_convert_tests.pl
index 3c8f09f0..393fc4a8 100755
--- a/src/test/run_qemu_img_convert_tests.pl
+++ b/src/test/run_qemu_img_convert_tests.pl
@@ -532,7 +532,7 @@ my $tests = [
name => "qcow2_external_snapshot_target",
parameters => [
"local:$vmid/vm-$vmid-disk-0.raw",
- "localsnapext:$vmid/vm-$vmid-disk-0.qcow2",
+ "localsnapext:$vmid/vm-$vmid-disk-target.qcow2",
1024 * 10,
],
expected => [
@@ -544,14 +544,15 @@ my $tests = [
"raw",
"--target-image-opts",
"/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
- "driver=qcow2,discard-no-unref=true,file.driver=file,"
- .
"file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-0.qcow2",
+ "discard-no-unref=true,driver=qcow2,file.driver=file"
+ .
",file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-target.qcow2",
],
},
{
name => "lvmqcow2_external_snapshot_target",
parameters => [
- "local:$vmid/vm-$vmid-disk-0.raw",
"lvm-store:vm-$vmid-disk-0.qcow2", 1024 * 10,
+ "local:$vmid/vm-$vmid-disk-0.raw",
"lvm-store:vm-$vmid-disk-target.qcow2",
+ 1024 * 10,
],
expected => [
"/usr/bin/qemu-img",
@@ -562,8 +563,8 @@ my $tests = [
"raw",
"--target-image-opts",
"/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
- "driver=qcow2,discard-no-unref=true,file.driver=host_device,"
- . "file.filename=/dev/pve/vm-$vmid-disk-0.qcow2",
+ "discard-no-unref=true,driver=qcow2,file.driver=host_device"
+ . ",file.filename=/dev/pve/vm-$vmid-disk-target.qcow2",
],
},
];
@@ -588,6 +589,17 @@ $storage_module->mock(
activate_volumes => sub {
return 1;
},
+ volume_snapshot_info => sub {
+ my ($cfg, $volid) = @_;
+ if (
+ $volid eq "lvm-store:vm-$vmid-disk-target.qcow2"
+ || $volid eq "localsnapext:$vmid/vm-$vmid-disk-target.qcow2"
+ ) {
+ # target volumes don't have snapshots
+ return {};
+ }
+ die "mocked volume_snapshot_info called with unexpected volid
$volid\n";
+ },
);
my $lio_module = Test::MockModule->new("PVE::Storage::LunCmd::LIO");
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
