[pve-devel] [PATCH qemu-server] migrate: use correct target storage id for checks

[pve-devel] [PATCH qemu-server] migrate: use correct target storage id for checks

[Fabian Ebner]

The '--targetstorage' parameter does not apply to shared storages.

Example for a problem solved with the enabled check: Given a VM with
images only on a shared storage 'storeA', not available on the target
node (i.e. restricted by the nodes property). Then using
'--targetstorage storeB' would make offline migration suddenly
"work", but of course the disks would not be accessible and then
trying to migrate back would fail...

Example for a problem solved with the content type check: if a
VM had a shared ISO image, and there was a '--targetstorage storeA'
option, availablity of the 'iso' content type is checked for
'storeA', which is wrong as the ISO would not be moved to that
storage.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

The first kind of issue is also present in stable-6.

 PVE/QemuMigrate.pm | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 0b41db7..7047d02 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -337,9 +337,15 @@ sub prepare {
 	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
 	# check if storage is available on both nodes
-	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
-
 	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
+
+	my $targetsid;
+	if ($scfg->{shared}) {
+	    $targetsid = $sid;
+	} else {
+	    $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
+	}
+
 	my $target_scfg = PVE::Storage::storage_check_enabled(
 	    $storecfg,
 	    $targetsid,
@@ -472,9 +478,16 @@ sub scan_local_volumes {
 
 	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 
-	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
 	    # check if storage is available on both nodes
 	    my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
+
+	    my $targetsid;
+	    if ($scfg->{shared}) {
+		$targetsid = $sid;
+	    } else {
+		$targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
+	    }
+
 	    PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
 
 	    return if $scfg->{shared};
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server] migrate: use correct target storage id for checks

[Fabian Ebner]

Ping

Am 25.06.21 um 14:32 schrieb Fabian Ebner:
> The '--targetstorage' parameter does not apply to shared storages.
> 
> Example for a problem solved with the enabled check: Given a VM with
> images only on a shared storage 'storeA', not available on the target
> node (i.e. restricted by the nodes property). Then using
> '--targetstorage storeB' would make offline migration suddenly
> "work", but of course the disks would not be accessible and then
> trying to migrate back would fail...
> 
> Example for a problem solved with the content type check: if a
> VM had a shared ISO image, and there was a '--targetstorage storeA'
> option, availablity of the 'iso' content type is checked for
> 'storeA', which is wrong as the ISO would not be moved to that
> storage.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> The first kind of issue is also present in stable-6.
> 
>   PVE/QemuMigrate.pm | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 0b41db7..7047d02 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -337,9 +337,15 @@ sub prepare {
>   	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>   
>   	# check if storage is available on both nodes
> -	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
> -
>   	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
> +
> +	my $targetsid;
> +	if ($scfg->{shared}) {
> +	    $targetsid = $sid;
> +	} else {
> +	    $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
> +	}
> +
>   	my $target_scfg = PVE::Storage::storage_check_enabled(
>   	    $storecfg,
>   	    $targetsid,
> @@ -472,9 +478,16 @@ sub scan_local_volumes {
>   
>   	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>   
> -	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
>   	    # check if storage is available on both nodes
>   	    my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
> +
> +	    my $targetsid;
> +	    if ($scfg->{shared}) {
> +		$targetsid = $sid;
> +	    } else {
> +		$targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
> +	    }
> +
>   	    PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
>   
>   	    return if $scfg->{shared};
> 

[pve-devel] applied: [PATCH qemu-server] migrate: use correct target storage id for checks

[Thomas Lamprecht]

On 25.06.21 14:32, Fabian Ebner wrote:
> The '--targetstorage' parameter does not apply to shared storages.
> 
> Example for a problem solved with the enabled check: Given a VM with
> images only on a shared storage 'storeA', not available on the target
> node (i.e. restricted by the nodes property). Then using
> '--targetstorage storeB' would make offline migration suddenly
> "work", but of course the disks would not be accessible and then
> trying to migrate back would fail...
> 
> Example for a problem solved with the content type check: if a
> VM had a shared ISO image, and there was a '--targetstorage storeA'
> option, availablity of the 'iso' content type is checked for
> 'storeA', which is wrong as the ISO would not be moved to that
> storage.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> The first kind of issue is also present in stable-6.
> 
>  PVE/QemuMigrate.pm | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

But as talked off-list: it makes sense to honor a mapping from a shared source
storage to any target storage too, if explicitly asked for.
But, that'll be easier to do once Fabians remote migration series gets merged,
as there the building blocks for such a "feature" are already present.