[pve-devel] [PATCH qemu-server/storage v4 0/2] fix #1611: implement import of base-images for LVM-thin Storage

[pve-devel] [PATCH qemu-server/storage v4 0/2] fix #1611: implement import of base-images for LVM-thin Storage

[Hannes Duerr]

if a base-image is to be migrated to a lvm-thin storage, a new
vm-image is allocated on the target side, then the data is written
and afterwards the image is converted to a base-image


Changes in V2:
* restructure and remove duplicaiton
* fix deactivation of volumes after migration

Changes in V3:
* fix Nits
* remove unnecessary oldname override
* deactivate not only offline volumes, but all

Changes in V4:
* remove debug stuff
* remove unnecessary key in $self

qemu-server:

Hannes Duerr (1):
  migration: secure and use source volume names for deactivation

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


pve-storage:

Hannes Duerr (1):
  fix #1611: implement import of base-images for LVM-thin Storage

 src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)


Summary over all repositories:
  2 files changed, 53 insertions(+), 0 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH qemu-server v4 1/2] migration: secure and use source volume names for deactivation

[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
so that we can deactivate the original volumes afterwards.

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..8d9b35a 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1455,6 +1455,8 @@ sub phase3_cleanup {
 
     my $tunnel = $self->{tunnel};
 
+    my $sourcevollist = PVE::QemuServer::get_vm_volumes($conf);
+
     if ($self->{volume_map} && !$self->{opts}->{remote}) {
 	my $target_drives = $self->{target_drive};
 
@@ -1586,8 +1588,7 @@ sub phase3_cleanup {
 
     # 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}, $sourcevollist);
     };
     if (my $err = $@) {
 	$self->log('err', $err);
-- 
2.39.2

[pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Hannes Duerr]

for base images we call the volume_import of the parent plugin and pass
it as vm-image instead of base-image, then convert it back as base-image

Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
 src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1d2e37c..96f619b 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -383,6 +383,56 @@ sub volume_has_feature {
     return undef;
 }
 
+sub volume_import {
+    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
+
+    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
+	$class->parse_volname($volname);
+
+    if (!$isBase) {
+	return $class->SUPER::volume_import(
+	    $scfg,
+	    $storeid,
+	    $fh,
+	    $volname,
+	    $format,
+	    $snapshot,
+	    $base_snapshot,
+	    $with_snapshots,
+	    $allow_rename
+	);
+    } else {
+	my $tempname;
+	my $vg = $scfg->{vgname};
+	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+	if ($lvs->{$vg}->{$volname}) {
+	    die "volume $vg/$volname already exists\n" if !$allow_rename;
+	    warn "volume $vg/$volname already exists - importing with a different name\n";
+
+	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
+	} else {
+	    $tempname = $volname;
+	    $tempname =~ s/base/vm/;
+	}
+
+	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
+	    $scfg,
+	    $storeid,
+	    $fh,
+	    $tempname,
+	    $format,
+	    $snapshot,
+	    $base_snapshot,
+	    $with_snapshots,
+	    $allow_rename
+	));
+
+	$volname = $class->create_base($storeid, $scfg, $newname);
+    }
+
+    return "$storeid:$volname";
+}
+
 # used in LVMPlugin->volume_import
 sub volume_import_write {
     my ($class, $input_fh, $output_file) = @_;
-- 
2.39.2

Re: [pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Lukas Wagner]

On 12/19/23 15:03, Hannes Duerr wrote:
> for base images we call the volume_import of the parent plugin and pass
> it as vm-image instead of base-image, then convert it back as base-image
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>   src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..96f619b 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,56 @@ sub volume_has_feature {
>       return undef;
>   }
>   
> +sub volume_import {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
                                                                 ^
Nit: camelCase :)
> +	$class->parse_volname($volname);
> +
> +    if (!$isBase) {
> +	return $class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $volname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	);
> +    } else {
> +	my $tempname;
> +	my $vg = $scfg->{vgname};
> +	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> +	if ($lvs->{$vg}->{$volname}) {
> +	    die "volume $vg/$volname already exists\n" if !$allow_rename;
> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
> +
> +	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
> +	} else {
> +	    $tempname = $volname;
> +	    $tempname =~ s/base/vm/;
> +	}
> +
> +	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $tempname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +
> +	$volname = $class->create_base($storeid, $scfg, $newname);
> +    }
> +
> +    return "$storeid:$volname";
> +}
> +
>   # used in LVMPlugin->volume_import
>   sub volume_import_write {
>       my ($class, $input_fh, $output_file) = @_;

-- 
- Lukas

[pve-devel] applied: [PATCH qemu-server v4 1/2] migration: secure and use source volume names for deactivation

[Fabian Grünbichler]

thanks!

On December 19, 2023 3:03 pm, Hannes Duerr wrote:
> 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
> so that we can deactivate the original volumes afterwards.
> 
> 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..8d9b35a 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -1455,6 +1455,8 @@ sub phase3_cleanup {
>  
>      my $tunnel = $self->{tunnel};
>  
> +    my $sourcevollist = PVE::QemuServer::get_vm_volumes($conf);
> +
>      if ($self->{volume_map} && !$self->{opts}->{remote}) {
>  	my $target_drives = $self->{target_drive};
>  
> @@ -1586,8 +1588,7 @@ sub phase3_cleanup {
>  
>      # 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}, $sourcevollist);
>      };
>      if (my $err = $@) {
>  	$self->log('err', $err);
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Fabian Grünbichler]

On December 19, 2023 3:03 pm, Hannes Duerr wrote:
> for base images we call the volume_import of the parent plugin and pass
> it as vm-image instead of base-image, then convert it back as base-image
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..96f619b 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,56 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub volume_import {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> +	$class->parse_volname($volname);
> +
> +    if (!$isBase) {
> +	return $class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $volname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	);
> +    } else {
> +	my $tempname;
> +	my $vg = $scfg->{vgname};
> +	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> +	if ($lvs->{$vg}->{$volname}) {
> +	    die "volume $vg/$volname already exists\n" if !$allow_rename;
> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
> +
> +	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
> +	} else {
> +	    $tempname = $volname;
> +	    $tempname =~ s/base/vm/;

couldn't we simply do this part unconditionally, and then call the SUPER
method? that one already checks whether $volname exists, does the same
die/warn and then unsets $volname in case of a collision, so that
alloc_image gets to pick a free one? then we just need to create_base on
the returned $volid and be done, making this a very thin wrapper..

but, I just realized a bigger issue with all of this code here - AFAICT
nothing ensures the storage is actually locked, so two parallel
imports (or an import racing with an alloc/..) could actually end up
trying to use the same volname. but we don't want to hold the lock for
the full duration of the import, just for the part covering the "find
free slot -> alloc_image" time span. this is not limited to LVM, but
properly handling it would require splitting up volume_import into two
plugin methods (breaking the storage plugin ABI) which might not even
work for all storage types.

> +	}
> +
> +	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $tempname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +
> +	$volname = $class->create_base($storeid, $scfg, $newname);
> +    }
> +
> +    return "$storeid:$volname";
> +}
> +
>  # used in LVMPlugin->volume_import
>  sub volume_import_write {
>      my ($class, $input_fh, $output_file) = @_;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Hannes Dürr]

On 1/30/24 11:00, Fabian Grünbichler wrote:
> On December 19, 2023 3:03 pm, Hannes Duerr wrote:
>> for base images we call the volume_import of the parent plugin and pass
>> it as vm-image instead of base-image, then convert it back as base-image
>>
>> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
>> ---
>>   src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
>> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
>> index 1d2e37c..96f619b 100644
>> --- a/src/PVE/Storage/LvmThinPlugin.pm
>> +++ b/src/PVE/Storage/LvmThinPlugin.pm
>> @@ -383,6 +383,56 @@ sub volume_has_feature {
>>       return undef;
>>   }
>>   
>> +sub volume_import {
>> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
>> +
>> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
>> +	$class->parse_volname($volname);
>> +
>> +    if (!$isBase) {
>> +	return $class->SUPER::volume_import(
>> +	    $scfg,
>> +	    $storeid,
>> +	    $fh,
>> +	    $volname,
>> +	    $format,
>> +	    $snapshot,
>> +	    $base_snapshot,
>> +	    $with_snapshots,
>> +	    $allow_rename
>> +	);
>> +    } else {
>> +	my $tempname;
>> +	my $vg = $scfg->{vgname};
>> +	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
>> +	if ($lvs->{$vg}->{$volname}) {
>> +	    die "volume $vg/$volname already exists\n" if !$allow_rename;
>> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
>> +
>> +	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
>> +	} else {
>> +	    $tempname = $volname;
>> +	    $tempname =~ s/base/vm/;
> couldn't we simply do this part unconditionally, and then call the SUPER
> method? that one already checks whether $volname exists, does the same
> die/warn and then unsets $volname in case of a collision, so that
> alloc_image gets to pick a free one? then we just need to create_base on
> the returned $volid and be done, making this a very thin wrapper..

If we simply rename base-image-x to vm-image-x and pass it on to the parent

image, we may not be able to create a base image of the imported volume

vm-image-x, as there may already be a base-image-x, which we have not 
checked

before.


The patch still applies on master

> but, I just realized a bigger issue with all of this code here - AFAICT
> nothing ensures the storage is actually locked, so two parallel
> imports (or an import racing with an alloc/..) could actually end up
> trying to use the same volname. but we don't want to hold the lock for
> the full duration of the import, just for the part covering the "find
> free slot -> alloc_image" time span. this is not limited to LVM, but
> properly handling it would require splitting up volume_import into two
> plugin methods (breaking the storage plugin ABI) which might not even
> work for all storage types.
>
Created a patch for this issue:
https://lists.proxmox.com/pipermail/pve-devel/2024-April/062581.html

>> +	}
>> +
>> +	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
>> +	    $scfg,
>> +	    $storeid,
>> +	    $fh,
>> +	    $tempname,
>> +	    $format,
>> +	    $snapshot,
>> +	    $base_snapshot,
>> +	    $with_snapshots,
>> +	    $allow_rename
>> +	));
>> +
>> +	$volname = $class->create_base($storeid, $scfg, $newname);
>> +    }
>> +
>> +    return "$storeid:$volname";
>> +}
>> +
>>   # used in LVMPlugin->volume_import
>>   sub volume_import_write {
>>       my ($class, $input_fh, $output_file) = @_;
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
>
>

[pve-devel] applied: [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Fabian Grünbichler]

with a small style follow-up, thanks!

create_base prints a warning (before this patch as well):

> WARNING: Combining activation change with other commands is not advised.

maybe we could get rid of that one? I guess it doesn't like `-an`
combined with other stuff..

On December 19, 2023 3:03 pm, Hannes Duerr wrote:
> for base images we call the volume_import of the parent plugin and pass
> it as vm-image instead of base-image, then convert it back as base-image
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..96f619b 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,56 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub volume_import {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> +	$class->parse_volname($volname);
> +
> +    if (!$isBase) {
> +	return $class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $volname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	);
> +    } else {
> +	my $tempname;
> +	my $vg = $scfg->{vgname};
> +	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> +	if ($lvs->{$vg}->{$volname}) {
> +	    die "volume $vg/$volname already exists\n" if !$allow_rename;
> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
> +
> +	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
> +	} else {
> +	    $tempname = $volname;
> +	    $tempname =~ s/base/vm/;
> +	}
> +
> +	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $tempname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +
> +	$volname = $class->create_base($storeid, $scfg, $newname);
> +    }
> +
> +    return "$storeid:$volname";
> +}
> +
>  # used in LVMPlugin->volume_import
>  sub volume_import_write {
>      my ($class, $input_fh, $output_file) = @_;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> 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