Re: [pve-devel] [PATCH qemu-server 07/11] cfg2cmd: allow PBS snapshots as backing files for drives

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 11.01.21 12:14, Stefan Reiter wrote:
> Uses the custom 'alloc-track' filter node to redirect writes to the
> original drives target, while unwritten blocks will be read from the
> specified PBS snapshot.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/QemuServer.pm | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d517dae..6de6ff6 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3022,7 +3022,8 @@ sub query_understood_cpu_flags {
>  }
>  
>  sub config_to_command {
> -    my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu) = @_;
> +    my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
> +        $pbs_backing) = @_;
>  
>      my $cmd = [];
>      my $globalFlags = [];
> @@ -3521,6 +3522,32 @@ sub config_to_command {
>  	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive);
>  	$drive_cmd .= ',readonly' if PVE::QemuConfig->is_template($conf);
>  
> +	if ($pbs_backing && defined($pbs_backing->{$ds})) {

please avoid extending config_to_command to much, move to own method.

> +	    my $pbs_conf = $pbs_backing->{$ds};
> +	    my $pbs_name = "drive-$ds-pbs";
> +
> +	    # add PBS block device to access snapshot from QEMU
> +	    my $blockdev = "driver=pbs,node-name=$pbs_name,read-only=on";
> +	    $blockdev .= ",repository=$pbs_conf->{repository}";
> +	    $blockdev .= ",snapshot=$pbs_conf->{snapshot}";
> +	    $blockdev .= ",archive=$pbs_conf->{archive}";
> +	    $blockdev .= ",keyfile=$pbs_conf->{keyfile}" if $pbs_conf->{keyfile};
> +	    push @$devices, '-blockdev', $blockdev;
> +
> +	    # modify drive command to put the desired target file behind an
> +	    # 'alloc-track' node
> +	    $drive_cmd =~ s/file=([^,]+)/file.file.filename=$1/;
> +	    $drive_cmd =~ s/,format=([^,]+)/,file.driver=$1,format=alloc-track/;

This seems really fragile and hacky, please avoid string replacements on variables
we assemble ourself.

> +	    $drive_cmd .= ",backing=$pbs_name";
> +	    $drive_cmd .= ",auto-remove=on";
> +
> +	    # note: 'cache' and 'aio' directly affect the 'drive' parameter, so
> +	    # we don't need to change them to 'file.', but 'detect-zeroes' works
> +	    # per blockdev and we want it to persist after the alloc-track is
> +	    # removed, so put it on 'file' directly
> +	    $drive_cmd =~ s/,detect-zeroes=([^,]+)/,file.detect-zeroes=$1/;
> +	}
> +
>  	push @$devices, '-drive',$drive_cmd;
>  	push @$devices, '-device', print_drivedevice_full(
>  	    $storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type);
> @@ -4915,6 +4942,15 @@ sub vm_start {
>  #   timeout => in seconds
>  #   paused => start VM in paused state (backup)
>  #   resume => resume from hibernation
> +#   pbs-backing => {
> +#      sata0 => {
> +#         repository
> +#         snapshot
> +#         keyfile
> +#         archive
> +#      },
> +#      virtio2 => ...
> +#   }
>  # migrate_opts:
>  #   nbd => volumes for NBD exports (vm_migrate_alloc_nbd_disks)
>  #   migratedfrom => source node
> @@ -4961,8 +4997,8 @@ sub vm_start_nolock {
>  	print "Resuming suspended VM\n";
>      }
>  
> -    my ($cmd, $vollist, $spice_port) =
> -	config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
> +    my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
> +	$conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'});
>  
>      my $migration_ip;
>      my $get_migration_ip = sub {
>