* [PATCH storage 1/3] test: add tests for zpool status parsing of the ZFS disk API
2026-10-03 12:49 [PATCH storage 0/3] api: disks: zfs: fix error counters and in-use spares in pool details Michal Fox
@ 2026-10-03 12:49 ` Michal Fox
2026-10-03 12:49 ` [PATCH storage 2/3] fix #6640, #6938: api: disks: zfs: return exact error counters Michal Fox
2026-10-03 12:49 ` [PATCH storage 3/3] fix #6389: api: disks: zfs: parse the message of hot spares in use Michal Fox
2 siblings, 0 replies; 4+ messages in thread
From: Michal Fox @ 2026-10-03 12:49 UTC (permalink / raw)
To: pve-devel
The detail call of the ZFS disk API parses the output of 'zpool status'
into the vdev tree of a pool. Add a test for it, which mocks the zpool
command and checks the result for a healthy pool with log, cache and
spare devices, so that fixes for the parser can extend it with their
cases.
Signed-off-by: Michal Fox <me@dualfroz.com>
---
The test goes through the API handler, which checks that /sbin/zpool
exists, so it relies on zfsutils-linux from the Build-Depends.
src/test/run_disk_tests.pl | 2 +-
src/test/zfs_pool_detail_test.pm | 165 +++++++++++++++++++++++++++++++
2 files changed, 166 insertions(+), 1 deletion(-)
create mode 100644 src/test/zfs_pool_detail_test.pm
diff --git a/src/test/run_disk_tests.pl b/src/test/run_disk_tests.pl
index 5a6af07..b4fd09b 100755
--- a/src/test/run_disk_tests.pl
+++ b/src/test/run_disk_tests.pl
@@ -6,7 +6,7 @@ use warnings;
use TAP::Harness;
my $harness = TAP::Harness->new({ verbosity => -2 });
-my $res = $harness->runtests("disklist_test.pm");
+my $res = $harness->runtests("disklist_test.pm", "zfs_pool_detail_test.pm");
exit -1 if !$res || $res->{failed} || $res->{parse_errors};
diff --git a/src/test/zfs_pool_detail_test.pm b/src/test/zfs_pool_detail_test.pm
new file mode 100644
index 0000000..a062656
--- /dev/null
+++ b/src/test/zfs_pool_detail_test.pm
@@ -0,0 +1,165 @@
+package PVE::API2::Disks::ZFS::TestPoolDetail;
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::API2::Disks::ZFS;
+use Test::More;
+use Test::MockModule;
+
+my $zpool_status_output;
+
+my $zfs_api_module = Test::MockModule->new('PVE::API2::Disks::ZFS');
+$zfs_api_module->mock(
+ run_command => sub {
+ my ($cmd, %param) = @_;
+
+ my $cmdline = join(' ', @$cmd);
+ die "unexpected run_command call: '$cmdline'\n"
+ if $cmdline ne '/sbin/zpool status -P tank';
+
+ $param{outfunc}->($_) for split(/\n/, $zpool_status_output);
+
+ return 0;
+ },
+);
+
+# each test is comprised of the following keys:
+# description => what the test is about
+# status => output of 'zpool status' for the pool 'tank'
+# expected => pool details as returned by the API
+my $tests = [
+ {
+ description => 'healthy pool with log, cache and spare',
+ status => <<"EOF",
+ pool: tank
+ state: ONLINE
+ scan: scrub repaired 0B in 00:00:01 with 0 errors on Sun Sep 13 00:24:02 2026
+config:
+
+\tNAME STATE READ WRITE CKSUM
+\ttank ONLINE 0 0 0
+\t mirror-0 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_A-part1 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_B-part1 ONLINE 0 0 0
+\tlogs
+\t /dev/disk/by-id/nvme-LOG-part1 ONLINE 0 0 0
+\tcache
+\t /dev/disk/by-id/nvme-CACHE-part1 ONLINE 0 0 0
+\tspares
+\t /dev/disk/by-id/ata-DISK_C-part1 AVAIL
+
+errors: No known data errors
+EOF
+ expected => {
+ name => 'tank',
+ state => 'ONLINE',
+ scan => 'scrub repaired 0B in 00:00:01 with 0 errors on Sun Sep 13 00:24:02 2026',
+ errors => 'No known data errors',
+ leaf => 0,
+ children => [
+ {
+ name => 'tank',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => 'mirror-0',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/ata-DISK_A-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ {
+ name => '/dev/disk/by-id/ata-DISK_B-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ ],
+ },
+ {
+ name => 'logs',
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/nvme-LOG-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ {
+ name => 'cache',
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/nvme-CACHE-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ {
+ name => 'spares',
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/ata-DISK_C-part1',
+ state => 'AVAIL',
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ ],
+ },
+ },
+];
+
+plan tests => scalar @$tests;
+
+for my $test (@$tests) {
+ $zpool_status_output = $test->{status};
+
+ my $pool = eval { PVE::API2::Disks::ZFS->detail({ node => 'localhost', name => 'tank' }) };
+ diag("unexpected error: $@") if $@;
+ is_deeply($pool, $test->{expected}, $test->{description});
+}
+
+done_testing();
+
+1;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH storage 2/3] fix #6640, #6938: api: disks: zfs: return exact error counters
2026-10-03 12:49 [PATCH storage 0/3] api: disks: zfs: fix error counters and in-use spares in pool details Michal Fox
2026-10-03 12:49 ` [PATCH storage 1/3] test: add tests for zpool status parsing of the ZFS disk API Michal Fox
@ 2026-10-03 12:49 ` Michal Fox
2026-10-03 12:49 ` [PATCH storage 3/3] fix #6389: api: disks: zfs: parse the message of hot spares in use Michal Fox
2 siblings, 0 replies; 4+ messages in thread
From: Michal Fox @ 2026-10-03 12:49 UTC (permalink / raw)
To: pve-devel
Without '-p', 'zpool status' shortens the READ, WRITE and CKSUM counters
of a vdev to a value with a unit suffix once they reach 1024, for
example 1130 read errors are shown as '1.10K'. The parser just adds
zero to those values, so the API returned 1.1 read errors in that case
and logged a warning like:
Argument "1.10K" isn't numeric in addition (+)
Request exact values with '-p'. For the non-JSON output of 'zpool
status' it only affects those counters, the sizes in the scan line stay
human readable.
Signed-off-by: Michal Fox <me@dualfroz.com>
---
src/PVE/API2/Disks/ZFS.pm | 2 +-
src/test/zfs_pool_detail_test.pm | 79 +++++++++++++++++++++++++++++++-
2 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/src/PVE/API2/Disks/ZFS.pm b/src/PVE/API2/Disks/ZFS.pm
index 7dae404..b26805a 100644
--- a/src/PVE/API2/Disks/ZFS.pm
+++ b/src/PVE/API2/Disks/ZFS.pm
@@ -217,7 +217,7 @@ __PACKAGE__->register_method({
die "zfsutils-linux not installed\n";
}
- my $cmd = [$ZPOOL, 'status', '-P', $param->{name}];
+ my $cmd = [$ZPOOL, 'status', '-P', '-p', $param->{name}];
my $pool = {
lvl => 0,
diff --git a/src/test/zfs_pool_detail_test.pm b/src/test/zfs_pool_detail_test.pm
index a062656..00d666a 100644
--- a/src/test/zfs_pool_detail_test.pm
+++ b/src/test/zfs_pool_detail_test.pm
@@ -18,7 +18,7 @@ $zfs_api_module->mock(
my $cmdline = join(' ', @$cmd);
die "unexpected run_command call: '$cmdline'\n"
- if $cmdline ne '/sbin/zpool status -P tank';
+ if $cmdline ne '/sbin/zpool status -P -p tank';
$param{outfunc}->($_) for split(/\n/, $zpool_status_output);
@@ -148,6 +148,83 @@ EOF
],
},
},
+ {
+ description => 'error counters above 1023',
+ status => <<"EOF",
+ pool: tank
+ state: ONLINE
+status: One or more devices has experienced an unrecoverable error. An
+\tattempt was made to correct the error. Applications are unaffected.
+action: Determine if the device needs to be replaced, and clear the errors
+\tusing 'zpool clear' or replace the device with 'zpool replace'.
+ see: https://openzfs.github.io/openzfs-docs/msg/ZFS-8000-9P
+ scan: scrub repaired 1.38M in 00:41:15 with 0 errors on Sun Sep 13 01:05:15 2026
+config:
+
+\tNAME STATE READ WRITE CKSUM
+\ttank ONLINE 0 0 0
+\t mirror-0 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_A-part1 ONLINE 1130 0 0
+\t /dev/disk/by-id/ata-DISK_B-part1 ONLINE 0 0 15565
+
+errors: No known data errors
+EOF
+ expected => {
+ name => 'tank',
+ state => 'ONLINE',
+ status => 'One or more devices has experienced an unrecoverable error. An'
+ . ' attempt was made to correct the error. Applications are unaffected.',
+ action => 'Determine if the device needs to be replaced, and clear the errors'
+ . " using 'zpool clear' or replace the device with 'zpool replace'.",
+ see => 'https://openzfs.github.io/openzfs-docs/msg/ZFS-8000-9P',
+ scan =>
+ 'scrub repaired 1.38M in 00:41:15 with 0 errors on Sun Sep 13 01:05:15 2026',
+ errors => 'No known data errors',
+ leaf => 0,
+ children => [
+ {
+ name => 'tank',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => 'mirror-0',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/ata-DISK_A-part1',
+ state => 'ONLINE',
+ read => 1130,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ {
+ name => '/dev/disk/by-id/ata-DISK_B-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 15565,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ ],
+ },
+ ],
+ },
+ },
];
plan tests => scalar @$tests;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH storage 3/3] fix #6389: api: disks: zfs: parse the message of hot spares in use
2026-10-03 12:49 [PATCH storage 0/3] api: disks: zfs: fix error counters and in-use spares in pool details Michal Fox
2026-10-03 12:49 ` [PATCH storage 1/3] test: add tests for zpool status parsing of the ZFS disk API Michal Fox
2026-10-03 12:49 ` [PATCH storage 2/3] fix #6640, #6938: api: disks: zfs: return exact error counters Michal Fox
@ 2026-10-03 12:49 ` Michal Fox
2 siblings, 0 replies; 4+ messages in thread
From: Michal Fox @ 2026-10-03 12:49 UTC (permalink / raw)
To: pve-devel
Hot spares have no error counters in the output of 'zpool status', so a
spare that is in use only has a message after its state:
nvme10n1 INUSE currently in use
The parser took the three words of that message as READ, WRITE and
CKSUM counters, so the API returned zero for all of them and lost the
message, while logging warnings like:
Argument "currently" isn't numeric in addition (+)
As 'zpool status' is called with '-p', the counters are always plain
numbers, so only accept digits for them. This way, the message of a
spare in use ends up in 'msg', like for any other vdev.
Signed-off-by: Michal Fox <me@dualfroz.com>
---
src/PVE/API2/Disks/ZFS.pm | 2 +-
src/test/zfs_pool_detail_test.pm | 103 +++++++++++++++++++++++++++++++
2 files changed, 104 insertions(+), 1 deletion(-)
diff --git a/src/PVE/API2/Disks/ZFS.pm b/src/PVE/API2/Disks/ZFS.pm
index b26805a..2215dd6 100644
--- a/src/PVE/API2/Disks/ZFS.pm
+++ b/src/PVE/API2/Disks/ZFS.pm
@@ -245,7 +245,7 @@ __PACKAGE__->register_method({
$config = 1;
} elsif (
$config
- && $line =~ m/^(\s+)(\S+)\s*(\S+)?(?:\s+(\S+)\s+(\S+)\s+(\S+))?\s*(.*)$/
+ && $line =~ m/^(\s+)(\S+)\s*(\S+)?(?:\s+(\d+)\s+(\d+)\s+(\d+))?\s*(.*)$/
) {
my ($space, $name, $state, $read, $write, $cksum, $msg) =
($1, $2, $3, $4, $5, $6, $7);
diff --git a/src/test/zfs_pool_detail_test.pm b/src/test/zfs_pool_detail_test.pm
index 00d666a..bcca5ae 100644
--- a/src/test/zfs_pool_detail_test.pm
+++ b/src/test/zfs_pool_detail_test.pm
@@ -225,6 +225,109 @@ EOF
],
},
},
+ {
+ description => 'hot spare in use',
+ status => <<"EOF",
+ pool: tank
+ state: ONLINE
+ scan: resilvered 725M in 00:00:00 with 0 errors on Mon May 12 16:50:25 2025
+config:
+
+\tNAME STATE READ WRITE CKSUM
+\ttank ONLINE 0 0 0
+\t mirror-0 ONLINE 0 0 0
+\t spare-0 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_A-part1 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_C-part1 ONLINE 0 0 0
+\t /dev/disk/by-id/ata-DISK_B-part1 ONLINE 0 0 0
+\tspares
+\t /dev/disk/by-id/ata-DISK_C-part1 INUSE currently in use
+
+errors: No known data errors
+EOF
+ expected => {
+ name => 'tank',
+ state => 'ONLINE',
+ scan => 'resilvered 725M in 00:00:00 with 0 errors on Mon May 12 16:50:25 2025',
+ errors => 'No known data errors',
+ leaf => 0,
+ children => [
+ {
+ name => 'tank',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => 'mirror-0',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => 'spare-0',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/ata-DISK_A-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ {
+ name => '/dev/disk/by-id/ata-DISK_C-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ {
+ name => '/dev/disk/by-id/ata-DISK_B-part1',
+ state => 'ONLINE',
+ read => 0,
+ write => 0,
+ cksum => 0,
+ msg => '',
+ leaf => 1,
+ },
+ ],
+ },
+ ],
+ },
+ {
+ name => 'spares',
+ msg => '',
+ leaf => 0,
+ children => [
+ {
+ name => '/dev/disk/by-id/ata-DISK_C-part1',
+ state => 'INUSE',
+ msg => 'currently in use',
+ leaf => 1,
+ },
+ ],
+ },
+ ],
+ },
+ },
];
plan tests => scalar @$tests;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread