[pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Max Carrara]

On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
> Overall thoughts - this is a bit much at once...
>
> 1) It doesn't build. The loader fails, claiming that `DirPlugin` is not
> derived from `PVE::Storage::Plugin`. Presumably because you only
> `require` it which does not set up the `@ISA`...
> (Followed by a bunch of "Plugin 'foo' is already loaded" errors)

Huh, really? That's interesting. I must've missed something then. Sorry
for the inconvenience!

>
> 2) I don't think we really need/want to rework the loading this much.
> We don't really gain much.

I perfectly understand; that's why I had sent this out as an RFC first
to gather some feedback -- had this lying around for a bit now and
didn't want to overcook.

>
> Using a plugin *list* instead of manually having a `use AllPlugins;`
> and `AllPlugins->register()` blocks might be worth it, but this is too
> much.
>
> But we definitely don't need a `wantarray`-using `sub` returning a list
> of standard plugins.
> Just use `my/our @PLUGINS = qw(The List);` please.
>
> 3) Apparently our style guide needs updating. While some things aren't
> explicit in there, they should be:
> Eg. you have a *lot* of postfix-`if` on multi-line expressions which
> should rather be regular prefix-if blocks.
>
> Also: `pveStorageDeprecateAt` - our style guide says `snake_case` for
> everything other than package names.

Ah, gotcha! Wasn't sure what to use here since it was an attr handler,
but thanks for the clarification.

>
> 4) You POD-document private `my sub`s. This does not fit our current
> code and I'm not convinced it is really worth it, but then there's a
> lot of code there I think could just be dropped (eg. the
> `get_standard_plugin_names()` sub is documented with POD while it
> should just be a list, while right above it there's a
> `$plugin_register_state` with not even a comment explaining its
> contents.
>
> 5) Generally: Please stop using `wantarray` where it has no actual
> benefit. A private `my sub` used exactly once really does not need
> this.
>

Regarding `wantarray`: Yeah we talked about this off-list wrt. the
PVE::Path series IIRC; the patches here are a bit older, so I hadn't
gotten rid of that pattern here yet.

>
> On Thu, Jan 30, 2025 at 03:51:18PM +0100, Max Carrara wrote:
> > RFC: Tighter API Control for Storage Plugins - v1
> > =================================================
> > 
> > Since this has been cooking for a while I've decided to send this in as
> > an RFC in order to get some early feedback in.
> > 
> > Note that this is quite experimental and also a little more complex;
> > I'll try my best to explain my reasoning for the changes in this RFC
> > below.
> > 
> > Any feedback on this is greatly appreciated!
> > 
> > Introduction
> > ------------
> > 
> > While working on a refactor of the Storage Plugin API, I've often hit
> > cases where I needed to move a subroutine around, but couldn't
> > confidently do so due to the code being rather old and the nature of
> > Perl as a language.
> > 
> > I had instances where things would spontaneously break here and there
> > after moving an inocuous-looking helper subroutine, due to it actually
> > being used in a different module / plugin, or worse, in certain cases in
> > a completely different package, because a plugin's helper sub ended
> > up being spontaneously used there.
> > 
> > To remedy this, I had originally added a helper subroutine for emitting
> > deprecation warnings. The plan back then was to leave the subroutines at
> > their original locations and have them call their replacement under the
> > hood while emitting a warning. Kind of like so:
> > 
> >   # PVE::Storage::SomePlugin
> > 
> >   sub some_helper_sub {
> >       my ($arg) = @_;
> > 
> >       # Emit deprecation warning here to hopefully catch any unexpected
> >       # callsites
> >       warn get_deprecation_warning(
> >           "PVE::Storage::Common::some_helper_sub"
> >       );
> > 
> >       # Call relocated helper here
> >       return PVE::Storage::Common::some_helper_sub($arg);
> >   }
> > 
> > This approach was decent-ish, but didn't *actually* guard against any
> > mishaps, unfortunately. With this approach, there is no guarantee that
> > the callsite will actually be caught, especially if e.g. a third-party
> > storage plugin was also using that subroutine and the plugin author
> > didn't bother checking or reading their CI logs.
> > 
> > What I instead wanted to have was some "strong guarantee" that a
> > subroutine absolutely cannot be used anymore after a certain point, e.g.
> > after a certain API version or similar.
>
> I don't think accidentally-public private helpers should be considered
> part of the API. We can just deprecate them immediately, remove them
> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
> all.

Hmm, fair. I wasn't sure what our stance on that exactly is, so I
dediced to be conservative here; as in: "If it's being used by someone
else, then it's already part of an API", if that makes sense.

Though, since we're fine with removing them, I'll just migrate them soon
and provide wrappers that emit a warning (or something) in case any
third-party modules are still using them. Once we do a major / minor
bump of PVE, we can remove the wrappers while not touching the storage
API{VER,AGE} (at least not for those helpers specifically).

>
> They may be using a private sub in `PVE::QemuServer` too - an attribute
> to warn on that (eg. just check `caller` against `__PACKAGE__`) should
> be "good enough" - a compile time solution would be way too much code.
> We don't need to write our own linters here.

Perhaps not our own linters, I agree, but IMO we should have *something*
that actually allows us to be a little bit more confident about
refactoring / migrating / tearing out Perl code.

I like what you mentioned in your second response to patch 3 [resp]
(and I also agree with your and Thomas's points there). I think what we
could have is an environment variable that perhaps doesn't (just) emit
warnings for deprecated things, but also logs them *in some other place*
so that we may continuously check whether deprecated things are still in
use somewhere.

For example, when running our services with an env var like
PVE_DEBUG_CHECK_DEPRECATIONS=1 (or whatever), we could log any usages of
deprecated stuff into some file like e.g. /var/log/pve/debug/deprecations.log.
Once we have better automated testing in place (e2e-testing etc.), we
could periodically probe that file and let it tell us where we're still
using deprecated subs etc.

Ideally, we could log those things as some kind of parsable format,
almost like some kind of runtime metric collection.

I know this sounds complicated and perhaps like it's "too much", but the
reason why I'm im advocating for *some* kind of mechanism like that is
because grepping through all of our repositories is sometimes just not
enough; it even bit me once when I was trying to remove an
inocuous-looking subroutine from our code [sub-removal], which didn't
show up anywhere when I had originally written that patch series.

Maybe to elaborate some more here: As you know, refactoring something in
Rust is quite easy (even fun) since the compiler and clippy just yell at
you in 99.999% of cases if you mess up. I tried to get Perl to do the
same thing here for me, but I agree with you that it's just too much
work and code for that to actually be effective (and tbf, I see now that
I should probably not try to make Perl act like Rust any further,
because it just won't ever do that equivalently anyways). Hence, do
those things at runtime instead and let the system tell us when we're
running debug builds.

It otherwise becomes very, very hard to make any changes *confidently*
if *somewhere* *something* could magically break because of some spooky
action at a distance.

So, I'll see if I can cook something up and toss you a couple more ideas
here and there off-list if you'd like; I agree with the points you made
and I think we should follow through on the idea of having a general
deprecation mechanism.

One last thing: I like the env var idea regarding plugin API checks [resp]
as well; I think I might work on that as some kind of standalone
feature.

Also, thanks for the thorough feedback! :) I know this is a lot, but
it's appreciated.

[resp]: https://lore.proxmox.com/pve-devel/cjiyfn6mrfhmxoxqoosyyh2csczf6jrc4uvuvyqmx75x56dus3@k6mvylmwd5pc/
[sub-removal]: https://git.proxmox.com/?p=pve-common.git;a=commit;h=d22ff1b644cc2ceb6d7ab98f232eab453a708ddf