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 C73FE62E77 for ; Fri, 18 Sep 2020 17:07:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B5D1F173CD for ; Fri, 18 Sep 2020 17:07:14 +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 04BAF173BF for ; Fri, 18 Sep 2020 17:07:14 +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 B470445441 for ; Fri, 18 Sep 2020 17:07:13 +0200 (CEST) To: Thomas Lamprecht , Proxmox VE development discussion References: <20200910143242.20898-1-a.lauterer@proxmox.com> <20200910143242.20898-4-a.lauterer@proxmox.com> From: Aaron Lauterer Message-ID: Date: Fri, 18 Sep 2020 17:07:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2 MIME-Version: 1.0 In-Reply-To: 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.014 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. [basedirplugin.pm, plugin.pm, plugins.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 15:07:44 -0000 On 9/18/20 4:24 PM, Thomas Lamprecht wrote: > 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 That was the first approach, but this would mean a die for all other plugins and if 3rd party plugins would not implement it, the directory based code will be used. But we cannot know if if the directory approach is the right one. > * a dummy "die implement me in subclass" method if above is not possible If I do it this way, I cannot put the actual code for dir based storage in the Plugin.pm, thus having an intermediate class for dir based storages would still be beneficial IMHO to avoid code duplication. IIRC we have 4 dir based storages (dir, CIFS, NFS, Gluster). > > Then increase the ABI version AND age to allow external plugins to tell > if they can or should implement this themself. Thanks for the hint. > > 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 Okay, thanks for the feedback :)