[pve-devel] [PATCH qemu-server 2/2] fix #2395: refactor qemu_img_convert to accept files as source
Thomas Lamprecht
t.lamprecht at proxmox.com
Thu Oct 17 09:35:51 CEST 2019
On 10/16/19 11:27 AM, Dominik Csapak wrote:
> and use it also for efidisk creation and importdisk
> this way we correctly handle zfs-over-iscsi options for those cases
>
> also write tests for it
>
one minor nit about a error message below, with that:
Reviewed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> makes more sense when viewed with -w, but i think comparing the
> complete source of qemu_img_convert before and after this patch is easiest
>
> PVE/QemuServer.pm | 112 +++++++++++++++--------------
> PVE/QemuServer/ImportDisk.pm | 11 +--
> test/run_qemu_img_convert_tests.pl | 25 +++++++
> test/test.vmdk | 0
> 4 files changed, 87 insertions(+), 61 deletions(-)
> create mode 100644 test/test.vmdk
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 8dda594..2388eab 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6884,66 +6884,77 @@ sub qemu_img_convert {
> my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid, 1);
> my ($dst_storeid, $dst_volname) = PVE::Storage::parse_volume_id($dst_volid, 1);
>
> - if ($src_storeid && $dst_storeid) {
> + die "invalid destination for qemu-img convert\n" if !$dst_storeid;
>
> - PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname);
> + my $cachemode;
> + my $src_path;
> + my $src_is_iscsi = 0;
> + my $src_format = 'raw';
>
> + if ($src_storeid) {
> + PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname);
> my $src_scfg = PVE::Storage::storage_config($storecfg, $src_storeid);
> - my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
> + $src_format = qemu_img_format($src_scfg, $src_volname);
> + $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
> + $src_is_iscsi = ($src_path =~ m|^iscsi://|);
> + $cachemode = 'none' if $src_scfg->{type} eq 'zfspool';
> + } elsif (-f $src_volid) {
> + $src_path = $src_volid;
> + if ($src_path =~ m/\.($QEMU_FORMAT_RE)$/) {
> + $src_format = $1;
> + }
> + }
>
> - my $src_format = qemu_img_format($src_scfg, $src_volname);
> - my $dst_format = qemu_img_format($dst_scfg, $dst_volname);
> + die "invalid source for qemu-img convert\n" if !$src_path;
I'd output the $src_volid so we/users can know why this triggered.
Something like the following, in nice:
"invalid source for qemu-img convert, '$src_volid' neither valid volume id nor exist as file!"
>
> - my $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
> - my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
> + my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
> + my $dst_format = qemu_img_format($dst_scfg, $dst_volname);
> + my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
> + my $dst_is_iscsi = ($dst_path =~ m|^iscsi://|);
>
> - my $src_is_iscsi = ($src_path =~ m|^iscsi://|);
> - my $dst_is_iscsi = ($dst_path =~ m|^iscsi://|);
> + my $cmd = [];
> + push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> + push @$cmd, '-l', "snapshot.name=$snapname" if($snapname && $src_format eq "qcow2");
> + push @$cmd, '-t', 'none' if $dst_scfg->{type} eq 'zfspool';
> + push @$cmd, '-T', $cachemode if defined($cachemode);
> +
> + if ($src_is_iscsi) {
> + push @$cmd, '--image-opts';
> + $src_path = convert_iscsi_path($src_path);
> + } else {
> + push @$cmd, '-f', $src_format;
> + }
>
> - my $cmd = [];
> - push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> - push @$cmd, '-l', "snapshot.name=$snapname" if($snapname && $src_format eq "qcow2");
> - push @$cmd, '-t', 'none' if $dst_scfg->{type} eq 'zfspool';
> - push @$cmd, '-T', 'none' if $src_scfg->{type} eq 'zfspool';
> + if ($dst_is_iscsi) {
> + push @$cmd, '--target-image-opts';
> + $dst_path = convert_iscsi_path($dst_path);
> + } else {
> + push @$cmd, '-O', $dst_format;
> + }
>
> - if ($src_is_iscsi) {
> - push @$cmd, '--image-opts';
> - $src_path = convert_iscsi_path($src_path);
> - } else {
> - push @$cmd, '-f', $src_format;
> - }
> + push @$cmd, $src_path;
>
> - if ($dst_is_iscsi) {
> - push @$cmd, '--target-image-opts';
> - $dst_path = convert_iscsi_path($dst_path);
> - } else {
> - push @$cmd, '-O', $dst_format;
> - }
> + if (!$dst_is_iscsi && $is_zero_initialized) {
> + push @$cmd, "zeroinit:$dst_path";
> + } else {
> + push @$cmd, $dst_path;
> + }
>
> - push @$cmd, $src_path;
> + my $parser = sub {
> + my $line = shift;
> + if($line =~ m/\((\S+)\/100\%\)/){
> + my $percent = $1;
> + my $transferred = int($size * $percent / 100);
> + my $remaining = $size - $transferred;
>
> - if (!$dst_is_iscsi && $is_zero_initialized) {
> - push @$cmd, "zeroinit:$dst_path";
> - } else {
> - push @$cmd, $dst_path;
> + print "transferred: $transferred bytes remaining: $remaining bytes total: $size bytes progression: $percent %\n";
> }
>
> - my $parser = sub {
> - my $line = shift;
> - if($line =~ m/\((\S+)\/100\%\)/){
> - my $percent = $1;
> - my $transferred = int($size * $percent / 100);
> - my $remaining = $size - $transferred;
> -
> - print "transferred: $transferred bytes remaining: $remaining bytes total: $size bytes progression: $percent %\n";
> - }
> -
> - };
> + };
>
> - eval { run_command($cmd, timeout => undef, outfunc => $parser); };
> - my $err = $@;
> - die "copy failed: $err" if $err;
> - }
> + eval { run_command($cmd, timeout => undef, outfunc => $parser); };
> + my $err = $@;
> + die "copy failed: $err" if $err;
> }
>
> sub qemu_img_format {
> @@ -7269,15 +7280,12 @@ sub create_efidisk($$$$$) {
> my (undef, $ovmf_vars) = get_ovmf_files($arch);
> die "EFI vars default image not found\n" if ! -f $ovmf_vars;
>
> - my $vars_size = PVE::Tools::convert_size(-s $ovmf_vars, 'b' => 'kb');
> + my $vars_size_b = -s $ovmf_vars;
> + my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
> my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
> PVE::Storage::activate_volumes($storecfg, [$volid]);
>
> - my $path = PVE::Storage::path($storecfg, $volid);
> - eval {
> - run_command(['/usr/bin/qemu-img', 'convert', '-n', '-f', 'raw', '-O', $fmt, $ovmf_vars, $path]);
> - };
> - die "Copying EFI vars image failed: $@" if $@;
> + qemu_img_convert($ovmf_vars, $volid, $vars_size_b, undef, 0);
>
> return ($volid, $vars_size);
> }
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index db7db63..5d391e6 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -37,14 +37,7 @@ sub do_import {
> warn "args: $src_path, $vmid, $storage_id, $optional\n",
> "\$dst_volid: $dst_volid\n", if $debug;
>
> - # qemu-img convert does the hard job
> - # we don't attempt to guess filetypes ourselves
> - my $convert_command = ['qemu-img', 'convert', $src_path, '-p', '-n', '-O', $dst_format];
> - if (PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid)) {
> - push @$convert_command, "zeroinit:$dst_path";
> - } else {
> - push @$convert_command, $dst_path;
> - }
> + my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
>
> my $create_drive = sub {
> my $vm_conf = PVE::QemuConfig->load_config($vmid);
> @@ -88,7 +81,7 @@ sub do_import {
> local $SIG{HUP} =
> local $SIG{PIPE} = sub { die "interrupted by signal\n"; };
> PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
> - run_command($convert_command);
> + PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
> PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
> PVE::QemuConfig->lock_config($vmid, $create_drive);
> };
> diff --git a/test/run_qemu_img_convert_tests.pl b/test/run_qemu_img_convert_tests.pl
> index fafb58a..5ad037f 100755
> --- a/test/run_qemu_img_convert_tests.pl
> +++ b/test/run_qemu_img_convert_tests.pl
> @@ -129,6 +129,31 @@ my $tests = {
> "local-lvm:vm-$vmid-disk-0", "not-existing:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 1,
> "storage 'not-existing' does not exists\n",
> ],
> + vmdkfile => [
> + "./test.vmdk", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> + "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "vmdk", "-O", "raw",
> + "./test.vmdk",
> + "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> + ]
> + ],
> + notexistingfile => [
> + "/foo/bar", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0,
> + "invalid source for qemu-img convert\n",
> + ],
> + efidisk => [
> + "/usr/share/kvm/OVMF_VARS-pure-efi.fd", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> + "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> + "/usr/share/kvm/OVMF_VARS-pure-efi.fd",
> + "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> + ]
> + ],
> + efi2zos => [
> + "/usr/share/kvm/OVMF_VARS-pure-efi.fd", "zfs-over-iscsi:vm-$vmid-disk-0", 1024*10, undef, 0, [
> + "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "--target-image-opts",
> + "/usr/share/kvm/OVMF_VARS-pure-efi.fd",
> + "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw",
> + ]
> + ],
> };
>
> my $command;
> diff --git a/test/test.vmdk b/test/test.vmdk
> new file mode 100644
> index 0000000..e69de29
>
More information about the pve-devel
mailing list