Re: [pve-devel] [PATCH v2 qemu-server pve-storage 1/2] migration: secure and use source volume names for cleanup

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

Am 07.12.23 um 10:12 schrieb Hannes Duerr:
> During migration, the volume names may change if the name is already in
> use at the target location. We therefore want to save the original names
> before the migration so that we can clean up the original volumes
> afterwards.
> 

Good catch! I think 'cleanup' is not the best word in the title and
commit message. This is specifically for deactivation. E.g. removal of
local migrated volumes already works as expected.

> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  PVE/QemuMigrate.pm | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index b87e47a..6c9e762 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -632,6 +632,7 @@ sub sync_offline_local_volumes {
>  
>      my $local_volumes = $self->{local_volumes};
>      my @volids = $self->filter_local_volumes('offline', 0);
> +    $self->{source_volumes} = \@volids;
>  

We need to deactivate all volumes on the source node, not only the
offline migrated ones. Note that the comment below specifically mentions
shared LVM LVs.

>      my $storecfg = $self->{storecfg};
>      my $opts = $self->{opts};
> @@ -1584,10 +1585,10 @@ sub phase3_cleanup {
>  	$self->{errors} = 1;
>      }
>  
> +

Nit: extra blank should not be here

>      # always deactivate volumes - avoid lvm LVs to be active on several nodes
>      eval {
> -	my $vollist = PVE::QemuServer::get_vm_volumes($conf);
> -	PVE::Storage::deactivate_volumes($self->{storecfg}, $vollist);
> +	PVE::Storage::deactivate_volumes($self->{storecfg}, $self->{source_volumes});
>      };
>      if (my $err = $@) {
>  	$self->log('err', $err);