[pve-devel] [PATCH v2 storage 00/10] import/export for shared storages

[pve-devel] [PATCH v2 storage 00/10] import/export for shared storages

[Fiona Ebner]

Changes in v2:
* add fix for path() in iSCSI direct plugin
* add export for iSCSI plugins
* add RFC for improving RBD volume exists helper

For remote migration, export/import functionality is also useful for
storages that are shared within a single cluster. While file-based
network storages already can rely on the default file-based
implementation in Plugin.pm, RBD and iSCSI plugins did not have the
functionality yet.


Regarding RBD:

For now, only 'raw+size' is supported and it's not possible to
export/import with snapshots. The volume or snapshot is mapped using
rbd and then the data is read via 'dd'.

Introducing an 'rbd' transport format might be feasible for more
complete (i.e. with snapshots, incremental) transfer between two RBD
storages.


For iSCSI plugins, remote migration will still fail further up the
stack though:

> can't migrate local disk 'iscsidirect:lun0': owned by other VM (owner = VM )
> can't migrate local disk 'iscsi:0.0.0.scsi-360014055b29367fb79a46b0bdb179fae': owned by other VM (owner = VM )


Fiona Ebner (9):
  iscsi direct plugin: fix return value for path() method in non-array
    context
  rbd plugin: schema: document default value for 'krbd' setting
  export: redirect stdout to avoid any unrelated messages ending up in
    the export stream
  rbd plugin: factor out helper to check if volume already exists
  rbd plugin: implement volume import/export
  iscsi plugin: support volume export
  iscsi direct plugin: support volume export
  rbd plugin: volume exists helper: distinguish between different errors
  plugins: volume import: align size up to 1KiB

Max Carrara (1):
  common: introduce common module

 src/PVE/CLI/pvesm.pm                 |   5 +-
 src/PVE/Storage/Common.pm            |  54 +++++++++++
 src/PVE/Storage/Common/Makefile      |   6 ++
 src/PVE/Storage/ISCSIDirectPlugin.pm |  65 ++++++++++++-
 src/PVE/Storage/ISCSIPlugin.pm       |  48 +++++++++
 src/PVE/Storage/LVMPlugin.pm         |   4 +-
 src/PVE/Storage/Makefile             |   2 +
 src/PVE/Storage/Plugin.pm            |   4 +-
 src/PVE/Storage/RBDPlugin.pm         | 139 ++++++++++++++++++++++++++-
 9 files changed, 318 insertions(+), 9 deletions(-)
 create mode 100644 src/PVE/Storage/Common.pm
 create mode 100644 src/PVE/Storage/Common/Makefile

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 01/10] iscsi direct plugin: fix return value for path() method in non-array context

[Fiona Ebner]

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

New in v2.

 src/PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
index eb329d4..6f02eee 100644
--- a/src/PVE/Storage/ISCSIDirectPlugin.pm
+++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
@@ -100,7 +100,7 @@ sub path {
 
     my $path = "iscsi://$portal/$target/$lun";
 
-    return ($path, $vmid, $vtype);
+    return wantarray ? ($path, $vmid, $vtype) : $path;
 }
 
 sub create_base {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 02/10] rbd plugin: schema: document default value for 'krbd' setting

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index f45ad3f..680e922 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -389,6 +389,7 @@ sub properties {
 	krbd => {
 	    description => "Always access rbd through krbd kernel module.",
 	    type => 'boolean',
+	    default => 0,
 	},
 	keyring => {
 	    description => "Client keyring contents (for external clusters).",
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 03/10] export: redirect stdout to avoid any unrelated messages ending up in the export stream

[Fiona Ebner]

Current export implementations luckily seems to not run into this
issue yet. However, for the upcoming implementation for RBD, mapping a
volume would print the device path to STDOUT, thus messing up the
export stream.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/pvesm.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/CLI/pvesm.pm b/src/PVE/CLI/pvesm.pm
index 9b9676b..d308b3d 100755
--- a/src/PVE/CLI/pvesm.pm
+++ b/src/PVE/CLI/pvesm.pm
@@ -315,7 +315,10 @@ __PACKAGE__->register_method ({
 
 	my $outfh;
 	if ($filename eq '-') {
-	    $outfh = \*STDOUT;
+	    # No other messages must go to STDOUT if it's used for the export stream!
+	    open($outfh, '>&', STDOUT) or die "unable to dup() STDOUT - $!\n";
+	    close(STDOUT);
+	    open(STDOUT, '>', '/dev/null');
 	} else {
 	    sysopen($outfh, $filename, O_CREAT|O_WRONLY|O_TRUNC)
 		or die "open($filename): $!\n";
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 04/10] rbd plugin: factor out helper to check if volume already exists

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 680e922..301918c 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -348,6 +348,16 @@ sub rbd_volume_du {
     die "got no matching image from rbd du\n";
 }
 
+my sub rbd_volume_exists {
+    my ($scfg, $storeid, $volname) = @_;
+
+    eval {
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $volname);
+	run_rbd_command($cmd, errmsg => "exist check",  quiet => 1);
+    };
+    return $@ ? undef : 1;
+}
+
 # Configuration
 
 sub type {
@@ -869,11 +879,8 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format)
 	if !$target_volname;
 
-    eval {
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $target_volname);
-	run_rbd_command($cmd, errmsg => "exist check",  quiet => 1);
-    };
-    die "target volume '${target_volname}' already exists\n" if !$@;
+    die "target volume '${target_volname}' already exists\n"
+	if rbd_volume_exists($scfg, $storeid, $target_volname);
 
     my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_image, $target_volname);
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[Fiona Ebner]

