[pve-devel] [RFC v1 pve-storage 1/6] version: introduce PVE::Storage::Version

[Wolfgang Bumiller]

On Thu, Jan 30, 2025 at 03:51:19PM +0100, Max Carrara wrote:
> The purpose of this module is to separate the handling of the Storage
> API's version from the remaining code while also providing additional
> mechanisms to handle changes to the Storage API more gracefully.
> 
> The first such mechanism is the `pveStorageDeprecateAt` attribute,
> which, when added to a subroutine, marks the subroutine as deprecated
> at a specific version. Should the lowest supported API version
> increase beyond the version passed to the attribute, the module will
> fail to compile (during Perl's CHECK phase).
> 
> This provides a more robust mechanism instead of having to rely on
> "FIXME" comments or similar.
> 
> Unfortunately attributes can't conveniently be exported using the
> `Exporter` module or similar mechanisms, and they can neither be
> referenced via their fully qualified path. The attribute's name is
> therefore intentionally made long in order to not conflict with any
> other possible attributes in the UNIVERSAL namespace.
> 
> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
> ---
>  src/PVE/Storage.pm         |  10 +--
>  src/PVE/Storage/Makefile   |   1 +
>  src/PVE/Storage/Version.pm | 180 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 185 insertions(+), 6 deletions(-)
>  create mode 100644 src/PVE/Storage/Version.pm
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 3b4f041..df4d62f 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -24,6 +24,8 @@ use PVE::RPCEnvironment;
>  use PVE::SSHInfo;
>  use PVE::RESTEnvironment qw(log_warn);
>  
> +use PVE::Storage::Version;
> +
>  use PVE::Storage::Plugin;
>  use PVE::Storage::DirPlugin;
>  use PVE::Storage::LVMPlugin;
> @@ -41,12 +43,8 @@ use PVE::Storage::PBSPlugin;
>  use PVE::Storage::BTRFSPlugin;
>  use PVE::Storage::ESXiPlugin;
>  
> -# Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> -# Age is the number of versions we're backward compatible with.
> -# This is like having 'current=APIVER' and age='APIAGE' in libtool,
> -# see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 1;
> +use constant APIVER => PVE::Storage::Version::APIVER;
> +use constant APIAGE => PVE::Storage::Version::APIAGE;
>  
>  our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
>  
> diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
> index ce3fd68..5315f69 100644
> --- a/src/PVE/Storage/Makefile
> +++ b/src/PVE/Storage/Makefile
> @@ -1,5 +1,6 @@
>  SOURCES= \
>  	Common.pm \
> +	Version.pm \
>  	Plugin.pm \
>  	DirPlugin.pm \
>  	LVMPlugin.pm \
> diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
> new file mode 100644
> index 0000000..a9216c9
> --- /dev/null
> +++ b/src/PVE/Storage/Version.pm
> @@ -0,0 +1,180 @@
> +=head1 NAME
> +
> +C<PVE::Storage::Version> - Storage API Version Management
> +
> +=head1 DESCRIPTION
> +
> +This module is concerned with tracking and managing the version of the Storage
> +API interface C<L<PVE::Storage::Plugin>>. Furthermore, this module also provides
> +a mechanism to I<deprecate> subroutines that are part of the Storage API via a
> +custom I<attribute> called C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>.
> +
> +For more information on this mechanism, see L</"DEPRECATING SUBROUTINES"> below.
> +
> +B<NOTE:> Importing this module will define the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>
> +attribute inside the C<UNIVERSAL> (global) namespace. Ensure that no other
> +symbols have the same name as this attribute in order to avoid conflicts.
> +
> +=cut
> +
> +package PVE::Storage::Version;
> +
> +use strict;
> +use warnings;
> +
> +use v5.36;

^  activates signatures, use them.

> +
> +use Attribute::Handlers;
> +use Carp;
> +use Scalar::Util qw(reftype);
> +
> +# NOTE: This module must not import PVE::Storage or any of its submodules,
> +# either directly or transitively.
> +#
> +# Otherwise there's a risk of introducing nasty cyclical imports and/or causing
> +# obscure errors during compile time, which may be very hard to debug.
> +
> +=head1 CONSTANTS
> +
> +=head3 APIVER
> +
> +The version of the Storage API.
> +
> +This version must be incremented if changes to the C<L<PVE::Storage::Plugin>>
> +API interface are made.
> +
> +=head3 APIAGE
> +
> +The API age is the number of versions we're backward compatible with.
> +
> +This is similar to having C<current='APIVER'> and C<age='APIAGE'> in C<libtool>.
> +For more information, see L<here|https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html>.
> +
> +Generally, you'll want to increment this when incrementing C<L<< APIVER|/APIVER >>>.
> +Resetting C<APIAGE> to C<0> means that backward compatibility is broken and should
> +only be done when appropriate (e.g. during a release of a new major version of PVE).
> +
> +=cut
> +
> +use constant APIVER => 10;
> +use constant APIAGE => 1;
> +
> +my $MIN_VERSION = (APIVER - APIAGE);
> +
> +
> +=head1 DEPRECATING SUBROUTINES
> +
> +Upon importing this module, the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>>
> +attribute becomes available. This attribute ought to be used similar to how
> +prototypes are used in Perl and requires a C<version> and a C<message> key to
> +be passed:
> +
> +    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
> +	# [...]
> +    }
> +
> +Most likely you'll need more space for the C<message>, in which case the
> +attribute may simply span multiple lines:
> +
> +    sub some_sub : pveStorageDeprecateAt(
> +	version => 42,
> +	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
> +	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
> +	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
> +	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
> +	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
> +	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
> +	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
> +	    . " diam venenatis, nec placerat felis viverra.",
> +    ) {
> +	# [...]
> +    }

