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

[Fabian Ebner <f.ebner@proxmox.com>]

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 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...

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.

[0]: 
https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d

>>
>> Signed-off-by: Fabian Ebner <f.ebner@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@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>