For now, only 'raw+size' is supported and it's not possible to
export/import with snapshots. The volume or snapshot is mapped using
krbd and then the data is read via 'dd'.

Introducing an 'rbd' transport format might be feasible for more
complete (i.e. with snapshots, incremental) transfer between two RBD
storages.

The code is mostly copied from the LVM plugin.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 115 +++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 301918c..0e53888 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -864,6 +864,121 @@ sub volume_has_feature {
     return undef;
 }
 
+sub volume_export_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+
+    return $class->volume_import_formats(
+	$scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots);
+}
+
+sub volume_export {
+    my (
+	$class,
+	$scfg,
+	$storeid,
+	$fh,
+	$volname,
+	$format,
+	$snapshot,
+	$base_snapshot,
+	$with_snapshots,
+    ) = @_;
+
+    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
+    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
+    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
+
+    my $file = $class->map_volume($storeid, $scfg, $volname, $snapshot);
+    eval {
+
+	my $size;
+	# should be faster than querying RBD, also checks for the device file's availability
+	run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub {
+	    my ($line) = @_;
+	    die "unexpected output from /sbin/blockdev: $line\n" if $line !~ /^(\d+)$/;
+	    $size = int($1);
+	});
+	PVE::Storage::Plugin::write_common_header($fh, $size);
+	run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh));
+    };
+    my $err = $@;
+
+    eval { $class->unmap_volume($storeid, $scfg, $volname); };
+    warn $@ if $@;
+
+    die $err if $err;
+
+    return;
+}
+
+sub volume_import_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+    return () if $with_snapshots; # not supported
+    return () if defined($base_snapshot); # not supported
+    return ('raw+size');
+}
+
+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 (undef, $name, $vmid, undef, undef, undef, $file_format) = $class->parse_volname($volname);
+    die "cannot import format $format into a volume of format $file_format\n"
+	if $file_format ne 'raw';
+
+    if (rbd_volume_exists($scfg, $storeid, $name)) {
+	die "volume $name already exists\n" if !$allow_rename;
+	warn "volume $name already exists - importing with a different name\n";
+	$name = undef;
+    }
+
+    my ($size) = PVE::Storage::Plugin::read_common_header($fh);
+    $size = int($size/1024);
+
+    my $mapped;
+    eval {
+	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
+	my $oldname = $volname;
+	$volname = $allocname;
+	if (defined($name) && $allocname ne $oldname) {
+	    die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
+	}
+
+	my $file = $class->map_volume($storeid, $scfg, $volname, undef);
+	$mapped = 1;
+
+	run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));
+    };
+    my $err = $@;
+
+    if ($mapped) {
+	eval { $class->unmap_volume($storeid, $scfg, $volname); };
+	warn $@ if $@;
+    }
+
+    if ($err) {
+	eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format); };
+	warn $@ if $@;
+	die $err;
+    }
+
+    return "$storeid:$volname";
+}
+
 sub rename_volume {
     my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 06/10] iscsi plugin: support volume export

[Fiona Ebner]

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

New in v2.

 src/PVE/Storage/ISCSIPlugin.pm | 48 ++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 6de7610..eb70453 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -596,5 +596,53 @@ sub volume_has_feature {
     return undef;
 }
 
+sub volume_export_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+
+    return () if defined($snapshot); # not supported
+    return () if defined($base_snapshot); # not supported
+    return () if $with_snapshots; # not supported
+    return ('raw+size');
+}
+
+sub volume_export {
+    my (
+	$class,
+	$scfg,
+	$storeid,
+	$fh,
+	$volname,
+	$format,
+	$snapshot,
+	$base_snapshot,
+	$with_snapshots,
+    ) = @_;
+
+    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
+    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
+    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
+    die "cannot export a snapshot in $class\n" if defined($snapshot);
+
+    my $file = $class->filesystem_path($scfg, $volname, $snapshot);
+    my $size;
+    run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub {
+	my ($line) = @_;
+	die "unexpected output from /sbin/blockdev: $line\n" if $line !~ /^(\d+)$/;
+	$size = int($1);
+    });
+    PVE::Storage::Plugin::write_common_header($fh, $size);
+    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh));
+    return;
+}
+
+sub volume_import_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+
+    return ();
+}
+
+sub volume_import {
+    die "volume import is not possible on iscsi storage\n";
+}
 
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 07/10] iscsi direct plugin: support volume export

[Fiona Ebner]

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

New in v2.

 src/PVE/Storage/ISCSIDirectPlugin.pm | 63 ++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
