[pve-devel] [PATCH qemu-server 2/2] map cloudinit disk to new vmid on restore

[Mira Limbeck]

On 5/14/19 9:08 AM, Thomas Lamprecht wrote:
> On 5/13/19 2:01 PM, Mira Limbeck wrote:
>> since the restore is now working but does not map the disk to the new
>> vmid, this patch solves that. it allocates the new disk with vdisk_alloc
>> to get the new disk name.
>> the regex to check if it could be a cloudinit config line is limited to
>> ide|sata|scsi because those are the only ones supported for cdrom
>> drives. the following 'parse_drive' is so we can use
>> 'drive_is_cloudinit' instead of having to use a custom regex for the
>> is_cloudinit check.
>> the format on restore is kept if the underlying storage supports it,
>> otherwise the default for that storage is used.
>>
>> this should fix #1807 completely. the restore error was already resolved
>> with commit 7e8ab2a, but the vmid of the disk might not have matched the new
>> one.
> looks OK in general, some minor comment inline.
>
>> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
>> ---
>>   PVE/QemuServer.pm | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 9d560ec..06329bd 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6264,6 +6264,22 @@ sub restore_vma_archive {
>>   		$storage_limits{$storeid} = $bwlimit;
>>   
>>   		$virtdev_hash->{$virtdev} = $devinfo->{$devname};
>> +	    } elsif ($line =~ m/^((?:ide|sata|scsi)\d+):\s*(.*)\s*$/) {
>> +		my $virtdev = $1;
>> +		my $drive = parse_drive($virtdev, $2);
>> +		if (drive_is_cloudinit($drive)) {
>> +		    my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
>> +		    my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>> +		    my $format = qemu_img_format($scfg, $volname); # has 'raw' fallback
>> +
>> +		    my $d = {};
>> +		    $d->{format} = $format;
>> +		    $d->{storeid} = $opts->{storage} // $storeid;
>> +		    $d->{size} = $PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
> do we need to set it, or wouldn't commit_ci just set it for us once the VM gets
> actually started again?
It has to be set at least, otherwise the size calculation for 
$alloc_size used in vdisk_alloc would fail. And in this case we have to 
set it to the right size already, otherwise we have a wrong disk that 
gets used until it is deleted and recreated again. We need the 
vdisk_alloc for the correct volid.
>
>> +		    $d->{file} = $drive->{file}; # to make drive_is_cloudinit check possible
>> +		    $d->{name} = "vm-$vmid-cloudinit";
> maybe a direct initialization alá
>
> my $d = {
>      format => $format,
>      storeid => $opts->{storage} // $storeid,
>      ...,
>      name => "vm-$vmid-cloudinit",
> };
>
> could look a bit cleaner, but naturally just IMO :-)
Then I'll change it together with the $d->{name} // undef thing.
>
>> +		    $virtdev_hash->{$virtdev} = $d;
>> +		}
>>   	    }
>>   	}
>>   
>> @@ -6334,8 +6350,12 @@ sub restore_vma_archive {
>>   	    my $supported = grep { $_ eq $d->{format} } @$validFormats;
>>   	    $d->{format} = $defFormat if !$supported;
>>   
>> +	    my $name = $d->{name} // undef;
> // is a check for defined-ness, so above is a no-op?
Yep.
>
>> +	    if ($name && $d->{format} ne 'raw') {
>> +		$name .= ".$d->{format}";
>> +	    }
>>   	    my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid,
>> -						  $d->{format}, undef, $alloc_size);
>> +						  $d->{format}, $name, $alloc_size);
>>   	    print STDERR "new volume ID is '$volid'\n";
>>   	    $d->{volid} = $volid;
>>   	    my $path = PVE::Storage::path($cfg, $volid);
>> @@ -6347,9 +6367,12 @@ sub restore_vma_archive {
>>   		$write_zeros = 0;
>>   	    }
>>   
>> -	    print $fifofh "${map_opts}format=$d->{format}:${write_zeros}:$d->{devname}=$path\n";
>> +	    my $is_cloudinit = defined($d->{file}) && drive_is_cloudinit($d);
>> +	    if (!$is_cloudinit) {
>> +		print $fifofh "${map_opts}format=$d->{format}:${write_zeros}:$d->{devname}=$path\n";
>>   
>> -	    print "map '$d->{devname}' to '$path' (write zeros = ${write_zeros})\n";
>> +		print "map '$d->{devname}' to '$path' (write zeros = ${write_zeros})\n";
>> +	    }
>>   	    $map->{$virtdev} = $volid;
>>   	}
>>   
>>
>