[PATCH storage] fix #8076: zfs: activate volume: check for existence to fix regression with cloud-init disk

[PATCH storage] fix #8076: zfs: activate volume: check for existence to fix regression with cloud-init disk

[Fiona Ebner]

Explicitly checking for existence during activation has the advantage
to catch issues earlier with a clearer error message. Note that the
implementation in Plugin.pm already explicitly checks for existence,
so this also aligns the implementation for ZFS closer to that.

This also fixes #8076: For a zvol (i.e. 'raw' format),
zfs_wait_for_zvol_link() was called without first checking if the
volume actually exists. This means that for non-existent volumes,
activation would only fail after the timeout of 300 seconds in worker
context.

This change also means that non-existent snapshots are caught during
activation already, since previously, activate_volume() would return
early without doing anything for snapshots.

The error message "dataset does not exist" is matched against, because
the exit status from 'zfs' will always be 1 if an error occured; from
the man page:
> The zfs utility exits 0 on success, 1 if an error occurs, and 2 if
> invalid command line options were specified.
Note that zfs_delete_zvol() already does the same.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/ZFSPoolPlugin.pm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 61c49d9..023bd21 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -288,6 +288,21 @@ sub zfs_request {
     return $msg;
 }
 
+my sub zfs_volume_exists {
+    my ($class, $scfg, $volname, $snapname) = @_;
+
+    my $name = ($class->parse_volname($volname))[1];
+    my $dataset = "$scfg->{pool}/${name}";
+    $dataset .= "\@${snapname}" if defined($snapname);
+
+    eval { $class->zfs_request($scfg, 10, 'list', '-o', 'name', '-Hp', $dataset); };
+    if (my $err = $@) {
+        return 0 if $err =~ m/dataset does not exist/;
+        die $err;
+    }
+    return 1;
+}
+
 sub zfs_wait_for_zvol_link {
     my ($class, $scfg, $volname, $timeout) = @_;
 
@@ -739,6 +754,12 @@ sub deactivate_storage {
 sub activate_volume {
     my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
 
+    if (!zfs_volume_exists($class, $scfg, $volname, $snapname)) {
+        my $name = "$volname";
+        $name .= "\@${snapname}" if defined($snapname);
+        die "volume ${name} does not exist on storage ${storeid}\n";
+    }
+
     return 1 if defined($snapname);
 
     my (undef, $dataset, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
-- 
2.47.3

Re: [PATCH storage] fix #8076: zfs: activate volume: check for existence to fix regression with cloud-init disk

[Fiona Ebner]

Ping

Am 07.10.26 um 1:49 PM schrieb Fiona Ebner:
> Explicitly checking for existence during activation has the advantage
> to catch issues earlier with a clearer error message. Note that the
> implementation in Plugin.pm already explicitly checks for existence,
> so this also aligns the implementation for ZFS closer to that.
> 
> This also fixes #8076: For a zvol (i.e. 'raw' format),
> zfs_wait_for_zvol_link() was called without first checking if the
> volume actually exists. This means that for non-existent volumes,
> activation would only fail after the timeout of 300 seconds in worker
> context.
> 
> This change also means that non-existent snapshots are caught during
> activation already, since previously, activate_volume() would return
> early without doing anything for snapshots.
> 
> The error message "dataset does not exist" is matched against, because
> the exit status from 'zfs' will always be 1 if an error occured; from
> the man page:
>> The zfs utility exits 0 on success, 1 if an error occurs, and 2 if
>> invalid command line options were specified.
> Note that zfs_delete_zvol() already does the same.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/Storage/ZFSPoolPlugin.pm | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
> index 61c49d9..023bd21 100644
> --- a/src/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/src/PVE/Storage/ZFSPoolPlugin.pm
> @@ -288,6 +288,21 @@ sub zfs_request {
>      return $msg;
>  }
>  
> +my sub zfs_volume_exists {
> +    my ($class, $scfg, $volname, $snapname) = @_;
> +
> +    my $name = ($class->parse_volname($volname))[1];
> +    my $dataset = "$scfg->{pool}/${name}";
> +    $dataset .= "\@${snapname}" if defined($snapname);
> +
> +    eval { $class->zfs_request($scfg, 10, 'list', '-o', 'name', '-Hp', $dataset); };
> +    if (my $err = $@) {
> +        return 0 if $err =~ m/dataset does not exist/;
> +        die $err;
> +    }
> +    return 1;
> +}
> +
>  sub zfs_wait_for_zvol_link {
>      my ($class, $scfg, $volname, $timeout) = @_;
>  
> @@ -739,6 +754,12 @@ sub deactivate_storage {
>  sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
> +    if (!zfs_volume_exists($class, $scfg, $volname, $snapname)) {
> +        my $name = "$volname";
> +        $name .= "\@${snapname}" if defined($snapname);
> +        die "volume ${name} does not exist on storage ${storeid}\n";
> +    }
> +
>      return 1 if defined($snapname);
>  
>      my (undef, $dataset, undef, undef, undef, undef, $format) = $class->parse_volname($volname);