index 6f02eee..e5f6808 100644
--- a/src/PVE/Storage/ISCSIDirectPlugin.pm
+++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
@@ -2,9 +2,12 @@ package PVE::Storage::ISCSIDirectPlugin;
 
 use strict;
 use warnings;
+
 use IO::File;
+use JSON qw(decode_json);
 use HTTP::Request;
 use LWP::UserAgent;
+
 use PVE::Tools qw(run_command file_read_firstline trim dir_glob_regex dir_glob_foreach);
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
@@ -252,4 +255,64 @@ sub volume_has_feature {
     return undef;
 }
 
+sub volume_export_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+
+    return () if defined($snapshot); # not supported
+    return () if defined($base_snapshot); # not supported
+    return () if $with_snapshots; # not supported
+    return ('raw+size');
+}
+
+sub volume_export {
+    my (
+	$class,
+	$scfg,
+	$storeid,
+	$fh,
+	$volname,
+	$format,
+	$snapshot,
+	$base_snapshot,
+	$with_snapshots,
+    ) = @_;
+
+    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
+    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
+    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
+    die "cannot export a snapshot in $class\n" if defined($snapshot);
+
+    my $file = $class->path($scfg, $volname, $storeid, $snapshot);
+
+    my $json = '';
+    run_command(
+	['/usr/bin/qemu-img', 'info', '-f', 'raw', '--output=json', $file],
+	outfunc => sub { $json .= shift },
+    );
+    die "failed to query size information for '$file' with qemu-img\n" if !$json;
+    my $info = eval { decode_json($json) };
+    die "could not parse qemu-img info command output for '$file' - $@\n" if $@;
+
+    my ($size) = ($info->{'virtual-size'} =~ /^(\d+)$/); # untaint
+    die "size '$size' not an integer\n" if !defined($size);
+    $size = int($size); # coerce back from string
+
+    PVE::Storage::Plugin::write_common_header($fh, $size);
+    run_command(
+	['qemu-img', 'dd', 'bs=64k', "if=$file", '-f', 'raw', '-O', 'raw'],
+	output => '>&'.fileno($fh),
+    );
+    return;
+}
+
+sub volume_import_formats {
+    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
+
+    return ();
+}
+
+sub volume_import {
+    die "volume import is not possible on iscsi storage\n";
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v2 storage 08/10] rbd plugin: volume exists helper: distinguish between different errors

[Fiona Ebner]

Only claim that the image does not exist if the error message
indicates it.

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

If we don't want to match the error, I'll switch the helper over to
list volumes and check there.

New in v2.

 src/PVE/Storage/RBDPlugin.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 0e53888..004842b 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -355,7 +355,11 @@ my sub rbd_volume_exists {
 	my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $volname);
 	run_rbd_command($cmd, errmsg => "exist check",  quiet => 1);
     };
-    return $@ ? undef : 1;
+    if (my $err = $@) {
+	return if $err =~ m/No such file or directory$/;
+	die $err;
+    }
+    return 1;
 }
 
 # Configuration
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 09/10] common: introduce common module

[Fiona Ebner]

From: Max Carrara <m.carrara@proxmox.com>

This module's purpose is to provide shared functions, constants, etc.
for storage plugins and storage-related operations.

It also contains the `get_deprecation_warning` subroutine that makes
it easier to warn developers and/or plugin authors that a subroutine
will be removed in the future.

Originally-by: Max Carrara <m.carrara@proxmox.com>
[FE: start out with a different function for my use case
     fixup Makefile]
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/Common.pm       | 54 +++++++++++++++++++++++++++++++++
 src/PVE/Storage/Common/Makefile |  6 ++++
 src/PVE/Storage/Makefile        |  2 ++
 3 files changed, 62 insertions(+)
 create mode 100644 src/PVE/Storage/Common.pm
 create mode 100644 src/PVE/Storage/Common/Makefile

diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
new file mode 100644
index 0000000..3ae20dd
--- /dev/null
+++ b/src/PVE/Storage/Common.pm
@@ -0,0 +1,54 @@
+package PVE::Storage::Common;
+
+use strict;
+use warnings;
+
+=pod
+
+=head1 NAME
+
+PVE::Storage::Common - Shared functions and utilities for storage plugins and storage operations
+
+=head1 DESCRIPTION
+
+This module contains common subroutines that are mainly to be used by storage
+plugins. This module's submodules contain subroutines that are tailored towards
+a more specific or related purpose.
+
+Functions concerned with storage-related C<PVE::SectionConfig> things, helpers
+for the C<PVE::Storage> API can be found in this module. Functions that can't
+be grouped in a submodule can also be found here.
+
+=head1 SUBMODULES
+
+=over
+
+=back
+
+=head1 FUNCTIONS
+
+=cut
+
+=pod
+
+=head3 align_size_up
+
+    $aligned_size = align_size_up($size, $granularity)
+
+Returns the next size bigger than or equal to C<$size> that is aligned with a
+granularity of C<$granularity>. Prints a message if the aligned size is not
+equal to the aligned size.
+
+=cut
+
+sub align_size_up : prototype($$) {
+    my ($size, $granularity) = @_;
+
+    my $padding = ($granularity - $size % $granularity) % $granularity;
+    my $aligned_size = $size + $padding;
+    print "size $size is not aligned to granularity $granularity, rounding up to $aligned_size\n"
+	if $aligned_size != $size;
+    return $aligned_size;
+}
+
+1;
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
new file mode 100644
index 0000000..0c4bba5
--- /dev/null
+++ b/src/PVE/Storage/Common/Makefile
@@ -0,0 +1,6 @@
+SOURCES = \
+
+
+.PHONY: install
+install:
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/Common/$$i; done
diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
index d5cc942..ce3fd68 100644
--- a/src/PVE/Storage/Makefile
+++ b/src/PVE/Storage/Makefile
@@ -1,4 +1,5 @@
 SOURCES= \
