[pve-devel] [PATCH storage/container v3 00/13] import/export for shared storages

[pve-devel] [PATCH storage/container v3 00/13] import/export for shared storages

[Fiona Ebner]

Chnages in v3:
* add fix for path() calls in Plugin.pm
* use rbd export/import instead of mapping+dd
* list images for volume exists helper
* fix commit message for patch adding Common module
* add patch dropping outdated error message check

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 )

pve-storage:

Fiona Ebner (10):
  plugin: export/import: fix calls to path() method
  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
  rbd plugin: improve volume exists helper
  iscsi plugin: support volume export
  iscsi direct plugin: support volume export
  plugins: volume import: align size up to 1KiB
  rbd plugin: list: drop outdated error message check

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 |  63 ++++++++++++
 src/PVE/Storage/ISCSIPlugin.pm       |  48 +++++++++
 src/PVE/Storage/LVMPlugin.pm         |   4 +-
 src/PVE/Storage/Makefile             |   2 +
 src/PVE/Storage/Plugin.pm            |  12 ++-
 src/PVE/Storage/RBDPlugin.pm         | 142 ++++++++++++++++++++++++---
 9 files changed, 317 insertions(+), 19 deletions(-)
 create mode 100644 src/PVE/Storage/Common.pm
 create mode 100644 src/PVE/Storage/Common/Makefile


pve-container:

Fiona Ebner (2):
  migration: allow rbd storages for remote migration
  migration: add reminder to evaluate dropping seemingly useless check
    for PVE 9

 src/PVE/LXC/Migrate.pm | 3 +++
 1 file changed, 3 insertions(+)


Summary over all repositories:
  10 files changed, 320 insertions(+), 19 deletions(-)

-- 
Generated by git-murpp 0.5.0


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

[pve-devel] [PATCH storage v3 01/13] plugin: export/import: fix calls to path() method

[Fiona Ebner]

The plugin API does not require call context detection for the
returned value of the path() method. See other plugins like
ISCSIDirect/ZFS that do not implement it. So do not expect it for
external plugins either.

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

