[pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage

[Fabian Ebner]

Am 22.03.21 um 11:01 schrieb Fabian Grünbichler:
> On March 22, 2021 9:56 am, Fabian Ebner wrote:
>> Am 19.03.21 um 15:16 schrieb Fabian Grünbichler:
>>> On March 19, 2021 2:49 pm, Fabian Ebner wrote:
>>>> it's cheap and saves code.
>>>
>>> but also changes behaviour in a non-backwards-compatible fashion.
>>>
>>> previously, if a disk was already on a storage that does not have images
>>> configured, and the migration leaves it on that storage, this config
>>> mismatch was ignored (hence the "grandfather in existing mismatches"
>>> comment). note that users might be able to migrate, but not able to
>>> change storage.cfg to fix this "misconfiguration".
>>>
>>
>> What about the recent change [0] in pve-storage then, i.e. not listing
>> VM disks for storages without an appropriate content type?
> 
> I'd have to think more about that (I mean, it obviously makes sense, the
> question is whether it breaks something ;))
> 

For referenced volumes on storages without an 'image' or 'rootfs' 
content type, it does break updating the disk size before migration.

>>
>> I'd argue that unreferenced images that lie on storages without an
>> 'image' content type should not be picked up in the first place. If we
>> don't agree on this, then [0] needs to be reverted...
> 
> but this is not just about unreferenced disks? I mean, this code runs
> for ALL disks returned by pve-storage. if pve-storage now filters, then
> this does not apply for the '"old == new" storage which has wrong content
> type' scenario, since this part of the code thinks there are no volumes
> there, so what I originally thought is broken is not,

It does apply when the content type of the storage is only 'rootfs' for 
example (and we don't currently have a dependency bump on the new 
pve-storage behavior ;)

> 
>> If we do agree on this, we should double down on [0] and actually be
>> precise about the content type in vdisk_list by either:
>>       A) adding a content/guest type parameter to vdisk_list.
>>       B) adapting the call sites to filter storages.
>> Afterwards, this patch could be applied with the appropriate dependency
>> bump ;)
>>
>> Note that referenced unused images will still be picked up by the
>> PVE::QemuServer::foreach_volid iteration, no matter what the content
>> type of their storage is.
> 
> but this would then mean that for referenced images, '"old != new"
> storage where both old and new storage have wrong content type' now
> suddenly works where it previously didn't? not because of this patch,
> but because of vdisk_list not returning those, thus skipping the check
> that is changed in this patch? oh, and shared storages with wrong
> content type also just work, but since we don't really touch volumes
> there anyway with migration that's probably fine..
> 

For shared ones, we skip the scan, and if there is a --targetstorge map, 
there already is a check as part of the API call itself.

> IFF we want to say "wrong content type" is wrong and should make the VM
> unmigratable, we should probably move the check into test_volid so that
> it catches ALL volumes always..
> 

But there might be more subtle things like the disk size issue, so I'd 
be in favor of reverting [0] (for now) and instead adapting the image 
removal on VM destruction to not scan storages without 'images' content.

>>
>> [0]:
>> https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d
>>
>>>>
>>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>>> ---
>>>>    PVE/QemuMigrate.pm | 7 ++-----
>>>>    1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>>>> index 3597cc9..44cecce 100644
>>>> --- a/PVE/QemuMigrate.pm
>>>> +++ b/PVE/QemuMigrate.pm
>>>> @@ -410,11 +410,8 @@ sub sync_disks {
>>>>    	    $log_error->("storage '$targetsid' is not available on node '$self->{node}'")
>>>>    		if !$target_scfg;
>>>>    
>>>> -	    # grandfather in existing mismatches
>>>> -	    if ($targetsid ne $storeid && $target_scfg) {
>>>> -		$log_error->("content type 'images' is not available on storage '$targetsid'")
>>>> -		    if !$target_scfg->{content}->{images};
>>>> -	    }
>>>> +	    $log_error->("content type 'images' is not available on storage '$targetsid'")
>>>> +		if $target_scfg && !$target_scfg->{content}->{images};
>>>>    
>>>>    	    PVE::Storage::foreach_volid($dl, sub {
>>>>    		my ($volid, $sid, $volinfo) = @_;
>>>> -- 
>>>> 2.20.1
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> pve-devel mailing list
>>>> pve-devel at lists.proxmox.com
>>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>