+	Common.pm \
 	Plugin.pm \
 	DirPlugin.pm \
 	LVMPlugin.pm \
@@ -18,5 +19,6 @@ SOURCES= \
 
 .PHONY: install
 install:
+	make -C Common install
 	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/$$i; done
 	make -C LunCmd install
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 10/10] plugins: volume import: align size up to 1KiB

[Fiona Ebner]

Previously, the size was rounded down which, in case of an image with
non-1KiB-aligned sze (only possible for external plugins or manually
created images) would lead to errors when attempting to write beyond
the end of the too small allocated target image.

For image allocation, the size is already rounded up to the
granularity of the storage. Do the same for import.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 4 +++-
 src/PVE/Storage/Plugin.pm    | 4 +++-
 src/PVE/Storage/RBDPlugin.pm | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 88fd612..38f7fa1 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -9,6 +9,8 @@ use PVE::Tools qw(run_command trim);
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
 
+use PVE::Storage::Common;
+
 use base qw(PVE::Storage::Plugin);
 
 # lvm helper functions
@@ -677,7 +679,7 @@ sub volume_import {
     }
 
     my ($size) = PVE::Storage::Plugin::read_common_header($fh);
-    $size = int($size/1024);
+    $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
 
     eval {
 	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 5deff76..94b3646 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -15,6 +15,8 @@ use PVE::Tools qw(run_command);
 use PVE::JSONSchema qw(get_standard_option register_standard_option);
 use PVE::Cluster qw(cfs_register_file);
 
+use PVE::Storage::Common;
+
 use JSON;
 
 use base qw(PVE::SectionConfig);
@@ -1777,7 +1779,7 @@ sub volume_import {
     }
 
     my ($size) = read_common_header($fh);
-    $size = int($size/1024);
+    $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
 
     eval {
 	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 004842b..08b1580 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -18,6 +18,8 @@ use PVE::RPCEnvironment;
 use PVE::Storage::Plugin;
 use PVE::Tools qw(run_command trim file_read_firstline);
 
+use PVE::Storage::Common;
+
 use base qw(PVE::Storage::Plugin);
 
 my $get_parent_image_name = sub {
@@ -951,7 +953,7 @@ sub volume_import {
     }
 
     my ($size) = PVE::Storage::Plugin::read_common_header($fh);
-    $size = int($size/1024);
+    $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
 
     my $mapped;
     eval {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 09/10] common: introduce common module

[Max Carrara]

On Tue Dec 17, 2024 at 4:48 PM CET, Fiona Ebner wrote:
> From: Max Carrara <m.carrara@proxmox.com>
>
> This module's purpose is to provide shared functions, constants, etc.
> for storage plugins and storage-related operations.
>
> It also contains the `get_deprecation_warning` subroutine that makes
> it easier to warn developers and/or plugin authors that a subroutine
> will be removed in the future.

Thanks for including this patch already! Was really happy to see this :)

Since I've never included a commit from somebody else's series / RFC in
another series, is the commit message usually left as it is? You did
mention below that you start out with a different function for your use
case, but wouldn't it make sense to adapt the commit message overall
accordingly? In this case, just the paragraph above my reply here.

Just a small thing I was wondering about; if it's not necessary then I'm
of course fine with leaving the message as it is.

Also thanks for fixing up the Makefile -- that slipped under my radar
back then, woops!

Another thing I wanna note: It's actually *really* great you're adding
this already; I was first intending to add this in an upcoming larger
series, but since you're adding it already it means that we can all add
stuff to this module independently and don't have to wait for my series
to make it in.

So, big thanks! It's much appreciated.

>
> Originally-by: Max Carrara <m.carrara@proxmox.com>
> [FE: start out with a different function for my use case
>      fixup Makefile]
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/Storage/Common.pm       | 54 +++++++++++++++++++++++++++++++++
>  src/PVE/Storage/Common/Makefile |  6 ++++
>  src/PVE/Storage/Makefile        |  2 ++
>  3 files changed, 62 insertions(+)
>  create mode 100644 src/PVE/Storage/Common.pm
>  create mode 100644 src/PVE/Storage/Common/Makefile
>
> diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
> new file mode 100644
> index 0000000..3ae20dd
> --- /dev/null
> +++ b/src/PVE/Storage/Common.pm
> @@ -0,0 +1,54 @@
> +package PVE::Storage::Common;
> +
> +use strict;
> +use warnings;
> +
> +=pod
> +
> +=head1 NAME
> +
> +PVE::Storage::Common - Shared functions and utilities for storage plugins and storage operations
> +
> +=head1 DESCRIPTION
> +
> +This module contains common subroutines that are mainly to be used by storage
> +plugins. This module's submodules contain subroutines that are tailored towards
> +a more specific or related purpose.
> +
> +Functions concerned with storage-related C<PVE::SectionConfig> things, helpers
> +for the C<PVE::Storage> API can be found in this module. Functions that can't
> +be grouped in a submodule can also be found here.
> +
> +=head1 SUBMODULES
> +
> +=over
> +
> +=back
> +
> +=head1 FUNCTIONS
> +
> +=cut
> +
> +=pod
> +
> +=head3 align_size_up
> +
> +    $aligned_size = align_size_up($size, $granularity)
> +
> +Returns the next size bigger than or equal to C<$size> that is aligned with a
> +granularity of C<$granularity>. Prints a message if the aligned size is not
> +equal to the aligned size.
> +
> +=cut
> +
> +sub align_size_up : prototype($$) {
> +    my ($size, $granularity) = @_;
> +
> +    my $padding = ($granularity - $size % $granularity) % $granularity;
> +    my $aligned_size = $size + $padding;
> +    print "size $size is not aligned to granularity $granularity, rounding up to $aligned_size\n"
> +	if $aligned_size != $size;
> +    return $aligned_size;
> +}
> +
> +1;
> diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
> new file mode 100644
> index 0000000..0c4bba5
> --- /dev/null
> +++ b/src/PVE/Storage/Common/Makefile
> @@ -0,0 +1,6 @@
> +SOURCES = \
> +
> +
> +.PHONY: install
> +install:
> +	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/Common/$$i; done
> diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
> index d5cc942..ce3fd68 100644
> --- a/src/PVE/Storage/Makefile
> +++ b/src/PVE/Storage/Makefile
> @@ -1,4 +1,5 @@
>  SOURCES= \
> +	Common.pm \
>  	Plugin.pm \
>  	DirPlugin.pm \
>  	LVMPlugin.pm \
> @@ -18,5 +19,6 @@ SOURCES= \
>  
>  .PHONY: install
>  install:
> +	make -C Common install
>  	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/$$i; done
>  	make -C LunCmd install



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 09/10] common: introduce common module

[Fiona Ebner]

Am 18.12.24 um 10:36 schrieb Max Carrara:
> On Tue Dec 17, 2024 at 4:48 PM CET, Fiona Ebner wrote:
>> From: Max Carrara <m.carrara@proxmox.com>
>>
>> This module's purpose is to provide shared functions, constants, etc.
>> for storage plugins and storage-related operations.
>>
>> It also contains the `get_deprecation_warning` subroutine that makes
>> it easier to warn developers and/or plugin authors that a subroutine
>> will be removed in the future.
> 
> Thanks for including this patch already! Was really happy to see this :)
> 
> Since I've never included a commit from somebody else's series / RFC in
> another series, is the commit message usually left as it is? You did
> mention below that you start out with a different function for your use
> case, but wouldn't it make sense to adapt the commit message overall
> accordingly? In this case, just the paragraph above my reply here.
> 

