From a5aa881ac530f4299d3b26557e1aab92842e2620 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 19:11:39 +0100 Subject: [PATCH] Treat resource identity lists as sets in change history Avoid configuration-history rows when source merges only reorder identity addresses or represent an absent list as empty. Real identity changes remain recorded. The #2319 serial-bearing disk fixture and registry-generation check cover the tags-only and no-op identity paths. Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 3 ++ .../internal/subsystems/unified-resources.md | 7 ++++ .../physical_disk_roundtrip_test.go | 5 ++- internal/unifiedresources/change_emission.go | 16 ++++++++- .../unifiedresources/change_emission_test.go | 30 +++++++++++++++++ internal/unifiedresources/registry_test.go | 33 +++++++++++++++++++ 6 files changed, 92 insertions(+), 2 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 7af862eb62..4fc181d6bb 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -817,6 +817,9 @@ change-journal rows when the PVE inventory is empty. Agent-only disks remain visible through their Agent source, and an explicit failed-query Agent fallback still supplies PVE inventory. `TestPhysicalDiskSkippedPollDoesNotPromoteAgentOnlySMARTToPVEInventory` checks repeated empty-inventory/skipped-poll cycles and journal row counts; +its serial-bearing fixture yields a tags-only row if the source guard is +removed, matching the reported history shape without claiming the reporter's +topology or installed write rate. With the guard, all three cycles add no rows. `TestPhysicalDiskSkippedPollPreservesSourceIdentity` checks genuine PVE disk continuity. These fixtures do not establish the reporter's installed cause. SMART enrichment matches serial, WWN, device path, and controller diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 5fde1efcf9..8e70cef3a7 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5055,6 +5055,13 @@ for provider-read breadcrumbs such as VMware tasks and events, plus the change model instead of introducing a second event shape, and `RecordChange` must stay idempotent by canonical change ID so poller refreshes and replayed supplemental snapshots do not duplicate resource history. +Resource identity change emission compares scalar identifiers exactly and +hostname, IP-address and MAC-address lists as sets. Reordering a source merge +or changing a list between nil and empty is not an identity change; a changed +serial/machine ID, guest key or actual set member still is. This prevents +no-op `identity` configuration rows without weakening canonical matching or +changing the public resource shape. `TestResourceChangeIdentityIgnoresSetOrderAndEmptySlices` +pins both the quiet and real-change cases. Change emission over registry rebuilds must diff relationships by edge identity only: canonical source, canonical target, type, and active state, order-insensitive (`relationshipsEquivalent` in `change_emission.go`). diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 60602b2b1c..64060eaa9a 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -195,7 +195,10 @@ func TestPhysicalDiskSkippedPollDoesNotPromoteAgentOnlySMARTToPVEInventory(t *te ID: "agent", Hostname: "node", LinkedNodeID: "pve-node", Status: "online", LastSeen: now, Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ - Device: "sda", Type: "sata", Health: "PASSED", + // A stable serial keeps the canonical identity unchanged; without + // the source guard, the false PVE observation produces exactly the + // tags-only change reported in #2319 rather than a tags+identity row. + Device: "sda", Serial: "disk-serial", Type: "sata", Health: "PASSED", }}}, }) store := unifiedresources.NewMemoryStore() diff --git a/internal/unifiedresources/change_emission.go b/internal/unifiedresources/change_emission.go index ea3a15a966..ab3df74291 100644 --- a/internal/unifiedresources/change_emission.go +++ b/internal/unifiedresources/change_emission.go @@ -236,7 +236,7 @@ func resourceChangedFields(before, after Resource) []string { if before.CustomURL != after.CustomURL { changed = append(changed, "customUrl") } - if !reflect.DeepEqual(before.Identity, after.Identity) { + if !resourceIdentityEquivalent(before.Identity, after.Identity) { changed = append(changed, "identity") } if (isAvailabilityOwnedResource(before) || isAvailabilityOwnedResource(after)) && @@ -545,6 +545,20 @@ func changeSourceAdapterForDataSource(source DataSource) ChangeSourceAdapter { } } +// Identity addresses, hostnames and MACs are sets, not ordered observations. +// Registry merges can reorder them or turn a nil slice into an empty one while +// the actual identity remains unchanged; those differences must not create +// configuration-history rows on every poll. +func resourceIdentityEquivalent(a, b ResourceIdentity) bool { + return a.MachineID == b.MachineID && + a.DMIUUID == b.DMIUUID && + a.ClusterName == b.ClusterName && + a.ProxmoxGuestKey == b.ProxmoxGuestKey && + sameStringSet(a.Hostnames, b.Hostnames) && + sameStringSet(a.IPAddresses, b.IPAddresses) && + sameStringSet(a.MACAddresses, b.MACAddresses) +} + func sameStringSet(a, b []string) bool { if len(a) != len(b) { return false diff --git a/internal/unifiedresources/change_emission_test.go b/internal/unifiedresources/change_emission_test.go index 76140455b7..a3f43c7029 100644 --- a/internal/unifiedresources/change_emission_test.go +++ b/internal/unifiedresources/change_emission_test.go @@ -522,6 +522,36 @@ func TestBuildResourceChange_ClassifiesConfigUpdate(t *testing.T) { } } +func TestResourceChangeIdentityIgnoresSetOrderAndEmptySlices(t *testing.T) { + before := Resource{ + ID: "disk-1", Type: ResourceTypePhysicalDisk, Status: StatusOnline, + Identity: ResourceIdentity{ + MachineID: "serial-1", Hostnames: []string{"node", "node.example"}, + }, + } + after := before + after.Identity = ResourceIdentity{ + MachineID: "serial-1", Hostnames: []string{"node.example", "node"}, + IPAddresses: []string{}, MACAddresses: []string{}, + } + if change := buildResourceChange(before, true, after, true, time.Now().UTC(), nil, SourcePulseDiff, ""); change != nil { + t.Fatalf("unchanged identity set emitted history: %+v", change) + } + + after.Identity.MachineID = "serial-2" + change := buildResourceChange(before, true, after, true, time.Now().UTC(), nil, SourcePulseDiff, "") + if change == nil || !sameStringSet(mustChangedFields(t, change), []string{"identity"}) { + t.Fatalf("real identity change not recorded: %+v", change) + } + + after.Identity.MachineID = before.Identity.MachineID + after.Identity.IPAddresses = []string{"192.0.2.10"} + change = buildResourceChange(before, true, after, true, time.Now().UTC(), nil, SourcePulseDiff, "") + if change == nil || !sameStringSet(mustChangedFields(t, change), []string{"identity"}) { + t.Fatalf("new identity set member not recorded: %+v", change) + } +} + func mustChangedFields(t *testing.T, change *ResourceChange) []string { t.Helper() raw, ok := change.Metadata["changedFields"] diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 734f1c647e..9203721d56 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -58,6 +58,39 @@ func TestRegistryGenerationComparisonIgnoresUnchangedTelemetry(t *testing.T) { } } +func TestRegistryGenerationComparisonTreatsIdentityListsAsSets(t *testing.T) { + store := NewMemoryStore() + now := time.Date(2026, 9, 29, 17, 0, 0, 0, time.UTC) + entry := IngestRecord{ + SourceID: "disk-1", + Resource: Resource{Type: ResourceTypePhysicalDisk, Name: "disk", Status: StatusOnline, LastSeen: now}, + Identity: ResourceIdentity{MachineID: "serial-1", Hostnames: []string{"node", "node.example"}}, + } + before := NewRegistry(store) + before.IngestRecords(SourceAgent, []IngestRecord{entry}) + + reordered := entry + reordered.Identity = ResourceIdentity{ + MachineID: "serial-1", Hostnames: []string{"node.example", "node"}, + IPAddresses: []string{}, MACAddresses: []string{}, + } + after := NewRegistry(store) + after.IngestRecords(SourceAgent, []IngestRecord{reordered}) + recordRegistryChangesBetweenGenerations(before, after, now.Add(time.Minute), nil, SourcePulseDiff, "") + if len(store.changes) != 0 { + t.Fatalf("equivalent identity lists emitted %d history rows: %+v", len(store.changes), store.changes) + } + + changed := reordered + changed.Identity.IPAddresses = []string{"192.0.2.10"} + later := NewRegistry(store) + later.IngestRecords(SourceAgent, []IngestRecord{changed}) + recordRegistryChangesBetweenGenerations(after, later, now.Add(2*time.Minute), nil, SourcePulseDiff, "") + if len(store.changes) != 1 || !sameStringSet(mustChangedFields(t, &store.changes[0]), []string{"identity"}) { + t.Fatalf("real address change not recorded once: %+v", store.changes) + } +} + // TestMemoryStore_RecordActionAuditAppliesRedaction is an integration check // at the registry-store boundary. The MemoryStore is the backing store the // registry uses in tests and contract examples, and operator-authored audit