One example with "..." as message should be good enough.

> +
> +As soon as the passed C<version> is below the I<lowest supported version>
> +(C<L<< APIVER|/APIVER >>> minus C<L<< APIAGE|/APIAGE >>>), the attribute raises
> +an exception during the C<CHECK> phase of the Perl interpreter and also displays
> +the passed C<message>. This happens before any runtime code is executed.
> +
> +Should the passed C<version> be equal to the lowest supported version, a warning
> +containing the passed C<message> is emitted whenever the subroutine is called
> +instead. This ensures that implementors of custom plugins are made aware of
> +subroutines being deprecated, should there be any in use.
> +
> +The C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>> attribute
> +may only be used inside C<L<PVE::Storage>> or its submodules and throws an error
> +if used elsewhere.
> +
> +This mechanism's sole purpose is to enforce deprecation of subroutines during
> +"compile time", forcing the subroutine to be removed once the API version is
> +incremented too far or the API age is reset.
> +
> +=head1 ATTRIBUTES
> +
> +=head3 UNIVERSAL::pveStorageDeprecateAt
> +
> +This attribute marks subroutines as deprecated at a specific C<version> and will
> +emit an error with the given C<message> if the lowest supported Storage API
> +version exceeds the C<version> passed to the attribute.
> +
> +    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
> +	# [...]
> +    }
> +
> +    sub some_sub : pveStorageDeprecateAt(
> +	version => 42,
> +	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
> +	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
> +	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
> +	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
> +	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
> +	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
> +	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
> +	    . " diam venenatis, nec placerat felis viverra.",
> +    ) {
> +	# [...]
> +    }
> +
> +Should the passed C<version> be equal to the currently lowest supported API
> +version, a warning containing the passed C<message> is emitted instead.
> +
> +=cut
> +
> +sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
> +    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
> +
> +    confess "'$attr_name' attribute may only be used inside PVE::Storage and its submodules"
> +	if $package !~ m/^PVE::Storage/;

(Could end in `($|:)`, otherwise this allows `PVE::StorageNotReally`)

> +
> +    my $data = { $attr_data->@* };
> +
> +    my ($version, $message) = ($data->{version}, $data->{message});
> +    my $reftype = reftype($referent);
> +
> +    confess "Referent is not a reference"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !defined($reftype);

^ Style issues mentioned in my cover letter reply.

> +
> +    confess "Unexpected reftype '$reftype'"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !($reftype eq 'CODE' || $reftype eq 'ANON');
> +
> +    confess "Either version or message not defined"
> +	. " -- usage: $attr_name(version => 42, message => \"...\")"
> +	if !defined($version) || !defined($message);
> +
> +    confess "Storage API version '$version' not supported: $message"
> +	if $version < $MIN_VERSION;
> +
> +    no warnings 'redefine';
> +    *$symbol = sub {
> +	my $symbol_name = *{$symbol}{"NAME"};
> +	carp "Warning: subroutine '$symbol_name' is deprecated - $message"
> +	    if $version == $MIN_VERSION;
> +	$referent->(@_);
> +    };
> +
> +    return;
> +}
> +
> +
> +1;
> -- 
> 2.39.5