New in v3.

 src/PVE/Storage/Plugin.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 5deff76..92609ad 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1697,7 +1697,7 @@ sub volume_export {
 
     my $err_msg = "volume export format $format not available for $class\n";
     if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
-	my $file = $class->path($scfg, $volname, $storeid) or die $err_msg;
+	my ($file) = $class->path($scfg, $volname, $storeid) or die $err_msg;
 	my $file_format = ($class->parse_volname($volname))[6];
 	my $size = file_size_info($file, undef, $file_format);
 
@@ -1731,7 +1731,7 @@ sub volume_export {
 sub volume_export_formats {
     my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
     if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
-	my $file = $class->path($scfg, $volname, $storeid)
+	my ($file) = $class->path($scfg, $volname, $storeid)
 	    or return;
 	my $format = ($class->parse_volname($volname))[6];
 	my $size = file_size_info($file, undef, $format);
@@ -1769,7 +1769,7 @@ sub volume_import {
 
     # Check for an existing file first since interrupting alloc_image doesn't
     # free it.
-    my $file = $class->path($scfg, $volname, $storeid);
+    my ($file) = $class->path($scfg, $volname, $storeid);
     if (-e $file) {
 	die "file '$file' already exists\n" if !$allow_rename;
 	warn "file '$file' already exists - importing with a different name\n";
@@ -1786,7 +1786,7 @@ sub volume_import {
 	if (defined($name) && $allocname ne $oldname) {
 	    die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
 	}
-	my $file = $class->path($scfg, $volname, $storeid)
+	my ($file) = $class->path($scfg, $volname, $storeid)
 	    or die "internal error: failed to get path to newly allocated volume $volname\n";
 	if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') {
 	    run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
-- 
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 storage v3 02/13] 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 storage v3 03/13] 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 storage v3 04/13] 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 storage v3 05/13] 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 exported or
imported via the corresponding 'rbd' commands.

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

Use the '--dest-pool' switch rather than '-p' for import, because the
latter is deprecated.

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

Changes in v3:
* use rbd export/import

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

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 301918c..44770b0 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -88,7 +88,13 @@ my $build_cmd = sub {
     my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
 
-    my $cmd = [$binary, '-p', $pool];
+
+    my $cmd = [$binary];
+    if ($op eq 'import') {
+	push $cmd->@*, '--dest-pool', $pool;
+    } else {
+	push $cmd->@*, '-p', $pool;
+    }
 
     if (defined(my $namespace = $scfg->{namespace})) {
 	# some subcommands will fail if the --namespace parameter is present
@@ -864,6 +870,100 @@ 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 ($size) = $class->volume_size_info($scfg, $storeid, $volname);
+    PVE::Storage::Plugin::write_common_header($fh, $size);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'export', '--export-format', '1', $volname, '-');
+    run_rbd_command(
+	$cmd,
+	errmsg => 'could not export image',
+	output => '>&'.fileno($fh),
+    );
+
+    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";
+	$volname = $class->find_free_diskname($storeid, $scfg, $vmid, $file_format);
+    }
+
+    my ($size) = PVE::Storage::Plugin::read_common_header($fh);
+    $size = int($size/1024);
+
+    eval {
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'import', '--export-format', '1', '-', $volname);
+	run_rbd_command(
+	    $cmd,
+	    errmsg => 'could not import image',
+	    input => '<&'.fileno($fh),
+	);
+    };
+    if (my $err = $@) {
+	# FIXME there is a slight race between finding the free disk name and removal here
+	# Does not only affect this plugin, see:
+	# https://lore.proxmox.com/pve-devel/20240403150712.262773-1-h.duerr@proxmox.com/
+	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 storage v3 06/13] rbd plugin: improve volume exists helper

[Fiona Ebner]

Currently, the helper would not distinguish between different kinds
of errors. Instead of relying on an error, list the images and check
there.

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

Changes in v3:
* do not match error message, list images instead

 src/PVE/Storage/RBDPlugin.pm | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 44770b0..1d1cfad 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -357,11 +357,24 @@ sub rbd_volume_du {
 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;
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'ls', '--format', 'json');
+    my $raw = '';
+    run_rbd_command(
+	$cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => sub { $raw .= shift; });
+
+    my $list;
+    if ($raw =~ m/^(\[.*\])$/s) { # untaint
+	$list = eval { JSON::decode_json($1); };
+	die "invalid JSON output from 'rbd ls': $@\n" if $@;
+    } else {
+	die "got unexpected data from 'rbd ls': '$raw'\n";
+    }
+
+    for my $name ($list->@*) {
+	return 1 if $name eq $volname;
+    }
+
+    return 0;
 }
 
 # 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 storage v3 07/13] iscsi plugin: support volume export

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 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 storage v3 08/13] iscsi direct plugin: support volume export

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 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 eb329d4..0aca4b6 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] [PATCH storage v3 09/13] 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 starts out with a align_size_up() function, that will (initially)
be used for volume import.

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

