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

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

[Hannes Duerr]

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


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

qemu-server:

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

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


pve-storage:

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

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


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

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH qemu-server v3 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
before the migration so that we can deactivate the original volumes
afterwards.

Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
 PVE/QemuMigrate.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b87e47a..ec4710d 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -744,6 +744,11 @@ sub phase1 {
     $conf->{lock} = 'migrate';
     PVE::QemuConfig->write_config($vmid, $conf);
 
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($ds, $drive) = @_;
+        push $self->{source_volumes}->@*, $drive->{file};
+    });
+
     $self->scan_local_volumes($vmid);
 
     # fix disk sizes to match their actual size and write changes,
@@ -1586,8 +1591,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}, $self->{source_volumes});
     };
     if (my $err = $@) {
 	$self->log('err', $err);
-- 
2.39.2

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

[Fabian Grünbichler]

On December 19, 2023 11:43 am, 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
> before the migration so that we can deactivate the original volumes
> afterwards.

we already do that though, no need for a new key in $self ;)

> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  PVE/QemuMigrate.pm | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index b87e47a..ec4710d 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -744,6 +744,11 @@ sub phase1 {
>      $conf->{lock} = 'migrate';
>      PVE::QemuConfig->write_config($vmid, $conf);
>  
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +        push $self->{source_volumes}->@*, $drive->{file};
> +    });
> +
>      $self->scan_local_volumes($vmid);

scan_local_volumes already records the local volumes (and shared ones,
in case of remote migration), and any mappings.

>  
>      # fix disk sizes to match their actual size and write changes,
> @@ -1586,8 +1591,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}, $self->{source_volumes});

one way to approach this here would be to re-use the information from
$self->{volume_map} and do a reverse lookup per volid. even easier
though would be to just get the vollist higher up in this sub, since the
local mapping takes place here as well, and we just need to call
get_vm_volumes with $conf before the volume_map replacements.

>      };
>      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
> 
> 
> 

[pve-devel] [PATCH pve-storage v3 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 | 51 ++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1d2e37c..2986b72 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -9,6 +9,7 @@ use PVE::Tools qw(run_command trim);
 use PVE::Storage::Plugin;
 use PVE::Storage::LVMPlugin;
 use PVE::JSONSchema qw(get_standard_option);
+use Data::Dumper;
 
 # see: man lvmthin
 # lvcreate -n ThinDataLV -L LargeSize VG
@@ -383,6 +384,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