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 2803D62E58 for ; Fri, 18 Sep 2020 16:25:17 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 14AA81715B for ; Fri, 18 Sep 2020 16:24:47 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 49E381714F for ; Fri, 18 Sep 2020 16:24:45 +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 130964543E for ; Fri, 18 Sep 2020 16:24:45 +0200 (CEST) To: Proxmox VE development discussion , Aaron Lauterer References: <20200910143242.20898-1-a.lauterer@proxmox.com> <20200910143242.20898-4-a.lauterer@proxmox.com> From: Thomas Lamprecht Message-ID: Date: Fri, 18 Sep 2020 16:24:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Thunderbird/81.0 MIME-Version: 1.0 In-Reply-To: <20200910143242.20898-4-a.lauterer@proxmox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.190 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [zfspoolplugin.pm, nfsplugin.pm, basedirplugin.pm, storage.pm, plugins.pm, rbdplugin.pm, glusterfsplugin.pm, plugin.pm, lvmplugin.pm, dirplugin.pm, cifsplugin.pm, lvmthinplugin.pm] Subject: Re: [pve-devel] [PATCH v3 storage 3/4] 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: Fri, 18 Sep 2020 14:25:17 -0000 On 9/10/20 4:32 PM, Aaron Lauterer wrote: > Functionality has been added for the following storage types: > > * dir based ones > * directory > * NFS > * CIFS > * gluster > * ZFS > * (thin) LVM > * Ceph > > A new feature `reassign` has been introduced to mark which storage > plugin supports the feature. > > A new intermediate class for directory based storages has been > introduced. This was necessary to maintain compatibility with third > party storage plugins and to avoid duplicate code in the dir based > plugins. > > The new `BaseDirPlugin.pm` adds the `reassign` feature flag and > containes the implementation for the reassign functionlity. > > In the future all the directory specific code in Plugin.pm should be > moved to the BaseDirPlugin.pm. But this will most likely break > compatibility with third party plugins and should thus be done with > care. how so? why don't you just add it in plugin.pm with either: * a general directory based implementation, and a no-op/die for the other ones * a dummy "die implement me in subclass" method if above is not possible Then increase the ABI version AND age to allow external plugins to tell if they can or should implement this themself. As is you do not allow any 3rd party plugin to provide their own method for this (they cannot know when it's even used) and those without it get an error as the Storage::reassign_volume one plainly calls into $class->reassign_volume which, as no default module is there, may fail much uglier than a `die "not implemented in class $class\n"` excetpion. > > Signed-off-by: Aaron Lauterer > --- > 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`. no, this is not good and has bad side effects too, FWICT, as it changes a variable for all api calls in a worker, which then is changed for all plugins... NAK. rather overwrite the has feature method, catch the specific case you need (reassign) and pass the other stuff back to the parent (SUPER) module > * 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 > > > PVE/Storage.pm | 10 ++++++ > PVE/Storage/BaseDirPlugin.pm | 61 ++++++++++++++++++++++++++++++++++ > PVE/Storage/CIFSPlugin.pm | 2 +- > PVE/Storage/DirPlugin.pm | 2 +- > PVE/Storage/GlusterfsPlugin.pm | 2 +- > PVE/Storage/LVMPlugin.pm | 24 +++++++++++++ > PVE/Storage/LvmThinPlugin.pm | 1 + > PVE/Storage/Makefile | 1 + > PVE/Storage/NFSPlugin.pm | 2 +- > PVE/Storage/Plugin.pm | 22 ++++++------ > PVE/Storage/RBDPlugin.pm | 31 +++++++++++++++++ > PVE/Storage/ZFSPoolPlugin.pm | 28 ++++++++++++++++ > 12 files changed, 172 insertions(+), 14 deletions(-) > create mode 100644 PVE/Storage/BaseDirPlugin.pm > > diff --git a/PVE/Storage.pm b/PVE/Storage.pm > index 4a60615..919c606 100755 > --- a/PVE/Storage.pm > +++ b/PVE/Storage.pm > @@ -1759,6 +1759,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/BaseDirPlugin.pm b/PVE/Storage/BaseDirPlugin.pm > new file mode 100644 > index 0000000..b7f4ae4 > --- /dev/null > +++ b/PVE/Storage/BaseDirPlugin.pm > @@ -0,0 +1,61 @@ > +package PVE::Storage::BaseDirPlugin; > + > +use strict; > +use warnings; > + > +use File::Path; > +use Data::Dumper; > + > +use base qw(PVE::Storage::Plugin); > + > +$PVE::Storage::Plugin::features->{reassign} = { current => { > + 'raw' => 1, > + 'qcow2'=> 1, > + 'vmdk' => 1, > + } > + }; > + > +sub 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}"; > +} > + > +1; > diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm > index 6edbc9d..d9714dd 100644 > --- a/PVE/Storage/CIFSPlugin.pm > +++ b/PVE/Storage/CIFSPlugin.pm > @@ -9,7 +9,7 @@ use File::Path; > use PVE::Storage::Plugin; > use PVE::JSONSchema qw(get_standard_option); > > -use base qw(PVE::Storage::Plugin); > +use base qw(PVE::Storage::BaseDirPlugin); > > # CIFS helper functions > > diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm > index 3c81d24..66b590e 100644 > --- a/PVE/Storage/DirPlugin.pm > +++ b/PVE/Storage/DirPlugin.pm > @@ -7,7 +7,7 @@ use File::Path; > use PVE::Storage::Plugin; > use PVE::JSONSchema qw(get_standard_option); > > -use base qw(PVE::Storage::Plugin); > +use base qw(PVE::Storage::BaseDirPlugin); > > # Configuration > > diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm > index 2dd414d..14111e6 100644 > --- a/PVE/Storage/GlusterfsPlugin.pm > +++ b/PVE/Storage/GlusterfsPlugin.pm > @@ -10,7 +10,7 @@ use PVE::Network; > use PVE::Storage::Plugin; > use PVE::JSONSchema qw(get_standard_option); > > -use base qw(PVE::Storage::Plugin); > +use base qw(PVE::Storage::BaseDirPlugin); > > # Glusterfs helper functions > > diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm > index c0740d4..4203eda 100644 > --- a/PVE/Storage/LVMPlugin.pm > +++ b/PVE/Storage/LVMPlugin.pm > @@ -337,6 +337,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) = @_; > > @@ -581,6 +588,7 @@ sub volume_has_feature { > > my $features = { > copy => { base => 1, current => 1}, > + reassign => {current => 1}, > }; > > my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) = > @@ -681,4 +689,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) = @_; > + > + 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 d1c5b1f..adc4ae2 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/Makefile b/PVE/Storage/Makefile > index 5b8f6e8..936fbf2 100644 > --- a/PVE/Storage/Makefile > +++ b/PVE/Storage/Makefile > @@ -1,5 +1,6 @@ > SOURCES= \ > Plugin.pm \ > + BaseDirPlugin.pm \ > DirPlugin.pm \ > LVMPlugin.pm \ > NFSPlugin.pm \ > diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm > index 6abb24b..f91decc 100644 > --- a/PVE/Storage/NFSPlugin.pm > +++ b/PVE/Storage/NFSPlugin.pm > @@ -10,7 +10,7 @@ use PVE::ProcFSTools; > use PVE::Storage::Plugin; > use PVE::JSONSchema qw(get_standard_option); > > -use base qw(PVE::Storage::Plugin); > +use base qw(PVE::Storage::BaseDirPlugin); > > # NFS helper functions > > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index e6cd99c..581c581 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -888,19 +888,20 @@ sub storage_can_replicate { > return 0; > } > > +our $features = { > + snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} }, > + clone => { base => {qcow2 => 1, raw => 1, vmdk => 1} }, > + template => { current => {qcow2 => 1, raw => 1, vmdk => 1, subvol => 1} }, > + copy => { base => {qcow2 => 1, raw => 1, vmdk => 1}, > + current => {qcow2 => 1, raw => 1, vmdk => 1}, > + snap => {qcow2 => 1} }, > + sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1}, > + current => {qcow2 => 1, raw => 1, vmdk => 1} }, > +}; > + > sub volume_has_feature { > my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_; > > - my $features = { > - snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} }, > - clone => { base => {qcow2 => 1, raw => 1, vmdk => 1} }, > - template => { current => {qcow2 => 1, raw => 1, vmdk => 1, subvol => 1} }, > - copy => { base => {qcow2 => 1, raw => 1, vmdk => 1}, > - current => {qcow2 => 1, raw => 1, vmdk => 1}, > - snap => {qcow2 => 1} }, > - sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1}, > - current => {qcow2 => 1, raw => 1, vmdk => 1} }, > - }; > > # clone_image creates a qcow2 volume > return 0 if $feature eq 'clone' && > @@ -1411,4 +1412,5 @@ sub volume_import_formats { > return (); > } > > + > 1; > diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm > index 38f2b46..325937a 100644 > --- a/PVE/Storage/RBDPlugin.pm > +++ b/PVE/Storage/RBDPlugin.pm > @@ -703,6 +703,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) = > @@ -719,4 +720,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 10354b3..9f5f055 100644 > --- a/PVE/Storage/ZFSPoolPlugin.pm > +++ b/PVE/Storage/ZFSPoolPlugin.pm > @@ -9,6 +9,8 @@ use PVE::Storage::Plugin; > use PVE::RPCEnvironment; > use Net::IP; > > +use Data::Dumper; > + > use base qw(PVE::Storage::Plugin); > > sub type { > @@ -647,6 +649,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) = > @@ -749,4 +752,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; >