Changes in v3:
* fix commit message

 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 storage v3 10/13] 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 92609ad..65cf43f 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 1d1cfad..a362529 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 {
@@ -955,7 +957,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 $cmd = $rbd_cmd->($scfg, $storeid, 'import', '--export-format', '1', '-', $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 storage v3 11/13] rbd plugin: list: drop outdated error message check

[Fiona Ebner]

This became outdated after Ceph commit ac547a5b7dc ("rbd: return 0 and
an empty list when pool is entirely empty") 11 years ago. See also:
https://tracker.ceph.com/issues/6693

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

New in v3.

 src/PVE/Storage/RBDPlugin.pm | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index a362529..ef4faa6 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -216,12 +216,7 @@ sub rbd_ls {
     my $parser = sub { $raw .= shift };
 
     my $cmd = $rbd_cmd->($scfg, $storeid, 'ls', '-l', '--format', 'json');
-    eval {
-	run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
-    };
-    my $err = $@;
-
-    die $err if $err && $err !~ m/doesn't contain rbd images/ ;
+    run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
 
     my $result;
     if ($raw eq '') {
-- 
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 container v3 12/13] migration: allow rbd storages for remote migration

[Fiona Ebner]

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

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 0debb9e..e1e6cab 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -303,6 +303,7 @@ sub phase1 {
 	    if ($remote) {
 		push @$migratable_storages, 'cifs';
 		push @$migratable_storages, 'nfs';
+		push @$migratable_storages, 'rbd';
 	    }
 
 	    die "storage type '$scfg->{type}' not supported\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 container v3 13/13] migration: add reminder to evaluate dropping seemingly useless check for PVE 9

[Fiona Ebner]

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

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index e1e6cab..a550d70 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -292,6 +292,8 @@ sub phase1 {
 	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 	    my $scfg =  PVE::Storage::storage_config($self->{storecfg}, $sid);
 
+	    # FIXME PVE 9 - why is this even here, can't it just be dropped completely? The storage
+	    # layer already dies if there is no valid transport format.
 	    # TODO move to storage plugin layer?
 	    my $migratable_storages = [
 		'dir',
-- 
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 storage v3 08/13] iscsi direct plugin: support volume export

[Fiona Ebner]

Am 19.12.24 um 11:43 schrieb Fiona Ebner:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Sorry, forgot to mention the changes here:
* do not rely on path() using wantarray


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

[pve-devel] partially-applied: [PATCH storage/container v3 00/13] import/export for shared storages

[Fabian Grünbichler]

Folded in Filip's T-B from v2, and applied all but the last
pve-container patch - those checks are there to cancel the migration
early on if a storage that is not supported is contained in the mix..

ideally they should move to the storage layer, as the original comment
indicates.. basically a helper that checks export and import formats
(and snapshots/base status) and returns a bool?

On December 19, 2024 11:43 am, Fiona Ebner wrote:
> Chnages in v3:
> * add fix for path() calls in Plugin.pm
> * use rbd export/import instead of mapping+dd
> * list images for volume exists helper
> * fix commit message for patch adding Common module
> * add patch dropping outdated error message check
> 
> 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 )
> 
> pve-storage:
> 
> Fiona Ebner (10):
>   plugin: export/import: fix calls to path() method
>   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
>   rbd plugin: improve volume exists helper
>   iscsi plugin: support volume export
>   iscsi direct plugin: support volume export
>   plugins: volume import: align size up to 1KiB
>   rbd plugin: list: drop outdated error message check
> 
> 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 |  63 ++++++++++++
>  src/PVE/Storage/ISCSIPlugin.pm       |  48 +++++++++
>  src/PVE/Storage/LVMPlugin.pm         |   4 +-
>  src/PVE/Storage/Makefile             |   2 +
>  src/PVE/Storage/Plugin.pm            |  12 ++-
>  src/PVE/Storage/RBDPlugin.pm         | 142 ++++++++++++++++++++++++---
>  9 files changed, 317 insertions(+), 19 deletions(-)
>  create mode 100644 src/PVE/Storage/Common.pm
>  create mode 100644 src/PVE/Storage/Common/Makefile
> 
> 
> pve-container:
> 
> Fiona Ebner (2):
>   migration: allow rbd storages for remote migration
>   migration: add reminder to evaluate dropping seemingly useless check
>     for PVE 9
> 
>  src/PVE/LXC/Migrate.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 
> Summary over all repositories:
>   10 files changed, 320 insertions(+), 19 deletions(-)
> 
> -- 
> Generated by git-murpp 0.5.0
> 
> 
> _______________________________________________
> 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

Re: [pve-devel] partially-applied: [PATCH storage/container v3 00/13] import/export for shared storages

[Fiona Ebner]

Am 19.12.24 um 12:45 schrieb Fabian Grünbichler:
> Folded in Filip's T-B from v2, and applied all but the last
> pve-container patch - those checks are there to cancel the migration
> early on if a storage that is not supported is contained in the mix..
> 
> ideally they should move to the storage layer, as the original comment
> indicates.. basically a helper that checks export and import formats
> (and snapshots/base status) and returns a bool?

Okay, I see. Yes, sounds nice to have for early checks. QemuMigrate can
benefit from that too. I guess splitting out the beginning of
storage_migrate() with volume_transfer_formats() should be enough?

We do need to make sure that plugins correctly treat "subvol" before
claiming they can transfer a volume of course, but at least the ones
already in the list should already do that (for those it would be a
pre-existing bug if not).


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