[pve-devel] [storage] fix #2154: Buggy "pvesm status" output

[Fabian Grünbichler]

On Mon, Apr 01, 2019 at 04:20:46PM +0200, Wolfgang Link wrote:
> Remove "zpool import" because there is no need for it.
> 
> zpool list <poolname> will import the pool if it is not imported.

this is not true? would be pretty crazy anyway ;)

> Anyway also there is no situation where Proxmox VE framework will
> export a pool.

but there might be plenty of systems where admins rely on PVE importing
the configured pools (e.g., with all the zfs-import-* services
disabled) - we do the same for other storages as well (scanning and
activating LVM, mounting NFS/CIFS exports/shares, ...).

the original bug sounds like either a race or a temporary failure to run
'zpool list'? adding an invalid zfspool entry into storage.cfg only
results in that one storage not being activated, with no effects on the
other valid zfspool entries..

> 
> So there should no need to import a pool in the code logic.
> If an error occurs, the error output will help the admin to fix the problem.
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 4bf6d50..e143c7d 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -173,9 +173,6 @@ sub zfs_request {
>  
>      if ($method eq 'zpool_list') {
>  	push @$cmd, 'zpool', 'list';
> -    } elsif ($method eq 'zpool_import') {
> -	push @$cmd, 'zpool', 'import';
> -	$default_timeout = 15 if $default_timeout < 15;
>      } else {
>  	push @$cmd, 'zfs', $method;
>      }
> @@ -529,13 +526,8 @@ sub activate_storage {
>  	$res = $class->zfs_request($scfg, undef, 'zpool_list', @param);
>      };
>  
> -    if ($@ || !defined($res) || $res !~ $pool) {
> -	eval {
> -	    @param = ('-d', '/dev/disk/by-id/', "$pool");
> -	    $class->zfs_request($scfg, undef, 'zpool_import', @param);
> -	};
> -	die "could not activate storage '$storeid', $@\n" if $@;
> -    }
> +    die "could not activate storage '$storeid', $@\n" if $@;
> +
>      return 1;
>  }
>  
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel