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

[pve-devel] [PATCH v2 qemu-server pve-storage 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

qemu-server:

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

 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 | 60 ++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)


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

-- 
2.39.2

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

[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.

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;
 
     my $storecfg = $self->{storecfg};
     my $opts = $self->{opts};
@@ -1584,10 +1585,10 @@ sub phase3_cleanup {
 	$self->{errors} = 1;
     }
 
+
     # 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 v2 qemu-server pve-storage 1/2] migration: secure and use source volume names for cleanup

[Fiona Ebner]

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

[pve-devel] [PATCH v2 qemu-server pve-storage 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 | 60 ++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1d2e37c..6c95919 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -383,6 +383,66 @@ sub volume_has_feature {
     return undef;
 }
 
+sub volume_import {
+    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
+    die "volume import format $format not available for $class\n"
+	if $format ne 'raw+size';
+    die "cannot import volumes together with their snapshots in $class\n"
+	if $with_snapshots;
+    die "cannot import an incremental stream in $class\n" if defined($base_snapshot);
+
+    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
+	$class->parse_volname($volname);
+    die "cannot import format $format into a file of format $file_format\n"
+	if $file_format ne 'raw';
+
+    my $oldbasename;
+    if (!$isBase) {
+	($storeid, $volname) =  split (/:/, $class->SUPER::volume_import(
+	    $scfg,
+	    $storeid,
+	    $fh,
+	    $name,
+	    $format,
+	    $snapshot,
+	    $base_snapshot,
+	    $with_snapshots,
+	    $allow_rename
+	));
+    } else {
+	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";
+
+	    $volname = $class->find_free_diskname($storeid, $scfg, $vmid);
+	} else {
+	    $oldbasename = $volname;
+	    $volname =~ s/base/vm/;
+	}
+
+	($storeid, $volname) =  split (/:/, $class->SUPER::volume_import(
+	    $scfg,
+	    $storeid,
+	    $fh,
+	    $volname,
+	    $format,
+	    $snapshot,
+	    $base_snapshot,
+	    $with_snapshots,
+	    $allow_rename
+	));
+
+	$volname = $class->create_base($storeid, $scfg, $volname);
+	if ($oldbasename) {
+	    $volname= $oldbasename;
+	}
+    }
+
+    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 v2 qemu-server pve-storage 2/2] fix #1611: implement import of base-images for LVM-thin Storage

[Fiona Ebner]

Am 07.12.23 um 10:12 schrieb 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 | 60 ++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..6c95919 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,66 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub volume_import {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +    die "volume import format $format not available for $class\n"
> +	if $format ne 'raw+size';
> +    die "cannot import volumes together with their snapshots in $class\n"
> +	if $with_snapshots;
> +    die "cannot import an incremental stream in $class\n" if defined($base_snapshot);
> +

Nit: Not really hurting, but since we use the parent's import method, I
think we can be fine relying on the checks there. If the parent's method
gains support for more in the future and can drop a check, we'd just
need to remove the check here too.

> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> +	$class->parse_volname($volname);
> +    die "cannot import format $format into a file of format $file_format\n"
> +	if $file_format ne 'raw';
> +
> +    my $oldbasename;

Nit: This variable is not used in the if branch, so declaration could be
moved into the else branch.

> +    if (!$isBase) {
> +	($storeid, $volname) =  split (/:/, $class->SUPER::volume_import(

Could also just be
return $class->SUPER...
instead of doing the split and re-assembling for return below. Also
avoids overwriting the passed-in params $storeid and $volname.

> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $name,

I'd still pass $volname just to be more future-proof

> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +    } else {
> +	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";
> +
> +	    $volname = $class->find_free_diskname($storeid, $scfg, $vmid);> +	} else {
> +	    $oldbasename = $volname;
> +	    $volname =~ s/base/vm/;
> +	}

Nit: I'd prefer to have new variables for both the temporary volname we
pass to the parent's volume_import and the new volname. Since they serve
different purposes.

> +
> +	($storeid, $volname) =  split (/:/, $class->SUPER::volume_import(

Using parse_volid() would be more robust than using an ad-hoc split.

> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $volname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +
> +	$volname = $class->create_base($storeid, $scfg, $volname);
> +	if ($oldbasename) {
> +	    $volname= $oldbasename;
> +	}

create_base() will tell you the actual name, you should not override it
with the old one. We expect it to match, but then there's no need for
the assignment. If it doesn't match, you'd be returning something wrong.
Or what am I missing?

> +    }
> +
> +    return "$storeid:$volname";
> +}
> +
>  # used in LVMPlugin->volume_import
>  sub volume_import_write {
>      my ($class, $input_fh, $output_file) = @_;

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

[Fiona Ebner]

Am 14.12.23 um 15:23 schrieb Fiona Ebner:
> Am 07.12.23 um 10:12 schrieb Hannes Duerr:
>> +
>> +	($storeid, $volname) =  split (/:/, $class->SUPER::volume_import(
> 
> Using parse_volid() would be more robust than using an ad-hoc split.
> 

Sorry, I meant:

PVE::Storage::parse_volume_id()

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

[Hannes Dürr]

On 12/14/23 15:23, Fiona Ebner wrote:
> create_base() will tell you the actual name, you should not override it
> with the old one. We expect it to match, but then there's no need for
> the assignment. If it doesn't match, you'd be returning something wrong.
> Or what am I missing?
If i am not mistaken there is the case:


Plugin           current imagename    action

lvmthin:        base-100-disk-1         old_base =

LVMplug:  -> vm-100-disk-1            not working due to collision

LVMplug:  -> vm-100-disk-2            call find_free_disk()

lvmthin    -> base-100-disk-2          != old_base

But I think we could also just skip this case and remove the 
$oldbasename completly.

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

[Fiona Ebner]

Am 18.12.23 um 13:02 schrieb Hannes Dürr:
> On 12/14/23 15:23, Fiona Ebner wrote:
>> create_base() will tell you the actual name, you should not override it
>> with the old one. We expect it to match, but then there's no need for
>> the assignment. If it doesn't match, you'd be returning something wrong.
>> Or what am I missing?
> If i am not mistaken there is the case:
> 
> 
> Plugin           current imagename    action
> 
> lvmthin:        base-100-disk-1         old_base =
> 
> LVMplug:  -> vm-100-disk-1            not working due to collision
> 
> LVMplug:  -> vm-100-disk-2            call find_free_disk()
> 
> lvmthin    -> base-100-disk-2          != old_base
> 

But in this case, returning old_base, i.e. base-100-disk-1, is wrong,
because that is not what the image is actually called. You would need to
additionally rename the image in LVM, but I don't think that's worth it.