[pve-devel] [PATCH v5 qemu-server 18/19] cleanup_remotedisks: also include those migrated with storage_migrate

[Fabian Ebner]

On 09.04.20 09:52, Fabian Grünbichler wrote:
>   On April 8, 2020 11:25 am, Fabian Ebner wrote:
>> Call cleanup_remotedisks in phase1_cleanup as well, because that's where
>> we end if sync_disks fails and some disks might already have been
>> transfered successfully.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   PVE/QemuMigrate.pm | 30 ++++++++++++++++++++++--------
>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 0a6277d..68eb083 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -577,16 +577,32 @@ sub sync_disks {
>>   sub cleanup_remotedisks {
>>       my ($self) = @_;
>>   
>> +    my $cleanup_volids = {};
>> +
>> +    foreach my $volid (values %{$self->{volume_map}}) {
>> +	$cleanup_volids->{$volid} = 1;
>> +    }
>> +
>>       foreach my $target_drive (keys %{$self->{target_drive}}) {
>>   	my $drivestr = $self->{target_drive}->{$target_drive}->{drivestr};
>>   	next if !defined($drivestr);
>>   
>>   	my $drive = PVE::QemuServer::parse_drive($target_drive, $drivestr);
>> +	my $volid = $drive->{file};
>> +	next if !defined($volid);
>>   
>> -	# don't clean up replicated disks!
>> -	next if defined($self->{replicated_volumes}->{$drive->{file}});
>> +	if (defined($self->{replicated_volumes}->{$volid})) {
>> +	    # don't clean up replicated disks!
>> +	    delete $cleanup_volids->{$volid};
>> +	} else {
>> +	    # volume_map might not yet contain all of the live-migrating volumes
>> +	    # if there's an error while starting the drive-mirror jobs
>> +	    $cleanup_volids->{$volid} = 1;
> 
> see patch #14 ;)
> 
> it might also be interesting to look at unifying all or some of the
> volume tracking keys in $self:
> 
> $self->{volumes} for local volumes, except replicated ones, except if
> live-migrating and referenced in current config
> $self->{online_local_volumes} source volids that should be NBD migrated
> $self->{replicated_volumes} for replicated volumes
> $self->{volume_map} for mappings of source to target volids
> $self->{target_drive} returned info from target node for NBD migrated volumes
> ... ?
> 
> really shows quite well what the main issue is for QemuMigrate.pm
> 

I can work on this after the release (is probably best) if you want.

>> +	}
>> +    }
>>   
>> -	my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
>> +    foreach my $volid (keys %{$cleanup_volids}) {
>> +	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
>>   
>>   	my $cmd = [@{$self->{rem_ssh}}, 'pvesm', 'free', "$storeid:$volname"];
>>   
>> @@ -638,11 +654,9 @@ sub phase1_cleanup {
>>   	$self->log('err', $err);
>>       }
>>   
>> -    if ($self->{volumes}) {
>> -	foreach my $volid (@{$self->{volumes}}) {
>> -	    $self->log('err', "found stale volume copy '$volid' on node '$self->{node}'");
>> -	    # fixme: try to remove ?
>> -	}
>> +    eval { $self->cleanup_remotedisks() };
>> +    if (my $err = $@) {
>> +	$self->log('err', $err);
>>       }
>>   
>>       eval { $self->cleanup_bitmaps() };
>> -- 
>> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>