Yes, the commit message should be adapted here. Thank you for pointing
this out!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 00/10] import/export for shared storages

[Fiona Ebner]

Sent a follow-up for container remote migration:
https://lore.proxmox.com/pve-devel/20241218103206.20822-1-f.ebner@proxmox.com/T/#u


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 01/10] iscsi direct plugin: fix return value for path() method in non-array context

[Fiona Ebner]

Am 17.12.24 um 16:48 schrieb Fiona Ebner:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
>  src/PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
> index eb329d4..6f02eee 100644
> --- a/src/PVE/Storage/ISCSIDirectPlugin.pm
> +++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
> @@ -100,7 +100,7 @@ sub path {
>  
>      my $path = "iscsi://$portal/$target/$lun";
>  
> -    return ($path, $vmid, $vtype);
> +    return wantarray ? ($path, $vmid, $vtype) : $path;
>  }
>  
>  sub create_base {

Actually, not sure if this is considered required by the plugin API (and
thus whether to call it a "fix"). The call in Storage.pm is

>     my ($path, $owner, $vtype) = $plugin->path($scfg, $volname, $storeid, $snapname);
>     return wantarray ? ($path, $owner, $vtype) : $path;

However, there are calls

> my $file = $class->path($scfg, $volname, $storeid)

in the (import/export calls) of the base implementation in Plugin.pm
which can get inherited by other plugins. Either we require the
wantarray detection in the API (technically also requires a APIAGE/VER
bump) or we change these calls not to expect the detection.

IMHO, it seems nicer to have the detection, as it's very easy to end up
with a broken call in scalar context.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 01/10] iscsi direct plugin: fix return value for path() method in non-array context

[Fiona Ebner]

