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

["Max Carrara" <m.carrara@proxmox.com>]

On Thu Apr 3, 2025 at 9:12 AM CEST, Fabian Grünbichler wrote:
> 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 ;)

Fair point :P

>
> > 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..

No that's a good point; this entire idea here could actually also be
done without introducing `PluginTemplate`.

>
> > 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).

Yeah I agree, I'm just not entirely sure yet which *exact* approach here
would be the best.

If you'd like, we can revisit this topic once it becomes relevant; for
now I think we can move forward with the inheritance structure that this
series proposes, that is:

SectionConfig
└── PluginBase
    └── Plugin
        ├── DirPlugin
        ├── ...
        ├── ExistingThirdPartyPlugin
        └── ...

In a future series, I'd then proceed with moving the
SectionConfig-related stuff to PluginBase (and perhaps other
"fundamental" stuff; yet to be decided what). When that happens, I'll
also see what "migration approach" would be the best, what internal /
external helpers to have, stability guarantees, etc.

The only thing I would definitely want to avoid is introducing another
inheritance layer somewhere, as it's already somewhat hard to keep track
of things.

>
> 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.

I agree here as well; that's something I attempted a long while ago.
I'm looking forward to revisiting that 👀

>
> > 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



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