[pve-devel] [PATCH v2] Add support for custom storage plugins

[Wolfgang Bumiller]

There are a few issues we need to fix before applying this, comments are
inline.

And there's one more thing since this AFAIK will be your first
contribution: As stated in our developer documentation we require a CLA
in order to be able to include contributions in our code. We use the
Harmony CLA, a community-centered agreement for open, source projects.
See: http://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

Comments inline.

On Sun, Aug 14, 2016 at 11:30:05AM +0300, Dmitry Petuhov wrote:
> PVE team cannot support specialized vendor-specific storage
> plugins because of lack of hardware. But we can allow users to
> add own plugins for their storages without need to rewrite any
> PVE code and thus ease PVE updates to them.
> 
> Idea of this patch is to add folder /usr/share/perl5/PVE/Storage/Custom
> where user can place his plugins and PVE will automatically load
> them on start or warn if it could not and continue. Maybe we could
> even load all plugins (except PVE::Storage::Plugin itself) this way,
> because current storage plugins are not really plugins, if they
> need to be explicitly loaded in PVE code :-).
> 
> This approach works (with some limitations) if plugin supports
> generic PVE way: full control of volumes lifecycle. And will not
> currently work for custom plugins like iSCSI, which needs to select
> pre-existing volumes. Maybe someone will add more flexible way to
> pve-manager to select input elements for storage plugins to target
> this.
> 
> Currently tested with my NetApp plugin.
> 
> New in v2: add API version check.
> 
> Custom plugins MUST have api() method returning version for which
> it was designed. If API changes from PVE side, module is just not
> being registered, so user have to update module. Until module update,
> corresponding storage will just disappear from PVE, so it shall
> not impose any data damage because of API change.
> 
> Signed-off-by: Dmitry Petuhov <mityapetuhov at gmail.com>
> ---
>  PVE/Storage.pm | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> old mode 100755
> new mode 100644
> index 25ff545..6e2a769
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1,5 +1,8 @@
>  package PVE::Storage;
>  
> +# Storage API version. Icrement it on incompatible changes in storage API interface.
> +use constant APIVER => 1;
> +

We usually put our constants after the imports. This would be a weird
break of style.

>  use strict;
>  use warnings;
>  use Data::Dumper;
> @@ -12,7 +15,7 @@ use File::Path;
>  use Cwd 'abs_path';
>  use Socket;
>  
> -use PVE::Tools qw(run_command file_read_firstline $IPV6RE);
> +use PVE::Tools qw(run_command file_read_firstline dir_glob_foreach $IPV6RE);
>  use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::JSONSchema;
> @@ -33,6 +36,25 @@ use PVE::Storage::ZFSPoolPlugin;
>  use PVE::Storage::ZFSPlugin;
>  use PVE::Storage::DRBDPlugin;
>  
> +if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
> +    dir_glob_foreach('/usr/share/perl5/PVE/Storage/Custom', '.*Plugin\.pm', sub {

I'd rather have this moved to after the *->register() block of the
standard plugins below.

> +	my ($file) = @_;
> +	my $modname = 'PVE::Storage::Custom::' . $file;
> +	$modname =~ s!.pm$!!;

Should be \. - although not really an issue at this point as it's
already filtered with /\.pm/ by dir_glob_foreach().

> +	$file = 'PVE/Storage/Custom/' . $file;
> +
> +	eval {
> +	    require $file;
> +	};
> +	if ($@) {
> +	    warn $@;
> +	} elsif ($modname->api() == APIVER) { # Check storage API version.

This errors out entirely if the module does not define an api()
subroutine. $modname->can() can be used to catch this case.
I think we should check both: $modname->isa('PVE::Storage::Plugin') and
$modname->can('api') before using it and provide approperate warnings
for each case.

Also currently if the API versions mismatch there's no warning.
Another else branch with a warning would be good, otherwise plugins with
unsupported api versions silently disappear after an upgrade. Better
give them a warning so they can easily see what's wrong when looking
through the syslog.

> +	    import $file;
> +	    $modname->register();
> +	}
> +    });
> +}
> +
>  # load and initialize all plugins
>  PVE::Storage::DirPlugin->register();
>  PVE::Storage::LVMPlugin->register();
> -- 
> 2.1.4