Am 18.12.24 um 14:39 schrieb Fiona Ebner:
> Am 17.12.24 um 16:48 schrieb Fiona Ebner:
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> New in v2.
>>
>>  src/PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
>> index eb329d4..6f02eee 100644
>> --- a/src/PVE/Storage/ISCSIDirectPlugin.pm
>> +++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
>> @@ -100,7 +100,7 @@ sub path {
>>  
>>      my $path = "iscsi://$portal/$target/$lun";
>>  
>> -    return ($path, $vmid, $vtype);
>> +    return wantarray ? ($path, $vmid, $vtype) : $path;
>>  }
>>  
>>  sub create_base {
> 
> Actually, not sure if this is considered required by the plugin API (and
> thus whether to call it a "fix"). The call in Storage.pm is
> 
>>     my ($path, $owner, $vtype) = $plugin->path($scfg, $volname, $storeid, $snapname);
>>     return wantarray ? ($path, $owner, $vtype) : $path;
> 
> However, there are calls
> 
>> my $file = $class->path($scfg, $volname, $storeid)
> 
> in the (import/export calls) of the base implementation in Plugin.pm
> which can get inherited by other plugins. Either we require the
> wantarray detection in the API (technically also requires a APIAGE/VER
> bump) or we change these calls not to expect the detection.
> 

For v3 of the series, I'll just go with the latter. We can always do the
API change later on.

> IMHO, it seems nicer to have the detection, as it's very easy to end up
> with a broken call in scalar context.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 06/10] iscsi plugin: support volume export

[Filip Schauer]

I tested exporting a volume from an iSCSI storage.
First I installed Debian in a VM on a LUN and then exported the volume:

```
$ pvesm export tgtstorage:0.0.1.scsi-360000000000000000e00000000010001 
raw+size output
3915055104 bytes (3.9 GB, 3.6 GiB) copied, 11 s, 356 MB/s
65472+0 records in
65472+0 records out
4290772992 bytes (4.3 GB, 4.0 GiB) copied, 12.0677 s, 356 MB/s
```

Then I imported the volume into a directory storage:

```
$ pvesm import local:117/vm-117-disk-0.raw raw+size output
```

Assigned the disk to a VM and it booted up just fine.

Trying to import into an iSCSI storage fails as expected:

```
$ pvesm import tgtstorage raw+size output
cannot import into volume 'tgtstorage'
```

Tested-by: Filip Schauer <f.schauer@proxmox.com>


On 17/12/2024 16:48, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> New in v2.
>
>   src/PVE/Storage/ISCSIPlugin.pm | 48 ++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
>
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> index 6de7610..eb70453 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -596,5 +596,53 @@ sub volume_has_feature {
>       return undef;
>   }
>   
> +sub volume_export_formats {
> +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> +
> +    return () if defined($snapshot); # not supported
> +    return () if defined($base_snapshot); # not supported
> +    return () if $with_snapshots; # not supported
> +    return ('raw+size');
> +}
> +
> +sub volume_export {
> +    my (
> +	$class,
> +	$scfg,
> +	$storeid,
> +	$fh,
> +	$volname,
> +	$format,
> +	$snapshot,
> +	$base_snapshot,
> +	$with_snapshots,
> +    ) = @_;
> +
> +    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
> +    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
> +    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
> +    die "cannot export a snapshot in $class\n" if defined($snapshot);
> +
> +    my $file = $class->filesystem_path($scfg, $volname, $snapshot);
> +    my $size;
> +    run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub {
> +	my ($line) = @_;
> +	die "unexpected output from /sbin/blockdev: $line\n" if $line !~ /^(\d+)$/;
> +	$size = int($1);
> +    });
> +    PVE::Storage::Plugin::write_common_header($fh, $size);
> +    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh));
> +    return;
> +}
> +
> +sub volume_import_formats {
> +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> +
> +    return ();
> +}
> +
> +sub volume_import {
> +    die "volume import is not possible on iscsi storage\n";
> +}
>   
>   1;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 07/10] iscsi direct plugin: support volume export

[Filip Schauer]

To test exporting a volume from the `iscsidirect` storage backend, I
installed `libiscsi-bin` and configured the storage in
`/etc/pve/storage.cfg` by using the same options as I used for the
`iscsi` storage.

```
$ pvesm export tgtdirectstorage:lun1 raw+size output
```

Then I imported the volume into a directory storage:

```
$ pvesm import local:117/vm-117-disk-0.raw raw+size output
Formatting '/var/lib/vz/images/117/vm-117-disk-0.raw', fmt=raw 
size=4290772992 preallocation=off
65472+0 records in
65472+0 records out
4290772992 bytes (4.3 GB, 4.0 GiB) copied, 3.62069 s, 1.2 GB/s
successfully imported 'local:117/vm-117-disk-0.raw'
```

Assigned the disk to a VM and it also booted up just fine.

Trying to import into a user mode iSCSI storage fails as expected:

```
$ pvesm import tgtdirectstorage raw+size output
cannot import into volume 'tgtdirectstorage'
```

Tested-by: Filip Schauer <f.schauer@proxmox.com>

