[pve-devel] [RFC qemu-server] clone disk: fix handling of snapshot-as-volume-chain for EFI disks

[pve-devel] [RFC qemu-server] clone disk: fix handling of snapshot-as-volume-chain for EFI disks

[Friedrich Weber]

Currently, cloning an EFI disk off a snapshot-as-volume-chain snapshot
fails with "qemu-img: Failed to load snapshot: Can't find snapshot".
The reason is that the special case for EFI disks calls `qemu-img dd`
with the additional `-l snapname` option, which is only valid for
qcow2-internal snapshots. For snapshot-as-volume-chain snapshots, the
source volume is already the volume corresponding to the snapshot.

Fix this by checking whether the snapshot in question is an external
snapshot, and if it is, omitting the `-l` option.

Reported in enterprise support.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    I noticed that two oddities about volume_snapshot_info:
    
    - it doesn't seem to work for qcow2-internal snapshots: It assumes
      that if `qemu img info` returns a JSON object, the snapshots are
      internal, and if it returns a JSON array, they are "external", but
      this doesn't seem correct -- I think because we pass
      --backing-chain, qemu-img will return an one-element JSON array even
      for internal snapshots. Hence, volume_snapshot_info incorrectly
      returns a hash ref with only a single member "current" if the
      snapshots are internal. But since I only found callsites guarded
      with snapshot-as-volume-chain, I guess this didn't hurt until now?
    
      It doesn't hurt for my patch either because it only checks for
      existence of $info->{$snapshot}->{ext} (which also works for
      qcow2-internal snapshots, because alreay $info->{$snapshot} will not
      exist then), but that's admittedly not nice and it's probably a good
      idea to fix volume_snapshot_info in addition.
    
    - but in which way should we fix it? Right now, for volume-chain
      snapshots, it seems like volume_snapshot_info will always have a
      member "current" pointing to the "actual" volume, even if the volume
      doesn't have any snapshots. Is this intended and do we want this
      also in case of qcow2-internal snapshots? From reading
      ZFSPoolPlugin::volume_snapshot_info, it looks like that one will
      return an empty hash (without "current") if the volume has no
      snapshots.

 src/PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cf195ccc..4cd7af52 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7868,7 +7868,9 @@ sub clone_disk {
                 if ($src_format eq 'qcow2' && $snapname) {
                     die "cannot clone qcow2 EFI disk snapshot - requires QEMU >= 6.2\n"
                         if !min_version(kvm_user_version(), 6, 2);
-                    push $cmd->@*, '-l', $snapname;
+                    my $snapshot_info =
+                        PVE::Storage::volume_snapshot_info($storecfg, $drive->{file});
+                    push $cmd->@*, '-l', $snapname if !$snapshot_info->{$snapname}->{ext};
                 }
                 push $cmd->@*, "bs=$bs", "osize=$size", "if=$src_path", "of=$dst_path";
                 run_command($cmd);
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel