[pve-devel] [PATCH qemu-server] fix #2173: use list_images to check for cloudinit disk

[Thomas Lamprecht]

Am 4/29/19 um 10:25 AM schrieb Mira Limbeck:
> After talking to @Dominik he suggested to use 'qemu-img info' to check if the disk exists. Would this be a better solution than the 'map_volume' one for you as well?
> 

Doesn't it just maps the volume too? Not sure of benefits..
Independent of what you choose, I'd still like to get the storage patch
for returning path for the default map_colume implementation.

> On 4/25/19 4:07 PM, Thomas Lamprecht wrote:
>> Am 4/25/19 um 12:10 PM schrieb Mira Limbeck:
>>> On 4/24/19 3:58 PM, Thomas Lamprecht wrote:
>>>> Am 4/15/19 um 1:50 PM schrieb Mira Limbeck:
>>>>> use list_images to check for existence of cloudinit disk instead of
>>>>> '-e'. this should solve the problem with rbd where the path returned by
>>>>> PVE::Storage::path is not checkable with '-e'.
>>>> "should" (guessing) or "does solve" (tested) it??
>>> I tested it of course. Bad wording
>>>> how does the 'qemu-img dd' then writes to it? does it map it itself,
>>>> or connects via rbd? (if so with which config?)
>>> The path contains config, id and keyring, so all required information is passed to 'qemu-img dd'. The string returned by PVE::Storage::path for one of my test-vms is: 'rbd:pool1/vm-102-cloudinit:conf=/etc/pve/ceph.conf:id=admin:keyring=/etc/pve/priv/ceph/pool1.keyring'
>> Thanks for clarifying!
>>
>>>> Why not just doing a map_volume before? then you can keep the rest as
>>>> is and not do a full content listing (albeit some plugins cache it,
>>>> it's not exactly cheap, especially with big storages)...
>>> I'll take a look at map_volume. If I understand it correctly it returns the absolute path (and if necessary maps the volume so one exists (e.g. rbd))?
>> Yes, the single issue is that the default implementation does not
>> defaults to returning the path, but we could change that just fine,
>> and actually Wolfgang and I talked about this a few times and, IIRC,
>> agreed that this change would be nicer ABI wise (and not break old
>> ABI)..
>>
>> As currently our map vol use go like:
>>
>> my $path = PVE:Storage::map_vol(...);
>> $path = PVE:Storage::path(...) if !defined($path);
>>
>>
>>
>>>>> Signed-off-by: Mira Limbeck<m.limbeck at proxmox.com>
>>>>> ---
>>>>>    PVE/QemuServer/Cloudinit.pm | 8 ++++++--
>>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
>>>>> index 445c777..da08470 100644
>>>>> --- a/PVE/QemuServer/Cloudinit.pm
>>>>> +++ b/PVE/QemuServer/Cloudinit.pm
>>>>> @@ -31,13 +31,17 @@ sub commit_cloudinit_disk {
>>>>>        my $iso_path = PVE::Storage::path($storecfg, $drive->{file});
>>>>>        my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>>>>        my $format = PVE::QemuServer::qemu_img_format($scfg, $volname);
>>>>> -    if (! -e $iso_path) {
>>>>> +
>>>>> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>>> +    my $vollist = $plugin->list_images($storeid, $scfg, $vmid);
>>>>> +    my $exists = grep { $_->{volid} eq $drive->{file} } @$vollist;
>>>>> +
>>>>> +    if (!$exists) {
>>>>>        $volname =~ m/(vm-$vmid-cloudinit(.(qcow2|raw))?)/;
>>>>>        my $name = $1;
>>>>>        my $d = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, 4 * 1024);
>>>>>        }
>>>>>    -    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>>>        $plugin->activate_volume($storeid, $scfg, $volname);
>>>>>          my $size = PVE::Storage::file_size_info($iso_path);
>>>>>