From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 24C2573477 for ; Thu, 15 Apr 2021 13:08:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0DC9018C44 for ; Thu, 15 Apr 2021 13:07:54 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 3F0B018C36 for ; Thu, 15 Apr 2021 13:07:52 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 064C145A06 for ; Thu, 15 Apr 2021 13:07:52 +0200 (CEST) To: pve-devel@lists.proxmox.com, a.lauterer@proxmox.com References: <20210402101923.13050-1-a.lauterer@proxmox.com> <20210402101923.13050-2-a.lauterer@proxmox.com> From: Fabian Ebner Message-ID: Date: Thu, 15 Apr 2021 13:07:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210402101923.13050-2-a.lauterer@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.007 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_SHORT 0.001 Use of a URL Shortener for very short URL NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lvmthinplugin.pm, proxmox.com, zfspoolplugin.pm, lvmplugin.pm, plugins.pm, rbdplugin.pm, glusterfsplugin.pm, dirplugin.pm, cifsplugin.pm, storage.pm, nfsplugin.pm, plugin.pm, gnu.org, basedirplugin.pm] Subject: Re: [pve-devel] [PATCH v6 storage 1/1] add disk reassign feature X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Apr 2021 11:08:24 -0000 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 > --- > 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; >