[pve-devel] [RFC 1/2 pve-storage] implement map_volume and unmap_volume

[Thomas Lamprecht]

On 9/21/18 7:58 AM, Dietmar Maurer wrote:
> This allows to request a mapped device/path explicitly,
> regardles of the storage option, eg. krbd option in the RBDplugin.

I like this approach more, less intrusive and more explicit.

I tested a bit around, i.e., create CT, snapshot it, clone from Snapshot,
destroy snapshot, destroy CT.

Single thing which was not working was the a full clone of a linked clone,
i.e., convert a CT to a template, create a linked clone with said template
as base (works) and then clone the linked clone (full clone), which seems
to work first - but then errors out with:

> create full clone of mountpoint rootfs (commonPool:base-105-disk-0/vm-106-disk-0)
> /dev/rbd5
> mke2fs 1.43.4 (31-Jan-2017)
> ...
> Removing image: 100% complete...done.
> TASK ERROR: clone failed: rbd error: rbd: error opening pool 'base-105-disk-0': (2) No such file or directory

(CT105 is the template, CT106 is the linked clone, 

Same test worked on an current KRBD enabled code path and LVM-Thin
(just to ensure that this is something dooable in general), so there
may be something off with passing volume/disk names.

some comments inline.

> 
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>  PVE/Storage.pm           | 24 +++++++++++++++++++
>  PVE/Storage/Plugin.pm    | 12 ++++++++++
>  PVE/Storage/RBDPlugin.pm | 62 ++++++++++++++++++++++++++++--------------------
>  3 files changed, 72 insertions(+), 26 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index f9732fe..9f3bb8e 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -671,6 +671,30 @@ sub vdisk_create_base {
>      });
>  }
>  
> +sub map_volume {
> +    my ($cfg, $volid, $snapname, $nomap) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid);
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    return $plugin->map_volume($storeid, $scfg, $volname, $snapname, $nomap);
> +}
> +
> +sub unmap_volume {
> +    my ($cfg, $volid, $snapname) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid);
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
> +}
> +
>  sub vdisk_alloc {
>      my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
>  
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8ae78e9..7dd6e0a 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -949,6 +949,18 @@ sub deactivate_storage {
>      # do nothing by default
>  }
>  
> +sub map_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $nomap) = @_;
> +
> +    return undef;
> +}
> +
> +sub unmap_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> +
> +    return 1;
> +}
> +
>  sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 41d89b5..e8b15f6 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -74,8 +74,6 @@ my $librados_connect = sub {
>  my $krbd_feature_disable = sub {
>      my ($scfg, $storeid, $name) = @_;
>  
> -    return 1 if !$scfg->{krbd};
> -
>      my ($major, undef, undef, undef) = ceph_version();
>      return 1 if $major < 10;
>  
> @@ -259,7 +257,7 @@ sub properties {
>  	    type => 'string',
>  	},
>  	krbd => {
> -	    description => "Access rbd through krbd kernel module.",
> +	    description => "Always access rbd through krbd kernel module.",
>  	    type => 'boolean',
>  	},
>      };
> @@ -428,8 +426,6 @@ sub clone_image {
>  
>      run_rbd_command($cmd, errmsg => "rbd clone '$basename' error");
>  
> -    &$krbd_feature_disable($scfg, $storeid, $name);
> -
>      return $newvol;
>  }
>  
> @@ -445,8 +441,6 @@ sub alloc_image {
>      my $cmd = &$rbd_cmd($scfg, $storeid, 'create', '--image-format' , 2, '--size', int(($size+1023)/1024), $name);
>      run_rbd_command($cmd, errmsg => "rbd create $name' error");
>  
> -    &$krbd_feature_disable($scfg, $storeid, $name);
> -
>      return $name;
>  }
>  
> @@ -544,39 +538,55 @@ sub deactivate_storage {
>      return 1;
>  }
>  
> -sub activate_volume {
> -    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> -
> -    return 1 if !$scfg->{krbd};
> +sub map_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $nomap) = @_;
>  
>      my ($vtype, $name, $vmid) = $class->parse_volname($volname);
> +    $name .= '@'.$snapname if $snapname;
> +
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
> -    my $path = "/dev/rbd/$pool/$name";
> -    $path .= '@'.$snapname if $snapname;
> -    return if -b $path;
> +    my $kerneldev = "/dev/rbd/$pool/$name";
> +
> +    return $kerneldev if $nomap;
> +
> +    return $kerneldev if -b $kerneldev; # already mapped
> +
> +    &$krbd_feature_disable($scfg, $storeid, $volname); # FIXME: $volname or $name

feature disable/enable could be only done once per image, I mean we
check first so this is not a problem, just could be optimized (like it
was previously)...

>  
> -    $name .= '@'.$snapname if $snapname;
>      my $cmd = &$rbd_cmd($scfg, $storeid, 'map', $name);
> -    run_rbd_command($cmd, errmsg => "can't mount rbd volume $name");
> +    run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
> +
> +    return $kerneldev;
> +}
> +
> +sub unmap_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> +
> +    my $kerneldev = $class->map_volume($storeid, $scfg, $volname, $snapname, 1);

I know you just do this to not duplicate the blockdevice path assembly,
but it feels a bit weird to directly have an map with $nomap method call
in unmap, maybe pull the common parts out in its own ($private) helper sub?

Would also allow us to remove the, somewhat strange, $nomap alltogether.
It feels weird to have a method which does xyz but has a 'don-not-do-xyz'
parameter, e.g., something like:

sub shutdown {
    my ($noshutdown) = @_;
    [...]
}

would also be a bit weird, IMO. (sometimes this may, in fact, be needed,
but this time it does not seems so).

> +
> +    if (-b $kerneldev) {
> +	my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
> +	run_rbd_command($cmd, errmsg => "can't unmap rbd device $kerneldev");
> +    }
>  
>      return 1;
>  }
>  
> -sub deactivate_volume {
> +sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
>      return 1 if !$scfg->{krbd};
>  
> -    my ($vtype, $name, $vmid) = $class->parse_volname($volname);
> -    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
> +    $class->map_volume($storeid, $scfg, $volname, $snapname);
>  
> -    my $path = "/dev/rbd/$pool/$name";
> -    $path .= '@'.$snapname if $snapname;
> -    return if ! -b $path;
> +    return 1;
> +}
> +
> +sub deactivate_volume {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
> -    my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $path);
> -    run_rbd_command($cmd, errmsg => "can't unmap rbd volume $name");
> +    $class->unmap_volume($storeid, $scfg, $volname, $snapname);
>  
>      return 1;
>  }
> @@ -592,7 +602,7 @@ sub volume_size_info {
>  sub volume_resize {
>      my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
>  
> -    return 1 if $running && !$scfg->{krbd};
> +    return 1 if $running && !$scfg->{krbd}; # FIXME???
>  
>      my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>  
> @@ -623,7 +633,7 @@ sub volume_snapshot_rollback {
>  sub volume_snapshot_delete {
>      my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
>  
> -    return 1 if $running && !$scfg->{krbd};
> +    return 1 if $running && !$scfg->{krbd}; # FIXME: ????
>  
>      $class->deactivate_volume($storeid, $scfg, $volname, $snap, {});
>  
> 

those last two FIXME hunks do not belong here, the (possible) issue
with having $running here is another topic.