[pve-devel] [PATCH] Add Multipath direct storage plugin and enable it.

[Wolfgang Bumiller]

Before going over the code I want to bring to your attention this code
snippet from the iSCSI (non-direct) plugin:

|	    #check multipath           
|	    if (-d "/sys/block/$bdev/holders") { 
|		my $multipathdev = dir_glob_regex("/sys/block/$bdev/holders", '[A-Za-z]\S*');
|		$bdev = $multipathdev if $multipathdev;
|	    }

The iSCSI plugin *should* notice multipath devices and make use of them
if I'm not mistaken here.
I wonder if it might be better to do the same in the iSCSI-direct
plugin?

Because currently there's one major problem with your current code: it
seems to include more than just multipath devices, iow. all disks on my
machine here.

I wonder if this is maybe a too general approach to what you're really
after?

A few inline comments follow:

On Wed, Jun 29, 2016 at 02:00:42PM +0300, Dmitry Petuhov wrote:
> Signed-off-by: Dmitry Petuhov <mityapetuhov at gmail.com>
> ---
>  PVE/Storage.pm                |   2 +
>  PVE/Storage/MPDirectPlugin.pm | 208 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 210 insertions(+)
>  create mode 100644 PVE/Storage/MPDirectPlugin.pm
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 011c4f3..ddf9ffe 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -32,6 +32,7 @@ use PVE::Storage::GlusterfsPlugin;
>  use PVE::Storage::ZFSPoolPlugin;
>  use PVE::Storage::ZFSPlugin;
>  use PVE::Storage::DRBDPlugin;
> +use PVE::Storage::MPDirectPlugin;
>  
>  # load and initialize all plugins
>  PVE::Storage::DirPlugin->register();
> @@ -46,6 +47,7 @@ PVE::Storage::GlusterfsPlugin->register();
>  PVE::Storage::ZFSPoolPlugin->register();
>  PVE::Storage::ZFSPlugin->register();
>  PVE::Storage::DRBDPlugin->register();
> +PVE::Storage::MPDirectPlugin->register();
>  PVE::Storage::Plugin->init();
>  
>  my $UDEVADM = '/sbin/udevadm';
> diff --git a/PVE/Storage/MPDirectPlugin.pm b/PVE/Storage/MPDirectPlugin.pm
> new file mode 100644
> index 0000000..76ced94
> --- /dev/null
> +++ b/PVE/Storage/MPDirectPlugin.pm
> @@ -0,0 +1,208 @@
> +package PVE::Storage::MPDirectPlugin;
> +
> +use strict;
> +use warnings;
> +use Data::Dumper;
> +use IO::File;
> +use PVE::Tools qw(run_command trim);
> +use PVE::Storage::Plugin;
> +use PVE::JSONSchema qw(get_standard_option);
> +
> +use base qw(PVE::Storage::Plugin);
> +
> +# Configuration
> +
> +sub type {
> +    return 'mpdirect';
> +}
> +
> +sub plugindata {
> +    return {
> +	content => [ {images => 1, rootdir => 1, none => 1}, { images => 1 }],
> +    };
> +}
> +
> +sub properties {
> +    return {
> +    };
> +}
> +
> +sub options {
> +    return {
> +	content => { optional => 1 },
> +    };
> +}
> +
> +# Storage implementation
> +
> +sub parse_volname {
> +    my ($class, $volname) = @_;
> +
> +    return ('images', $volname, undef, undef, undef, undef, 'raw');
> +}
> +
> +sub filesystem_path {
> +    my ($class, $scfg, $volname, $snapname) = @_;
> +
> +    die "Direct attached device snapshot is not implemented" if defined($snapname);
> +
> +    my ($vtype, $name, $vmid) = $class->parse_volname($volname);
> +
> +    my $path = "/dev/disk/by-id/wwn-0x$name";
> +
> +    return wantarray ? ($path, $vmid, $vtype) : $path;
> +}
> +
> +sub create_base {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    die "Cannot create physical device";
> +}
> +
> +sub clone_image {
> +    my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
> +
> +    die "Cannot create physical device";
> +}
> +
> +sub alloc_image {
> +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> +
> +    die "Cannot create physical device";
> +}
> +
> +sub free_image {
> +    my ($class, $storeid, $scfg, $volname, $isBase) = @_;
> +
> +    die "Cannot create physical device";
> +}
> +
> +sub list_images {
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +
> +    my $vgname = $scfg->{vgname};
> +
> +    my $res = [];
> +
> +    opendir my $dir, '/dev/disk/by-id' || die "Cannot list disks";
> +
> +    while (my $disk = readdir $dir) {
> +	# We don't want partitions here
> +	next if $disk =~ m/-part\d+$/;
> +
> +	next if $disk !~ m/^wwn-0x(.+)$/;
> +	my $wwid = $1;

This includes all disks, not just multipath devices.

# multipath -ll
# ls -l /dev/disk/by-id |grep wwn- |grep -v -- -part
lrwxrwxrwx 1 root root  9 Jun 29 09:09 wwn-0x5000c50064bbed7d -> ../../sdd
lrwxrwxrwx 1 root root  9 Jun 29 09:09 wwn-0x5002538d409de6ec -> ../../sdb
lrwxrwxrwx 1 root root  9 Jun 29 09:09 wwn-0x500a07510e6af4b5 -> ../../sdc
lrwxrwxrwx 1 root root  9 Jun 29 09:09 wwn-0x55cd2e404c608ca2 -> ../../sda
lrwxrwxrwx 1 root root  9 Jun 29 09:11 wwn-0x60014059c03c50c2e8b4d99972140221 -> ../../sdf

> +
> +	my $volid = "$storeid:$wwid";
> +
> +	if ($vollist) {
> +	    my $found = grep { $_ eq $volid } @$vollist;
> +	    next if !$found;
> +	} else {
> +	    next if defined($vmid);
> +	}
> +
> +	my $actualdev = readlink "/dev/disk/by-id/$disk";
> +	$actualdev =~ s/..\/..\///;

would be easier to read with a different regex symbol, eg: s at ../../@@;

> +
> +	my $size;
> +	if (open SZ, "/sys/block/$actualdev/size" ) {

We have a helper for this:
  file_read_firstline("/sys/block/$actualdev/size");

For future reference: for new code our prefered style there should use
variables and include the open-mode:
  open my $sz, '<', "/sys/block/$actualdev/size"

> +	    $size = <SZ>;
> +	    $size *= 512;
> +	    close SZ;
> +	} else {
> +	    $size = undef;
> +	}
> +
> +	push @$res, {
> +	    volid => $volid, format => 'raw', size => $size,
> +	};
> +    }
> +    closedir $dir;
> +
> +    return $res;
> +}
> +
> +sub status {
> +    my ($class, $storeid, $scfg, $cache) = @_;
> +
> +    # We cannot know status of undefined set of devices
> +    return undef;
> +}
> +
> +sub activate_storage {
> +    my ($class, $storeid, $scfg, $cache) = @_;
> +
> +    # Server's SCSI subsystem is always up, so there's nothing to do
> +    # Maybe we could control selected controllers/ports in future.
> +    return 1;
> +}
> +
> +sub deactivate_storage {
> +    my ($class, $storeid, $scfg, $cache) = @_;
> +
> +    return 1;
> +}
> +
> +sub activate_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> +
> +    die "volume snapshot is not possible on direct-attached storage device" if $snapname;
> +
> +    return 1;
> +}
> +
> +sub deactivate_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> +
> +    die "volume snapshot is not possible on direct-attached storage  device" if $snapname;
> +
> +    return 1;
> +}
> +
> +sub volume_resize {
> +    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
> +
> +    die "Direct-attached storage device cannot be resized";
> +}
> +
> +sub volume_snapshot {
> +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> +
> +    die "Direct-attached storage snapshot is not implemented";
> +}
> +
> +sub volume_snapshot_rollback {
> +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> +
> +    die "Direct-attached storage snapshot rollback is not implemented";
> +}
> +
> +sub volume_snapshot_delete {
> +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> +
> +    die "Direct-attached storage snapshot delete is not implemented";
> +}
> +
> +sub volume_has_feature {
> +    my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
> +
> +    my $features = {
> +	copy => { base => 1, current => 1},
> +    };
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> +	$class->parse_volname($volname);
> +
> +    my $key = undef;
> +    if($snapname){
> +	$key = 'snap';
> +    }else{
> +	$key =  $isBase ? 'base' : 'current';
> +    }
> +    return 1 if $features->{$feature}->{$key};
> +
> +    return undef;
> +}
> +
> +1;
> -- 
> 2.1.4