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

[Fiona Ebner]

Am 06.02.25 um 15:39 schrieb Thomas Lamprecht:
> Am 06.02.25 um 15:05 schrieb Fiona Ebner:
>> Am 05.02.25 um 16:20 schrieb Max Carrara:
>>> On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
>>>> 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).
>>
>> I'd also err on the side of caution here. We never explicitly documented
>> what is and isn't part of the plugin API, so chances are that some
>> external plugins do make use of some such helpers. Removing them during
>> a minor release or without APIAGE reset will not be nice to plugin
>> authors. We'll likely do an APIAGE reset for PVE 9 in any case, so we
>> could just do the breaking change for such helpers then too.
> 
> Do we really need to reset the Age? I.e. do we have strong reasons like
> some security ones or big bug (data eating) potential?

There are no such strong reasons, but we didn't have such strong reasons
last time either (IIRC changing snapshot parameter for export for btrfs
or something like that). I thought we need to do that on any breaking
change? We do have a few queued up already for the next reset. Hence my
suggestion to do it for PVE 9 so plugin authors can adapt together with
adapting for the new Debian release.

> 
> As in general I'd try very hard to avoid resetting that, as it causes
> a huge amount of fallout for our users. Refactoring and having a handful
> of methods (which had close to zero maintenance cost) removed is hardly
> a good reason.
> 
> And I'd also try hard to reduce, not increases warnings for regular users,
> which cannot really do anything about them anyway but get annoying and
> scary warnings spammed in their log. Instead, I'd rather make (warnings
> for) those checks opt-in and document how they can be checked for in our
> respective wiki [0], that way the storage plugin vendors that do care
> can actually fix them by moving to the more modern API and potential
> newer features and roll those fixes out without the user seeing any
> impact.

Sure, that's why I'm against doing it during a minor release or without
an APIAGE reset, because that will implicitly break plugins currently
using the methods rather than give the plugin authors an explicit heads-up.

> 
> [0]: https://pve.proxmox.com/wiki/Storage_Plugin_Development
> 
> Tangentially related more/stricter checks at runtime are really bogus,
> we can only lose. If one is really affected by such "magical" perl
> behavior then more test that get run at package assembly (build) time
> and integration tests to cover interaction with downstream packages
> is much more helpful. Avoiding refactoring just for the sake of it might
> also help (yes, rust allows doing that fearlessly, no, that does _not_
> mean it's a sufficient argument for doing so there too).