On 17/12/2024 16:48, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> New in v2.
>
>   src/PVE/Storage/ISCSIDirectPlugin.pm | 63 ++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
>
> diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
> index 6f02eee..e5f6808 100644
> --- a/src/PVE/Storage/ISCSIDirectPlugin.pm
> +++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
> @@ -2,9 +2,12 @@ package PVE::Storage::ISCSIDirectPlugin;
>   
>   use strict;
>   use warnings;
> +
>   use IO::File;
> +use JSON qw(decode_json);
>   use HTTP::Request;
>   use LWP::UserAgent;
> +
>   use PVE::Tools qw(run_command file_read_firstline trim dir_glob_regex dir_glob_foreach);
>   use PVE::Storage::Plugin;
>   use PVE::JSONSchema qw(get_standard_option);
> @@ -252,4 +255,64 @@ sub volume_has_feature {
>       return undef;
>   }
>   
> +sub volume_export_formats {
> +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> +
> +    return () if defined($snapshot); # not supported
> +    return () if defined($base_snapshot); # not supported
> +    return () if $with_snapshots; # not supported
> +    return ('raw+size');
> +}
> +
> +sub volume_export {
> +    my (
> +	$class,
> +	$scfg,
> +	$storeid,
> +	$fh,
> +	$volname,
> +	$format,
> +	$snapshot,
> +	$base_snapshot,
> +	$with_snapshots,
> +    ) = @_;
> +
> +    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
> +    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
> +    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
> +    die "cannot export a snapshot in $class\n" if defined($snapshot);
> +
> +    my $file = $class->path($scfg, $volname, $storeid, $snapshot);
> +
> +    my $json = '';
> +    run_command(
> +	['/usr/bin/qemu-img', 'info', '-f', 'raw', '--output=json', $file],
> +	outfunc => sub { $json .= shift },
> +    );
> +    die "failed to query size information for '$file' with qemu-img\n" if !$json;
> +    my $info = eval { decode_json($json) };
> +    die "could not parse qemu-img info command output for '$file' - $@\n" if $@;
> +
> +    my ($size) = ($info->{'virtual-size'} =~ /^(\d+)$/); # untaint
> +    die "size '$size' not an integer\n" if !defined($size);
> +    $size = int($size); # coerce back from string
> +
> +    PVE::Storage::Plugin::write_common_header($fh, $size);
> +    run_command(
> +	['qemu-img', 'dd', 'bs=64k', "if=$file", '-f', 'raw', '-O', 'raw'],
> +	output => '>&'.fileno($fh),
> +    );
> +    return;
> +}
> +
> +sub volume_import_formats {
> +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> +
> +    return ();
> +}
> +
> +sub volume_import {
> +    die "volume import is not possible on iscsi storage\n";
> +}
> +
>   1;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 00/10] import/export for shared storages

[Aaron Lauterer]

Did a high-level test between a PVE+Ceph cluster and a single PVE node 
with the remote-migration of a Windows Server 2022 VM with EFI & TPM disks.

Ceph RBD -> remote LVM thin
LVM thin -> remote Ceph RBD

Worked in both directions, and the VM booted up as expected after each 
migration.

One thing I ran into, only tangentially related to this series, is that 
we don't support the 'raw+size' option for ZFS. Maybe we can get it 
working on ZFS at least for VM disk images (zvol)?

Maybe it might also be time to consider if we want to handle CT volumes 
differently on ZFS in the long term (file based dataset). In all other 
storage options we have a block dev or raw file that we loop mount into 
the CT. Aligning this with ZFS would probably simplify things quite a bit.


With the above mentioned tests, partially:
Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[Daniel Kral]

On 12/17/24 16:48, Fiona Ebner wrote:
> [ ... ]
> +
> +    die "volume export format $format not available for $class\n" if $format ne 'raw+size';
> +    die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots;
> +    die "cannot export an incremental stream in $class\n" if defined($base_snapshot);
> +
> +    my $file = $class->map_volume($storeid, $scfg, $volname, $snapshot);
> +    eval {
> +

nit: unnecessary newline

> +	my $size;
> +	# should be faster than querying RBD, also checks for the device file's availability
> +	run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub {
> +	    my ($line) = @_;
> +	    die "unexpected output from /sbin/blockdev: $line\n" if $line !~ /^(\d+)$/;
> +	    $size = int($1);
> +	});
> [ ... ]
> +
> +    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 (undef, $name, $vmid, undef, undef, undef, $file_format) = $class->parse_volname($volname);

nit: is there any downside to

`my ($name, $vmid, $format) = ($class->parse_volname($volname))[1,2,6];`

> +    die "cannot import format $format into a volume of format $file_format\n"
> +	if $file_format ne 'raw';
> [ ... ]

Besides the two minor remarks, the `volume_{import,export}{,_formats}` 
subs look good to me (wrt. the LVMPlugin implementation it is based on). 
In the future it would probably be nice to factor out some common logic 
between the RBD, LVM and (now) ISCSI storage plugin.

With respect to patch #2, #4, #5, #8, #9, #10 and the followup patch #1 
from [0], I've tested the manual exports, manual imports and remote 
migrations of VMs and container with rbd storages.

