Re: [pve-devel] [PATCH v1 pve-storage 2/8] pluginbase: add high-level plugin API description

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 2, 2025 6:31 pm, Max Carrara wrote:
> On Mon Mar 31, 2025 at 5:13 PM CEST, Fabian Grünbichler wrote:
>> On March 26, 2025 3:20 pm, Max Carrara wrote:
>> > Add a short paragraph in DESCRIPTION serving as an introduction as
>> > well as the GENERAL PARAMETERS and CACHING EXPENSIVE OPERATIONS
>> > sections.
>> > 
>> > These sections are added in order to avoid repeatedly describing the
>> > same parameters as well as to elaborate on / clarify a couple terms,
>> > e.g. what the $cache parameter does or what a volume in our case is.
>> > 
>> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> > ---
>> >  src/PVE/Storage/PluginBase.pm | 77 +++++++++++++++++++++++++++++++++++
>> >  1 file changed, 77 insertions(+)
>> > 
>> > diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
>> > index e56aa72..16977f3 100644
>> > --- a/src/PVE/Storage/PluginBase.pm
>> > +++ b/src/PVE/Storage/PluginBase.pm
>> > @@ -4,6 +4,83 @@ C<PVE::Storage::PluginBase> - Storage Plugin API Interface
>> >  
>> >  =head1 DESCRIPTION
>> >  
>> > +This module documents the public Storage Plugin API of PVE and serves
>> > +as a base for C<L<PVE::Storage::Plugin>>. Plugins must B<always> inherit from
>> > +C<L<PVE::Storage::Plugin>>, as this module is for documentation purposes
>> > +only.
>>
>> does this make sense? if we now provide a clean base for the structure
>> of plugins, why shouldn't plugins be able to use that, but instead have
>> to inherit from PVE::Storage::Plugin which has a lot of extra stuff that
>> makes things messy?
>>
>> granted, switching over to load from PluginBase could be done as a
>> follow up or with 9.0 (or not at all, if there is a rationale)..
>>
>> after this series we have:
>>
>> SectionConfig
>> -> PluginBase (not an actual base plugin w.r.t. SectionConfig, and not
>> something you base plugins on as a result)
>> --> Plugin (a combination of base plugin and base of all our
>> directory-based plugins)
>> ---> other plugins, including third party ones
>>
>> which seems unfortunate, even if the contents of PluginBase are helpful
>> when implementing your own..
>>
>> IMHO this should either be
>>
>> SectionConfig
>> -> PluginBase (actual base plugin, with SectionConfig implementations
>> moved over from current Plugin.pm as needed)
>> --> PluginTemplate (what's currently PluginBase in this series - nicely
>> documented parent of third party plugins, not actually registered, just
>> contains the storage.cfg interface without the low-level SectionConfig
>> things)
>> ---> NewThirdPartyPlugin (clean slate implementation just using the
>> nicely documented interfaces, guaranteed to not use any helpers from
>> Plugin since it's not a (grand)parent)
>> --> Plugin (base of built-in plugins, probably base of existing third
>> party plugins, should ideally have a different name in the future!)
>> ---> DirPlugin
>> ---> ...
>> ---> ExistingThirdPartyPlugin (this might rely on helpers from Plugin,
>> so we can't just rename that one unless we wait for 9.0)
>>
>> or
>>
>> SectionConfig
>> -> PluginBase (actual base plugin + docs of Plugin API)
>> --> Plugin (base of our plugins and existing third party ones, dir-related helpers, ..)
>> ---> other plugins, including third party ones
>> --> NewThirdPartyPlugin (clean slate as above)
> 
> I agree with your point here -- for now, `::PluginBase` should IMO still
> only be about documentation and enumerating what's part of the Plugin
> API, *but* adding more layers inbetween can still be done eventually.
> 
> However, I think we shouldn't have two different parents for internal
> and external plugins, as it would introduce yet another thing we'd have
> to track wrt. APIAGE resets etc. Maybe it's not actually an issue in
> this case, but ...

the API would be defined by the "public"/external/top-level base
obviously, not by our intermediate shared base ;)

> That actually gives me an idea that's similar to your first suggestion:
> 
> As of this series, the hierarchy would be as follows:
> 
> PluginBase
> └── Plugin
>     ├── ExistingThirdPartyPlugin
>     ├── DirPlugin
>     └── ...
> 
> We could keep `PluginBase` as it is, since IMO having the docs and the
> SectionConfig-related stuff in one place is fine, unless we really want
> to keep the docs separate from the rest of the code. (Would seem a bit
> redundant to introduce another inheritance layer in that case, but I
> personally don't mind.)

I don't mind having the SectionConfig and the "skeleton" in one module,
provided they are clearly separated. the SectionConfig methods are
inherited anyway and any third party plugin must uphold the invariant of
not messing with them..

> Then, we eventually introduce `PluginTemplate` on the same layer as
> `Plugin`. `PluginTemplate` would only contain implementations for the
> most basic methods (and not provide defaults for file-based storages).

do you have something in mind for this "Template"? and if those methods
were so generic and basic at the same time, why shouldn't they live in
PluginBase itself? ;) I am not yet convinced an extra layer like this
makes much sense, but I am happy to listen to (concrete) arguments why
it should exist. IMHO the less code inherited by external plugins the
better, else we always have to not touch that code for ages to avoid
breaking them..

> PluginBase
> ├── Plugin
> │   ├── ExistingThirdPartyPlugin
> │   ├── DirPlugin
> │   └── ...
> └── PluginTemplate
> 
> The idea behind this is that we could then "migrate" each plugin and
> base it off `PluginTemplate` instead. Helpers that are shared between
> plugins could go into `PVE::Storage::Common::*` instead of being
> implicitly becoming part of plugins' modules due to inheritance.

the issue with that is that we've now just moved the problem - what are
the stability guarantees for PVE::Storage::Common::* ? are external
plugins allowed/supposed to use them? I do think we want to keep some
amount of "PVE-specific, internal" helper code (whether in the plugins
themselves or in some standalone helper module) that are off-limits for
external plugins, if just to avoid writing us into a corner. the easiest
way to achieve that is to migrate external plugins away from Plugin.pm
(which we can enforce at plugin loading time at some point after a
deprecation period).

obviously for helpers that we deem stable enough moving them to Common
and wrapping them in their old location in Plugin would be a viable
migration strategy.

> PluginBase
> ├── Plugin
> │   ├── ...
> │   └── ExistingThirdPartyPlugin
> └── PluginTemplate
>     ├── ...
>     └── DirPlugin (cleaned up)
> 
> That way we could step by step "disentangle" the existing plugins from
> each other without having to constantly keep the original behaviour(s)
> of `Plugin` in the back of one's head and account for them. Instead,
> each plugin would implement precisely what it itself needs.
> 
> Since both the old `Plugin` and the new `PluginTemplate` share the same
> interface, namely `PluginBase`, we could still support the old `Plugin`
> until everything's been moved over and third-party devs have had enough
> time to adapt their own code, too.

see above - we can do all of that without introducing PluginTemplate as
well, including at some point no longer allowing third party plugins to
re-use Plugin but force them to be based of PluginBase.

> While doing all this, we could also rework parts of the API that didn't
> age that gracefully, perhaps deprecate certain old methods, introduce
> new methods, etc. as we'd have a "clean" break, sotospeak.

this could also be done without moving all plugins over to
PluginTemplate, although it might be a bit more messy/dangerous.

> So, to summarize my idea:
> - Keep `PluginBase` as it is right now, but also include
>   SectionConfig-related code
> - Introduce `PluginTemplate` with minimal implementation later down the
>   line on the same inheritance layer as `Plugin`
> - Slowly migrate our plugins, basing them off of `PluginTemplate` while
>   tossing out old code, making them independent from one another, and
>   collecting shared helpers in `PVE::Storage::Common::*`
> 
> I'd really like to hear your thoughts on this, because I'm not sure if
> this is *actually* feasible or provides any ROI down the line. One
> alternative that I can think of is to just keep the inheritance
> hierarchy as it is (as in this series) and disentangle the plugins as
> they are right now, without changing their parent (so, almost the same
> as your second idea). I did start breaking apart our plugins like that
> last year, but that was too much all at once [1].
> 
> [1]: https://lore.proxmox.com/pve-devel/20240717094034.124857-1-m.carrara@proxmox.com/


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel