From d0e7c747d97cc33671002639623913dde8bee30c Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sun, 16 Aug 2026 06:27:23 +0100 Subject: [PATCH] Clear stale ZFS alerts when a storage loses its pool attachment checkZFSPoolHealth only runs while storage.ZFSPool is attached, so its clear paths never fire once the attachment goes away and previously raised zfs-pool-state, zfs-pool-errors, and zfs-device alerts linger until the multi-day stale cleanup. Clear them on the spot when a storage is checked without a pool. This is the recovery path for the shared storages that were wrongly matched to a node-local pool before the matcher's type gate. Refs #1731 --- .../v6/internal/subsystems/alerts.md | 5 ++ internal/alerts/alerts_test.go | 58 +++++++++++++++++++ internal/alerts/storage.go | 34 +++++++++++ 3 files changed, 97 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index e036d2081..8193a17a3 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -583,6 +583,11 @@ incidents. ZFS device alert labels must preserve raw device names such as `/dev/sda4`, but must not join pool and device labels with a raw slash because device paths can already begin with `/`; browser alert surfaces consume the runtime `resourceName` as authored rather than patching storage labels locally. +ZFS pool and device alerts follow the storage's pool attachment lifecycle. A +storage checked without an attached ZFS pool must shed any previously raised +zfs-pool-state, zfs-pool-errors, and zfs-device alerts on that check rather +than waiting for the stale-alert cleanup, because the health path can only +clear its own alerts while the attachment exists (#1731). Ceph pool storage threshold resolution is also source-alias aware. Storage alerts must evaluate the normalized pool storage id while accepting legacy `agent:-ceph-pool-` override keys as aliases, so operators do not diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 0b3a0288e..c74fc2ae2 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -20162,3 +20162,61 @@ func TestMergeSnapshotOverrideInheritsZeroFields(t *testing.T) { t.Fatalf("explicit snapshot override field lost: %+v", explicit) } } + +// A storage that stops carrying an attached ZFS pool must shed its ZFS alerts +// on the next check instead of keeping them until the multi-day cleanup. This +// is the recovery path for shared storages that were wrongly matched to a +// node-local pool before the matcher gained its shared-type gate (#1731). +func TestCheckStorageClearsZFSAlertsWhenPoolDetaches(t *testing.T) { + m := newTestManager(t) + + degraded := &models.ZFSPool{ + Name: "rpool", + State: "DEGRADED", + Devices: []models.ZFSDevice{ + {Name: "sda", State: "ONLINE", ReadErrors: 3}, + }, + } + storage := models.Storage{ + ID: "nfs-share", + Name: "NFS Share", + Node: "pve-node1", + Instance: "pve-instance", + ZFSPool: degraded, + } + m.checkZFSPoolHealth(storage) + + stateAlertID := buildCanonicalStateID("nfs-share/zfs-pool:rpool", "nfs-share/zfs-pool:rpool-state") + deviceAlertID := buildCanonicalStateID("nfs-share/zfs-pool:rpool/device:sda", "nfs-share/zfs-pool:rpool/device:sda-health") + + m.mu.RLock() + testRequireActiveAlert(t, m, stateAlertID) + testRequireActiveAlert(t, m, deviceAlertID) + m.mu.RUnlock() + + // An unrelated storage keeps its ZFS alerts. + other := storage + other.ID = "other-zfs" + other.Name = "Other ZFS" + m.checkZFSPoolHealth(other) + + storage.ZFSPool = nil + m.clearStorageZFSAlerts(storage) + + m.mu.RLock() + _, stateExists := testLookupActiveAlert(t, m, stateAlertID) + _, deviceExists := testLookupActiveAlert(t, m, deviceAlertID) + otherStateID := buildCanonicalStateID("other-zfs/zfs-pool:rpool", "other-zfs/zfs-pool:rpool-state") + _, otherExists := testLookupActiveAlert(t, m, otherStateID) + m.mu.RUnlock() + + if stateExists { + t.Error("expected pool state alert cleared after pool detached") + } + if deviceExists { + t.Error("expected device alert cleared after pool detached") + } + if !otherExists { + t.Error("expected unrelated storage to keep its ZFS alerts") + } +} diff --git a/internal/alerts/storage.go b/internal/alerts/storage.go index 0580efbd9..6b4b0bef9 100644 --- a/internal/alerts/storage.go +++ b/internal/alerts/storage.go @@ -107,6 +107,40 @@ func (m *Manager) CheckStorage(storage models.Storage) { // Check ZFS pool status if this is ZFS storage if storage.ZFSPool != nil { m.checkZFSPoolHealth(storage) + } else { + m.clearStorageZFSAlerts(storage) + } +} + +// clearStorageZFSAlerts removes ZFS pool/device alerts for a storage that no +// longer carries an attached ZFS pool. checkZFSPoolHealth can only clear its +// own alerts while the pool stays attached, so a storage whose attachment goes +// away (a shared storage that was wrongly matched to a node-local pool, or a +// pool genuinely detached from a storage) would otherwise keep its stale ZFS +// alerts until the multi-day cleanup (#1731). +func (m *Manager) clearStorageZFSAlerts(storage models.Storage) { + storageID := strings.TrimSpace(storage.ID) + if storageID == "" { + return + } + compositePrefix := storageID + "/zfs-pool:" + + m.mu.Lock() + defer m.mu.Unlock() + for alertID, alert := range m.activeAlerts { + if alert == nil { + continue + } + switch alert.Type { + case "zfs-pool-state", "zfs-pool-errors", "zfs-device": + default: + continue + } + resourceID := strings.TrimSpace(alert.ResourceID) + if resourceID != storageID && !strings.HasPrefix(resourceID, compositePrefix) { + continue + } + m.clearAlertNoLock(alertID) } }