ZFS: re-read a transitional pool state before failing the monitor

[lhjnano]

Follow-up to #2206.

#2206 made start wait for the pool state to settle, and that part works.
But it only covered the start path, and the failure I was actually chasing
happens on the monitor path — so #2206 on its own did not fix it. Sorry for
coming back so soon after it was merged; I should have looked at the monitor
in the same pass instead of splitting it across two rounds.

The problem

/proc/spl/kstat/zfs/<pool>/state is a lock-free snapshot. A monitor that
samples it while the pool is being imported, or while a vdev is being
replaced, can read a transient OFFLINE, or an empty string if the read
raced an export. zpool_monitor maps anything that is not
ONLINE/DEGRADED/FAULTED to OCF_ERR_GENERIC, so that single sample is
reported to pacemaker as a resource failure. With a low
migration-threshold it is enough to move a perfectly healthy pool off its
node.

We hit this in production. On one node we logged 1210 ONLINE/OFFLINE
transitions in 65 minutes (median 4 ms, longest burst 78 ms). About 93% of
the samples read ONLINE first, which is why this looks like a rare flake
rather than a broken pool.

The window is small, but it is only reachable while the pool is in
transition and a monitor lands in the same instant. A plain
zpool export / zpool import loop does not reproduce it — I ran 100
import cycles and 3273 samples without a single bad reading. It needs the
transition and a concurrent monitor together.

The fix

Re-read the state before deciding. A pool that is really broken keeps
reporting the same value, so the retry costs nothing in that case; one that
was caught mid-transition settles within a few samples.

The retry budget is bounded by a new monitor_settle_ms (default 500 ms)
and additionally clamped to OCF_RESKEY_CRM_meta_timeout minus a small
margin, so the monitor can never overrun its own deadline. Setting
monitor_settle_ms=0 restores the previous behaviour of failing on the
first reading.

Measured effect

Same node, same script, 60 import/export transitions with a concurrent
monitor loop:

agent	monitor failures
before this series	5 / 60
with this series	0 / 60

Caught in the act. On an unrelated test cluster, running the patched agent,
the monitor sampled a transitional state twice and recovered both times:

rf_pool42836001: start settled on 'ONLINE'
rf_pool42836001: 'OFFLINE' (kstat=present), re-reading
rf_pool42836001: settled on 'ONLINE'

rf_pool43290001: 'OFFLINE' (kstat=present), re-reading
rf_pool43290001: settled on 'ONLINE'

Pacemaker recorded Result of monitor operation ... ok for both. Without
the re-read those two samples would have been OCF_ERR_GENERIC on a healthy
pool.

Note the first pool: start settled on 'ONLINE' was logged, and the pool
still read OFFLINE from a later monitor. Waiting on the start path is not
enough on its own, which is what this series is for.

Across the same run: 22 starts, all settled, no warnings, 2 re-reads, 0
monitor failures.

The oscillation itself is not specific to one machine. Sampling the kstat
every 10ms from the moment each pool appeared, on a different cluster
again:

A  [258ms] ONLINE -> OFFLINE  ->  [273ms] ONLINE    15ms
B  [ 68ms] ONLINE -> OFFLINE  ->  [ 84ms] ONLINE    16ms
B  [206ms] ONLINE -> OFFLINE  ->  [221ms] ONLINE    15ms

Three bursts, all 15-16ms wide and all within 300ms of the import. Against
a 5s monitor interval that is a small chance of collision per transition,
which is why this reads as a rare flake in the field rather than as a
reproducible bug.

Commits

commit	what it does	diff
1	ZFS: read the pool health in one place — pull the kstat/zpool list reading into zpool_read_health, and make the kstat root a parameter (kstat_root) so it can be pointed at a fixture. Pure refactor: meta-data output is byte-identical to the base.	+50 −24
2	ZFS: require a stable state before start returns — rework #2206's settle loop on top of the above, so start and monitor share one implementation, and log the state it settled on. A successful settle used to leave no trace at all, so the only way to tell it had run was to time the agent by hand; this runs once per start, not per monitor.	+68 −27
3	ZFS: re-read a transitional state before failing the monitor — the actual fix, plus monitor_settle_ms and its validation.	+51 −1
4	ZFS: add a test for the transitional-state handling	+145 −1

Commit 1 is deliberately separate so the behaviour change in 3 is small and
readable on its own.

Tests

heartbeat/tests/ZFS.test.sh drives the real agent with a zpool stub on
PATH and a fake kstat tree, so it runs anywhere — no ZFS, no pool, no
root, no tools/ocft setup. It is wired into make check, so it runs in CI
with the rest of the suite rather than being something a reviewer has to
take my word for.

$ make check
...
ok      steady ONLINE                                        rc=0
ok      steady DEGRADED                                      rc=0
ok      OFFLINE burst then ONLINE                            rc=0
ok      empty reading then ONLINE                            rc=0
ok      steady OFFLINE                                       rc=1
ok      steady OFFLINE stays inside the budget               1s
ok      monitor_settle_ms=0 keeps the old behaviour          rc=1
ok      settle_timeout=abc is rejected                       rc=6
ok      monitor_settle_ms=abc is rejected                    rc=6
9 passed, 0 failed

The heartbeat/Makefile.am hunk is only what wires that in: EXTRA_DIST
for the new file and a check-ZFS target added to the existing check
dependency. If you would rather this lived under tools/ocft, or not run in
check at all, say so and I will move it.

On the size

This is larger than I would like for a follow-up — about 310 added lines,
roughly half of it the test, and most of the rest commit 1, which moves
existing code around without changing behaviour.

I split it this way so the behaviour change would stay small on its own,
and so the fix would be verifiable in CI rather than only described in a
commit message.

[knet-jenkins]

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2213/1/input

[oalbrigt]

The agent changes looks good.

I've added some suggestions for the test script.

[oalbrigt]

We do not have ZFS in our CI, and most of the users dont use it in their build envs, so we only want it as a separate check, like you've done below.

[lhjnano]

Done — I dropped the heartbeat/Makefile.am change entirely rather than just
taking it out of check, since the test now lives under tools/ocft and
there is nothing left for that file to wire up.

[knet-jenkins]

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2213/2/input