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

[Max Carrara]

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.

This gave me an idea: What if there was a way to check this at
"compile-time" in Perl instead of dynamically emitting warnings and
hoping that somebody eventually sees them?

Instead of just focusing on helper subroutines, I decided to expand on
this idea further and make it so that the storage API can actually
enforce API-conformance at "compile-time".

Right now, when a third-party plugin says it has API level X, we simply
trust that it has API level X. It's up to the plugin author to ensure
that it actually does -- and it's also up to the administrator to test
the plugin and ensure that nothing breaks when it's being used.

This is not a strong guarantee, mainly because a plugin author may e.g.
simply forget to adapt a method in their plugin, the administrator might
install a plugin carelessly, etc. At the same time, we don't have a
strong guarantee that *our* plugins conform to the API as well; in other
words, it's hard to make any bigger changes without being confident that
nothing broke in the process.

Overview of what this RFC Proposes to add
-----------------------------------------

- Patch 01: Separate storage API version handling from main code and
  introduce general subroutine deprecation mechanism

- Patch 02: Rework plugin loading and registration mechanism in order to
  allow adding compile-time checks

- Patch 03: Actually add compile-time check that prevents plugins from
  overriding certain methods (which exactly still needs to be decided)

- Patch 04: Use the new check from patch 03, replacing some FIXME
  comments as example

- Patch 05 & 06: Add convenience checks for SectionConfig conformity;
  these are mostly there to improve the "developer experience" to make
  developing a third-party plugin a little easier for third parties

The exact changes are much better explained in each of the patches'
commit message. Patches 01 - 04 are where most of the bulk work is being
done; patch 05 & 06 just show how the plugin registration mechanism can
be expanded.

Closing Thoughts
----------------

This still needs quite a bit of polishing and could perhaps use
PVE::Path [3] once it's merged. As I said in the beginning, I wanted to
send this out in order to get some feedback in first before continuing
to iterate on this, so any feedback is highly appreciated!

The other question is whether we actually want something like this; I
personally would be in favour of stricter checks in our Perl code, but
at the same time, I don't want to overengineer things.

The last thing I want to add (should this get greenlighted) is a
repertoire of tests that actually verify whether the API checks work as
intended with third party modules. Since this is quite hard and quite
elaborate to test, I decided to omit that here for now, as it would just
bloat the RFC (and potentially detract from the main idea).

References
----------

[1]: https://lore.proxmox.com/pve-devel/20240717094034.124857-8-m.carrara@proxmox.com/
[2]: https://perldoc.perl.org/Attribute::Handlers
[3]: https://lore.proxmox.com/pve-devel/20250109144818.430185-1-m.carrara@proxmox.com/

Summary of Changes
------------------

Max Carrara (6):
  version: introduce PVE::Storage::Version
  rework plugin loading and registration mechanism
  introduce compile-time checks for prohibited sub overrides
  plugin: replace fixme comments for deprecated methods with attribute
  introduce check for `type` method and duplicate types
  introduce check for duplicate plugin properties

 src/PVE/Storage.pm              | 647 ++++++++++++++++++++++++++++----
 src/PVE/Storage/CIFSPlugin.pm   |   4 -
 src/PVE/Storage/CephFSPlugin.pm |   4 -
 src/PVE/Storage/DirPlugin.pm    |   5 +-
 src/PVE/Storage/Makefile        |   1 +
 src/PVE/Storage/NFSPlugin.pm    |   4 -
 src/PVE/Storage/PBSPlugin.pm    |   4 -
 src/PVE/Storage/Plugin.pm       | 147 +++++++-
 src/PVE/Storage/Version.pm      | 433 +++++++++++++++++++++
 9 files changed, 1146 insertions(+), 103 deletions(-)
 create mode 100644 src/PVE/Storage/Version.pm

-- 
2.39.5