diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index c3d2879a0f..9df72c3422 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1664,6 +1664,18 @@ Proxmox disk health alert evaluation now lives in identity, disk health assessment alerts, known-firmware health suppression, and SSD wearout alerts; future Proxmox disk-health behavior should extend that checker owner rather than expanding the central Manager file. +PVE disk-health and wearout occurrences retain the existing +`instance:node:disk:sanitized-device-path` resource reference. The shared +constructor in unified resources must produce the same identity as the alert +checker; changing occurrence IDs to match registry source IDs would reopen +historical incidents and can re-notify a worn disk. The registry resolves this +reference as an alias of the exact physical disk for operator-state lookups. +Muting one physical disk therefore resolves its active wearout alert and blocks +later writes and notifications without muting another disk or changing the +wearout recovery rules. `TestProxmoxDiskCanonicalResourceIDTrimsIdentity` pins +the persisted identity shape; monitoring's registry-backed +`TestProxmoxPhysicalDiskMuteResolvesAndSuppressesWearoutAlert` pins the policy +path. These are source invariants, not evidence of installed field relief. Shared metric threshold runtime now lives in `internal/alerts/metric_runtime.go`. That file owns metric threshold lookup, per-metric delay and intent resolution, reducer input composition, active-alert diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 96831c9796..f54532363e 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -4483,6 +4483,15 @@ Canonical physical-disk views now expose the full disk identity and SMART metadata needed by monitoring refresh paths, so physical-disk temperature and SMART merges can run from unified `ReadState` instead of from snapshot-owned disk arrays. +For a Proxmox physical disk with a complete instance, node and device path, +canonical aliases include the persisted PVE disk-alert reference +`instance:node:disk:sanitized-device-path`. The alert checker and registry use +one constructor for that legacy reference. It is a lookup alias, never the +disk's primary canonical ID or source ID, and must not leak across nodes, +instances or non-disk resources. Ambiguous alias claims still fail closed under +normal reference resolution. `TestProxmoxDiskAlertAliasIsScopedAndNotPrimaryIdentity` +pins the identity boundary, and the registry-backed monitoring mute test pins +its operator-state consumer. When host-agent SMART and Proxmox physical-disk rows merge, the unified resource must preserve both the enriched `PhysicalDiskMeta` and the Proxmox source payload (`ProxmoxData.NodeName`, `Instance`, and source id). A diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 7e78fb8377..257841ce14 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -423,9 +423,18 @@ func TestBuildBackupPVETemplateSubjectKeyTrimsParts(t *testing.T) { } func TestProxmoxDiskCanonicalResourceIDTrimsIdentity(t *testing.T) { - got := proxmoxDiskCanonicalResourceID(" inst ", " node ", "/dev/sda") - if got != "inst:node:disk:dev-sda" { - t.Fatalf("proxmoxDiskCanonicalResourceID() = %q, want %q", got, "inst:node:disk:dev-sda") + for _, tt := range []struct { + name, instance, node, path, want string + }{ + {"trimmed source identity", " inst ", " node ", "/dev/sda", "inst:node:disk:dev-sda"}, + {"nested device path", "inst", "node", "/dev/disk/by-id/ATA_DISK", "inst:node:disk:dev-disk-by-id-ata-disk"}, + {"root path", "inst", "node", "/", "inst:node:disk:root"}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := proxmoxDiskCanonicalResourceID(tt.instance, tt.node, tt.path); got != tt.want { + t.Fatalf("proxmoxDiskCanonicalResourceID() = %q, want %q", got, tt.want) + } + }) } } diff --git a/internal/alerts/disk_health.go b/internal/alerts/disk_health.go index db41da7cb3..4afe7de5c4 100644 --- a/internal/alerts/disk_health.go +++ b/internal/alerts/disk_health.go @@ -13,7 +13,7 @@ import ( ) func proxmoxDiskCanonicalResourceID(instance, node, devPath string) string { - return fmt.Sprintf("%s:%s:disk:%s", strings.TrimSpace(instance), strings.TrimSpace(node), sanitizeAlertKey(devPath)) + return unifiedresources.ProxmoxPhysicalDiskAlertResourceID(instance, node, devPath) } func proxmoxDiskAlertMetadata(disk proxmox.Disk) map[string]interface{} { diff --git a/internal/monitoring/monitor_alert_intent_test.go b/internal/monitoring/monitor_alert_intent_test.go index 71d6fcbddc..6832ec92ce 100644 --- a/internal/monitoring/monitor_alert_intent_test.go +++ b/internal/monitoring/monitor_alert_intent_test.go @@ -39,6 +39,59 @@ func TestInstallOperatorIntentResolverProjectsCanonicalResourcePolicy(t *testing } } +func TestProxmoxPhysicalDiskMuteResolvesAndSuppressesWearoutAlert(t *testing.T) { + store := unifiedresources.NewMemoryStore() + registry := unifiedresources.NewRegistry(store) + instance, node := "pve", "rocket" + makeDisk := func(path string) models.PhysicalDisk { + return models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID(instance, node, path, "", ""), + Instance: instance, Node: node, DevPath: path, + Model: "KINGSTON SA400", Type: "ssd", Health: "PASSED", Wearout: 0, + LastChecked: time.Now().UTC(), + } + } + registry.IngestSnapshot(models.StateSnapshot{ + PhysicalDisks: []models.PhysicalDisk{makeDisk("/dev/sda"), makeDisk("/dev/sdb")}, + }) + diskID := unifiedresources.ProxmoxPhysicalDiskAlertResourceID(instance, node, "/dev/sda") + canonicalID, found := registry.ResolveReferenceID(diskID) + if !found { + t.Fatalf("PVE wearout alert resource %q did not resolve to a physical disk", diskID) + } + otherID := unifiedresources.ProxmoxPhysicalDiskAlertResourceID(instance, node, "/dev/sdb") + if other, ok := registry.ResolveReferenceID(otherID); !ok || other == canonicalID { + t.Fatalf("other PVE disk identity = %q, %v; want a different disk", other, ok) + } + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + t.Cleanup(manager.Stop) + worn := proxmox.Disk{DevPath: "/dev/sda", Model: "KINGSTON SA400", Type: "ssd", Health: "PASSED", Wearout: 0} + manager.CheckDiskHealth(instance, node, worn) + if got := len(manager.GetActiveAlerts()); got != 1 { + t.Fatalf("unmuted worn disk raised %d alerts, want one", got) + } + if err := store.SetResourceOperatorState(unifiedresources.ResourceOperatorState{ + CanonicalID: canonicalID, MonitoringMode: unifiedresources.MonitoringModeMuted, + }); err != nil { + t.Fatal(err) + } + monitor := &Monitor{alertManager: manager} + monitor.installOperatorIntentResolver(unifiedresources.NewMonitorAdapter(registry)) + if got := len(manager.GetActiveAlerts()); got != 0 { + t.Fatalf("mute left %d existing PVE wearout alerts active", got) + } + manager.CheckDiskHealth(instance, node, worn) + if got := len(manager.GetActiveAlerts()); got != 0 { + t.Fatalf("muted PVE disk raised %d new wearout alerts", got) + } + worn.DevPath = "/dev/sdb" + manager.CheckDiskHealth(instance, node, worn) + if got := len(manager.GetActiveAlerts()); got != 1 { + t.Fatalf("other PVE disk raised %d alerts, want one", got) + } +} + func TestInstallOperatorIntentResolverInheritsScopedMaintenanceFromParent(t *testing.T) { store := unifiedresources.NewMemoryStore() registry := unifiedresources.NewRegistry(store) diff --git a/internal/unifiedresources/canonical_identity.go b/internal/unifiedresources/canonical_identity.go index e86011b9a2..bc89842669 100644 --- a/internal/unifiedresources/canonical_identity.go +++ b/internal/unifiedresources/canonical_identity.go @@ -117,6 +117,16 @@ func canonicalAliases(resource Resource, primaryID, platformID, hostname string) strings.TrimSpace(resource.Proxmox.NodeName), ) values = append(values, resource.Proxmox.NodeAliases...) + // PVE disk alerts predate the unified resource registry and retain a + // path-shaped resource reference. Resolve it to this exact physical + // disk so a UI mute reaches both new and restored occurrences (#2112). + if CanonicalResourceType(resource.Type) == ResourceTypePhysicalDisk && resource.PhysicalDisk != nil && + strings.TrimSpace(resource.Proxmox.Instance) != "" && strings.TrimSpace(resource.Proxmox.NodeName) != "" && + strings.TrimSpace(resource.PhysicalDisk.DevPath) != "" { + values = append(values, ProxmoxPhysicalDiskAlertResourceID( + resource.Proxmox.Instance, resource.Proxmox.NodeName, resource.PhysicalDisk.DevPath, + )) + } } return uniqueTrimmed(values...) diff --git a/internal/unifiedresources/canonical_identity_test.go b/internal/unifiedresources/canonical_identity_test.go index 519de9d994..b99ed9bfae 100644 --- a/internal/unifiedresources/canonical_identity_test.go +++ b/internal/unifiedresources/canonical_identity_test.go @@ -143,6 +143,36 @@ func TestRefreshCanonicalIdentityKeepsProxmoxPresentationSeparateFromNativeAlias } } +func TestProxmoxDiskAlertAliasIsScopedAndNotPrimaryIdentity(t *testing.T) { + resource := Resource{ + ID: "physical-disk-1", Type: ResourceTypePhysicalDisk, + Proxmox: &ProxmoxData{Instance: "pve-a", NodeName: "node-1"}, + PhysicalDisk: &PhysicalDiskMeta{DevPath: "/dev/sda"}, + } + assertAlias := func(want string, present bool) { + t.Helper() + RefreshCanonicalIdentity(&resource) + if resource.Canonical == nil || resource.Canonical.PrimaryID != resource.ID { + t.Fatalf("disk alert alias changed primary identity: %+v", resource.Canonical) + } + found := false + for _, alias := range resource.Canonical.Aliases { + found = found || alias == want + } + if found != present { + t.Fatalf("alias %q present = %v, want %v; aliases = %v", want, found, present, resource.Canonical.Aliases) + } + } + + alertID := ProxmoxPhysicalDiskAlertResourceID("pve-a", "node-1", "/dev/sda") + assertAlias(alertID, true) + resource.Proxmox.NodeName = "node-2" + assertAlias(alertID, false) + assertAlias(ProxmoxPhysicalDiskAlertResourceID("pve-a", "node-2", "/dev/sda"), true) + resource.Type = ResourceTypeStorage + assertAlias(ProxmoxPhysicalDiskAlertResourceID("pve-a", "node-2", "/dev/sda"), false) +} + func TestUnavailableMemoryFacetsDoNotChangeCanonicalIdentity(t *testing.T) { total := int64(8 << 30) resource := Resource{ diff --git a/internal/unifiedresources/physical_disk_ids.go b/internal/unifiedresources/physical_disk_ids.go index 7e2642d5a7..517a8bb8f2 100644 --- a/internal/unifiedresources/physical_disk_ids.go +++ b/internal/unifiedresources/physical_disk_ids.go @@ -64,6 +64,42 @@ func ProxmoxPhysicalDiskSourceID(instance, node, device, controller, target stri return legacy } +// ProxmoxPhysicalDiskAlertResourceID is the persisted resource reference used +// by PVE disk-health and wearout alerts. Keep this distinct from the disk's +// source ID: existing alert occurrences use this shape, while the registry +// needs it as an alias to apply per-disk operator intent. +func ProxmoxPhysicalDiskAlertResourceID(instance, node, device string) string { + return fmt.Sprintf("%s:%s:disk:%s", strings.TrimSpace(instance), strings.TrimSpace(node), physicalDiskAlertKey(device)) +} + +func physicalDiskAlertKey(device string) string { + trimmed := strings.TrimSpace(device) + if trimmed == "/" { + return "root" + } + trimmed = strings.Trim(trimmed, "/\\ ") + if trimmed == "" { + trimmed = "root" + } + var builder strings.Builder + previousDash := false + for _, r := range strings.ToLower(trimmed) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '.': + builder.WriteRune(r) + previousDash = false + case !previousDash: + builder.WriteByte('-') + previousDash = true + } + } + key := strings.Trim(builder.String(), "-.") + if key == "" { + return "disk" + } + return key +} + func PhysicalDiskMetricID(disk models.PhysicalDisk) string { if diskinventory.IsUsableHardwareID(disk.Serial) || diskinventory.IsUsableHardwareID(disk.WWN) { return PreferredPhysicalDiskMetricID(disk.Serial, disk.WWN, "")