- When exporting with "pvesm export ... --with-snapshots 1" I get an 
expected error
- When exporting with any format besides "raw+size" I get an expected error
- When exporting with "pvesm export ... --snapshot <snapshot>" the 
volume is correctly mapped and exported
- When exporting with "pvesm export ...", the volume has the same 
checksum as with "rbd export ..." with the size header prepended
- When importing with "--allow-rename" the volume is correctly renamed
- Remote migration works for VMs with `qm remote-migrate ...` from RBD 
to directory storage (ext4, xfs) and lvm
- Remote migration works for container with `pct remote-migrate ...` 
(after also applying patch #1 from the follow up) from RBD to directory 
storage (ext4, xfs) and lvm

I checked the integrity of the volumes with `md5sum` and if they boot up 
fine. The only thing that I noticed - which is probably unrelated to 
this patch series - is that VMs as well as container have "migrate" 
locks after successful remote migrations (the lock is removed if I kill 
the migration during the process). From the log output alone it seems 
that the local VM/CT is never unlocked, only the one on the remote.

I also tested importing the volumes with `pvesm import ...`, which I had 
exported before with `pvesm export ...`, which worked just as expected.

Consider this patch as:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[Fiona Ebner]

Am 18.12.24 um 15:20 schrieb Daniel Kral:
> - When exporting with "pvesm export ...", the volume has the same
> checksum as with "rbd export ..." with the size header prepended

Well, I totally missed the existence of "rbd export" in my hurry to get
this working. Seems to be about 1.5 times faster than mapping+dd from
some initial testing. Will use that in v3.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[DERUMIER, Alexandre via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 13306 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "d.kral@proxmox.com" <d.kral@proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export
Date: Wed, 18 Dec 2024 15:33:46 +0000
Message-ID: <15b28f8ffe3292817c5171ab8e683b0d8f446dfe.camel@groupe-cyllene.com>

>>Am 18.12.24 um 15:20 schrieb Daniel Kral:
> >>- When exporting with "pvesm export ...", the volume has the same
> checksum as with "rbd export ..." with the size header prepended

>>Well, I totally missed the existence of "rbd export" in my hurry to
>>get
>>this working. Seems to be about 1.5 times faster than mapping+dd from
>>some initial testing. Will use that in v3.

Hi, fiona, rbd export|import, this is the way, like zfs send|receive. 
(with snapshot support too, with export-diff|import-diff)

I't really fast because, if I remember, it's use big block size and is 
able to do parallelism. (can be tunned with --rbd-concurrent-
management-ops <number> )



No related, but could it be possible to implement it, for simple
vm/template full cloning with source+target are both rbd ? It's really
faster with 'qemu-img convert'





[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[Fiona Ebner]

Am 18.12.24 um 16:33 schrieb DERUMIER, Alexandre via pve-devel:
>>> Am 18.12.24 um 15:20 schrieb Daniel Kral:
>>>> - When exporting with "pvesm export ...", the volume has the same
>> checksum as with "rbd export ..." with the size header prepended
> 
>>> Well, I totally missed the existence of "rbd export" in my hurry to
>>> get
>>> this working. Seems to be about 1.5 times faster than mapping+dd from
>>> some initial testing. Will use that in v3.
> 
> Hi, fiona, rbd export|import, this is the way, like zfs send|receive. 
> (with snapshot support too, with export-diff|import-diff)
> 

Saw that in the man page, and yes, would be the way to go for an 'rbd'
transport format with incremental support similar to 'zfs' :)

> I't really fast because, if I remember, it's use big block size and is 
> able to do parallelism. (can be tunned with --rbd-concurrent-
> management-ops <number> )
> 

We'll need to evaluate trade-off between speed and putting more load on
the system/Ceph. After all, the disk move might not be the most
important thing happening at that moment.

> No related, but could it be possible to implement it, for simple
> vm/template full cloning with source+target are both rbd ? It's really
> faster with 'qemu-img convert'

Hmm, we could shift offline copy of images to the storage layer (at
least in some cases). We just need a version of storage_migrate() that
goes to the local node instead of SSH to a different one. Could you
please open a feature request for this?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[DERUMIER, Alexandre via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 12949 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "d.kral@proxmox.com" <d.kral@proxmox.com>, "f.ebner@proxmox.com" <f.ebner@proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export
Date: Thu, 19 Dec 2024 10:43:29 +0000
Message-ID: <ab3972308b117d6acbdab8db5d81d0b195733db0.camel@groupe-cyllene.com>

> No related, but could it be possible to implement it, for simple
> vm/template full cloning with source+target are both rbd ? It's
> really
> faster with 'qemu-img convert'

>>Hmm, we could shift offline copy of images to the storage layer (at
>>least in some cases). We just need a version of storage_migrate()
>>that
>>goes to the local node instead of SSH to a different one. Could you
>>please open a feature request for this?

Done:

https://bugzilla.proxmox.com/show_bug.cgi?id=6002

(Another way could be clone + flatten if the storage support it (like
ceph), this avoid  network read/write, and really offloading the copy)

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel