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 <[email protected]>
---
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 :)
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
