We often have a generic implementation in Plugin.pm for filesystem-based
storages (maybe erroring out if there is no $scfg->{path}). Is there a
reason not to use such an approach, i.e. making file_reassign_volume the
default reassign_volume implementation in Plugin.pm? This would avoid
some fragmentation/duplication. Of course, the plugins that cannot use
the generic implementation need to provide their own or signal that they
do not have the feature.
More comments inline:
Am 02.04.21 um 12:19 schrieb Aaron Lauterer:
Functionality has been added for the following storage types:
* dir based ones
* directory
* NFS
* CIFS
* gluster
What about CephFS? Can it use the generic implementation or not? If it
can and you choose the approach I suggested at the beginning, you don't
even need to add code to it ;)
* ZFS
* (thin) LVM
* Ceph
A new feature `reassign` has been introduced to mark which storage
plugin supports the feature.
Version API and AGE have been bumped.
Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
---
v5 -> v6:
* refactor dir based feature check to reduce code repetition by
introducing the file_can_reassign_volume sub that does the actual check
v4 -> v5:
* rebased on master
* bumped api ver and api age
* rephrased "not implemented" message as suggested [0].
v3 -> v4:
* revert intermediate storage plugin for directory based plugins
* add a `die "not supported"` method in Plugin.pm
* dir based plugins now call the file_reassign_volume method in
Plugin.pm as the generic file/directory based method
* restored old `volume_has_feature` method in Plugin.pm and override it
in directory based plugins to check against the new `reassign` feature
(not too happy about the repetition for each plugin)
v2 -> v3:
* added feature flags instead of dummy "not implemented" methods in
plugins which do not support it as that would break compatibility with
3rd party plugins.
Had to make $features available outside the `has_features` method in
Plugins.pm in order to be able to individually add features in the
`BaseDirPlugin.pm`.
* added the BaseDirPlugin.pm to maintain compat with 3rd party plugins,
this is explained in the commit message
* moved the actual renaming from the `reassign_volume` to a dedicated
`rename_volume` method to make this functionality available to other
possible uses in the future.
* added support for linked clones ($base)
rfc -> v1 -> v2: nothing changed
[0] https://lists.proxmox.com/pipermail/pve-devel/2020-November/046031.html
PVE/Storage.pm | 15 +++++++--
PVE/Storage/CIFSPlugin.pm | 13 ++++++++
PVE/Storage/DirPlugin.pm | 13 ++++++++
PVE/Storage/GlusterfsPlugin.pm | 13 ++++++++
PVE/Storage/LVMPlugin.pm | 24 ++++++++++++++
PVE/Storage/LvmThinPlugin.pm | 1 +
PVE/Storage/NFSPlugin.pm | 13 ++++++++
PVE/Storage/Plugin.pm | 60 ++++++++++++++++++++++++++++++++++
PVE/Storage/RBDPlugin.pm | 31 ++++++++++++++++++
PVE/Storage/ZFSPoolPlugin.pm | 26 +++++++++++++++
10 files changed, 207 insertions(+), 2 deletions(-)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..b950c82 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::DRBDPlugin;
use PVE::Storage::PBSPlugin;
# Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 8;
+use constant APIVER => 9;
# Age is the number of versions we're backward compatible with.
# This is like having 'current=APIVER' and age='APIAGE' in libtool,
# see
https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 7;
+use constant APIAGE => 8;
# load standard plugins
PVE::Storage::DirPlugin->register();
@@ -349,6 +349,7 @@ sub volume_snapshot_needs_fsfreeze {
# snapshot - taking a snapshot is possible
# sparseinit - volume is sparsely initialized
# template - conversion to base image is possible
+# reassign - reassigning disks to other guest is possible
# $snap - check if the feature is supported for a given snapshot
# $running - if the guest owning the volume is running
# $opts - hash with further options:
@@ -1843,6 +1844,16 @@ sub complete_volume {
return $res;
}
+sub reassign_volume {
+ my ($cfg, $volid, $target_vmid) = @_;
+
+ my ($storeid, $volname) = parse_volume_id($volid);
+ my $scfg = storage_config($cfg, $storeid);
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+ return $plugin->reassign_volume($scfg, $storeid, $volname, $target_vmid);
+}
+
# Various io-heavy operations require io/bandwidth limits which can be
# configured on multiple levels: The global defaults in datacenter.cfg, and
# per-storage overrides. When we want to do a restore from storage A to
storage
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index be06cc7..3affa5d 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -293,4 +293,17 @@ sub update_volume_notes {
PVE::Storage::DirPlugin::update_volume_notes($class, @_);
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+ return $class->file_reassign_volume($scfg, $storeid, $volname,
$target_vmid);
+}
+
+sub volume_has_feature {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
+
+ shift @_;
+ return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
+ return $class->SUPER::volume_has_feature(@_);
+}
+
1;
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 2267f11..cfdb575 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -156,4 +156,17 @@ sub check_config {
return $opts;
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+ return $class->file_reassign_volume($scfg, $storeid, $volname,
$target_vmid);
+}
+
+sub volume_has_feature {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
+
+ shift @_;
+ return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
+ return $class->SUPER::volume_has_feature(@_);
+}
+
1;
diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
index 2dd414d..8132a3e 100644
--- a/PVE/Storage/GlusterfsPlugin.pm
+++ b/PVE/Storage/GlusterfsPlugin.pm
@@ -348,4 +348,17 @@ sub check_connection {
return defined($server) ? 1 : 0;
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+ return $class->file_reassign_volume($scfg, $storeid, $volname,
$target_vmid);
+}
+
+sub volume_has_feature {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
+
+ shift @_;
+ return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
+ return $class->SUPER::volume_has_feature(@_);
+}
+
1;
diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
index df49b76..0d9a691 100644
--- a/PVE/Storage/LVMPlugin.pm
+++ b/PVE/Storage/LVMPlugin.pm
@@ -339,6 +339,13 @@ sub lvcreate {
run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
}
+sub lvrename {
+ my ($vg, $oldname, $newname) = @_;
+
+ my $cmd = ['/sbin/lvrename', $vg, $oldname, $newname];
+ run_command($cmd, errmsg => "lvrename '${vg}/${oldname}' to '${newname}'
error");
+}
+
sub alloc_image {
my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
@@ -584,6 +591,7 @@ sub volume_has_feature {
my $features = {
copy => { base => 1, current => 1},
+ reassign => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -684,4 +692,20 @@ sub volume_import_write {
input => '<&'.fileno($input_fh));
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+
+ $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
+ my $target_volname = $class->find_free_diskname($storeid, $scfg,
$target_vmid);
+ return $class->rename_volume($scfg, $storeid, $volname,
$target_volname, $target_vmid);
+ });
+}
+
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid) = @_;
Nit: The signature is different than the corresponding one in Plugin.pm.
I know you're not using $base here, but maybe add it for consistency.
That said, why not assemble the new volume ID in the reassign function
itself and have the rename function only be concerned with renaming?
Then you could drop the $base form the signature altogether.
+
+ lvrename($scfg->{vgname}, $source_volname, $target_volname);
+ return "${storeid}:${target_volname}";
+}
+
1;
diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
index c9e127c..895af8b 100644
--- a/PVE/Storage/LvmThinPlugin.pm
+++ b/PVE/Storage/LvmThinPlugin.pm
@@ -355,6 +355,7 @@ sub volume_has_feature {
template => { current => 1},
copy => { base => 1, current => 1, snap => 1},
sparseinit => { base => 1, current => 1},
+ reassign => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 39bf15a..7217ca2 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -197,4 +197,17 @@ sub update_volume_notes {
PVE::Storage::DirPlugin::update_volume_notes($class, @_);
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+ return $class->file_reassign_volume($scfg, $storeid, $volname,
$target_vmid);
+}
+
+sub volume_has_feature {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
+
+ shift @_;
+ return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
+ return $class->SUPER::volume_has_feature(@_);
+}
+
1;
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d2d8184..c1ea41d 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -927,6 +927,7 @@ sub storage_can_replicate {
return 0;
}
+
Nit: accidental newline
sub volume_has_feature {
my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
@@ -1456,4 +1457,63 @@ sub volume_import_formats {
return ();
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+ die "not implemented in storage plugin '$class'\n";
+}
+
+# general reassignment method for file/directory based storages
+sub file_reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+
+ my $base;
+ my $base_vmid;
+ my $format;
+ my $source_vmid;
+
+ (undef, $volname, $source_vmid, $base, $base_vmid, undef, $format) =
$class->parse_volname($volname);
+
+ # parse_volname strips the directory from volname
+ my $source_volname = "${source_vmid}/${volname}";
+
+ if ($base) {
+ $base = "${base_vmid}/${base}/";
+ } else {
+ $base = '';
+ }
+
+ $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
+ my $target_volname = $class->find_free_diskname($storeid, $scfg,
$target_vmid, $format, 1);
+
+ return $class->rename_volume($scfg, $storeid, $source_volname,
$target_volname, $target_vmid, $base);
+ });
+}
+
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid,
$base) = @_;
+
+ my $basedir = $class->get_subdir($scfg, 'images');
+ my $imagedir = "${basedir}/${vmid}";
+
+ mkpath $imagedir;
+
+ my $old_path = "${basedir}/${source_volname}";
+ my $new_path = "${imagedir}/${target_volname}";
+
+ rename($old_path, $new_path) ||
+ die "rename '$old_path' to '$new_path' failed - $!\n";
+
+ return "${storeid}:${base}${vmid}/${target_volname}";
+}
+
+sub file_can_reassign_volume {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running,
$opts) = @_;
+
+ my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
+ $class->parse_volname($volname);
+
+ return 1 if !$isBase && $format =~ /^raw$|^qcow2$|^vmdk$/;
+ return undef;
+}
+
1;
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index fab6d57..86a9a0d 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -712,6 +712,7 @@ sub volume_has_feature {
template => { current => 1},
copy => { base => 1, current => 1, snap => 1},
sparseinit => { base => 1, current => 1},
+ reassign => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -728,4 +729,34 @@ sub volume_has_feature {
return undef;
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+
+ my $base;;
+ (undef, $volname, undef, $base) = $class->parse_volname($volname);
+
+ if ($base) {
+ $base = $base . '/';
+ } else {
+ $base = '';
+ }
+
+ $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
+ my $target_volname = $class->find_free_diskname($storeid, $scfg,
$target_vmid);
+ return $class->rename_volume($scfg, $storeid, $volname,
$target_volname, $target_vmid, $base);
+ });
+}
+
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid,
$base) = @_;
+
+ my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_volname,
$target_volname);
+
+ run_rbd_command(
+ $cmd,
+ errmsg => "could not rename image '${source_volname}' to
'${target_volname}'",
+ );
+ return "${storeid}:${base}${target_volname}";
+}
+
1;
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index fe65ae4..c63a94b 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -686,6 +686,7 @@ sub volume_has_feature {
copy => { base => 1, current => 1},
sparseinit => { base => 1, current => 1},
replicate => { base => 1, current => 1},
+ reassign => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -788,4 +789,29 @@ sub volume_import_formats {
return $class->volume_export_formats($scfg, $storeid, $volname, undef,
$base_snapshot, $with_snapshots);
}
+sub reassign_volume {
+ my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
+
+ my $base;;
+ (undef, $volname, undef, $base) = $class->parse_volname($volname);
+
+ if ($base) {
+ $base = $base . '/';
+ } else {
+ $base = '';
+ }
+
+ $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
+ my $target_volname = $class->find_free_diskname($storeid, $scfg,
$target_vmid);
+ return $class->rename_volume($scfg, $storeid, $volname,
$target_volname, $target_vmid, $base);
+ });
+}
+
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid,
$base) = @_;
+
+ $class->zfs_request($scfg, 5, 'rename', "$scfg->{pool}/$source_volname",
"$scfg->{pool}/$target_volname");
+ return "${storeid}:${base}${target_volname}";
+}
+
1;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
