Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature

[Aaron Lauterer <a.lauterer@proxmox.com>]

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 <a.lauterer@proxmox.com>
>> ---
>> 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 :)