diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index e6cf52d39..616b32f2b 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5245,3 +5245,24 @@ provider keys, canonical IDs, JSON projection, temperature/size/cadence, separat nodes/controller members, and confirmed inventory removal. `TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. This is synthetic runtime evidence, not USB hardware or reporter acceptance. + +### Linked direct-disk aliases with missing hardware identity (#2076) + +Agent SMART and PVE physical-disk observations may share a canonical resource +when they have the same already-correlated parent, the same normalized device +path, compatible controller/target metadata and exactly one opposite-source +candidate. The direct-device fallback applies only when at least one observation +has neither a usable serial nor WWN. Two differing populated hardware identities +are not missing identity; matching capacity or model is never identity evidence. +Controller-member targets cannot use this fallback because a kernel path can +represent several members. Existing explicit hardware and SAS correlation remain +separate rules. Source-native IDs and per-source status are retained on the merged +resource, as are hardware details supplied by the richer collector. + +`TestIssue2076USBMixedSourceSnapshot` covers serial-present/absent in either +collector, both absent and placeholder identity through repeated ordinary snapshot +replacement, physical-disk views and JSON projection. Negative cases retain +separate conflicting IDs, devices, hosts and controller members; the linked-disk +ambiguity test rejects multiple opposite-source candidates in both directions. +These are synthetic source proofs, not appliance acceptance or a claim about +which collector produced a reporter's row. diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 34ea89552..fa85deb04 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -3053,7 +3053,8 @@ func (rr *ResourceRegistry) resolveLinkedResource(source DataSource, sourceID st // one already-linked host boundary. This is the durable join for cases where // Proxmox reports a SAS address in its serial field while smartctl reports the // drive's real serial. Device paths are safe only inside the common parent and -// only when the topology match is unique. +// only when the topology match is unique. Serial-less direct devices also use +// this join: a USB bridge may expose hardware identity to only one collector. func (rr *ResourceRegistry) resolveLinkedPhysicalDisk(source DataSource, incoming Resource) string { if incoming.PhysicalDisk == nil || incoming.ParentID == nil { return "" @@ -3090,7 +3091,7 @@ func (rr *ResourceRegistry) resolveLinkedPhysicalDisk(source DataSource, incomin existingDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(existing.PhysicalDisk.DevPath)) agentReportsSAS := (source == SourceProxmox && strings.EqualFold(existing.PhysicalDisk.DiskType, "sas")) || (source == SourceAgent && strings.EqualFold(incoming.PhysicalDisk.DiskType, "sas")) - deviceMatch := agentReportsSAS && + deviceMatch := (agentReportsSAS || physicalDiskMissingIdentityPathCompatible(incoming.PhysicalDisk, existing.PhysicalDisk)) && incomingDevice != "" && incomingDevice == existingDevice && physicalDiskTopologyCompatible(incoming.PhysicalDisk, existing.PhysicalDisk) @@ -3105,6 +3106,21 @@ func (rr *ResourceRegistry) resolveLinkedPhysicalDisk(source DataSource, incomin return matchID } +// A kernel path identifies a direct device only within its already-correlated +// host. Missing identity is not conflicting identity. Never use this fallback +// for controller members: the block path can represent several physical disks. +func physicalDiskMissingIdentityPathCompatible(left, right *PhysicalDiskMeta) bool { + if left == nil || right == nil { + return false + } + if diskinventory.IsControllerMemberTarget(left.Target) || diskinventory.IsControllerMemberTarget(right.Target) { + return false + } + leftHasID := diskinventory.IsUsableHardwareID(left.Serial) || diskinventory.IsUsableHardwareID(left.WWN) + rightHasID := diskinventory.IsUsableHardwareID(right.Serial) || diskinventory.IsUsableHardwareID(right.WWN) + return !leftHasID || !rightHasID +} + func physicalDiskTopologyCompatible(left, right *PhysicalDiskMeta) bool { if left == nil || right == nil { return false @@ -3503,13 +3519,13 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou previous := existing.PhysicalDisk existing.PhysicalDisk = mergePhysicalDiskData(existing.PhysicalDisk, incoming.PhysicalDisk) if source == SourceProxmox && previous != nil && hasDataSource(existing.Sources, SourceAgent) { - if previous.Serial != "" && + if diskinventory.IsUsableHardwareID(previous.Serial) && (previous.Collection == nil || (previous.Collection.Serial.State == diskinventory.FieldAvailable && strings.HasPrefix(strings.ToLower(previous.Collection.Serial.Source), "smartctl"))) { existing.PhysicalDisk.Serial = previous.Serial } - if previous.WWN != "" { + if diskinventory.IsUsableHardwareID(previous.WWN) { existing.PhysicalDisk.WWN = previous.WWN } if previous.DiskType != "" { @@ -3830,10 +3846,10 @@ func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMet if incoming.Vendor != "" { merged.Vendor = incoming.Vendor } - if incoming.Serial != "" { + if diskinventory.IsUsableHardwareID(incoming.Serial) { merged.Serial = incoming.Serial } - if incoming.WWN != "" { + if diskinventory.IsUsableHardwareID(incoming.WWN) { merged.WWN = incoming.WWN } if incoming.DiskType != "" { diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 5f87f952f..d6af61d4c 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -1,6 +1,8 @@ package unifiedresources import ( + "encoding/json" + "fmt" "reflect" "strings" "testing" @@ -6482,3 +6484,123 @@ func TestPlatformOnlyResourceStatusRecoversWithoutOverridingAgent(t *testing.T) t.Fatalf("platform overwrote higher-priority agent status: %s", got.Status) } } + +func usbAliasSnapshot(agentSerial, pveSerial string) models.StateSnapshot { + now := time.Now() + return models.StateSnapshot{ + Nodes: []models.Node{{ID: "pve-node", Name: "node", Instance: "pve", LinkedAgentID: "agent", Status: "online", LastSeen: now}}, + Hosts: []models.Host{{ID: "agent", Hostname: "node", LinkedNodeID: "pve-node", Status: "online", LastSeen: now, Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{Device: "sdx", Serial: agentSerial, Type: "usb", SizeBytes: 239_000_000_000, Health: "PASSED", Temperature: 31}}}}}, + PhysicalDisks: []models.PhysicalDisk{{ID: ProxmoxPhysicalDiskSourceID("pve", "node", "/dev/sdx", "", ""), Instance: "pve", Node: "node", DevPath: "/dev/sdx", Serial: pveSerial, Type: "usb", Size: 239_000_000_000, Health: "PASSED", LastChecked: now}}, + } +} + +// Exercise ordinary host/PVE snapshot ingestion, canonical views and their JSON +// projection repeatedly, not only a correlation helper or an invented identity. +func TestIssue2076USBMixedSourceSnapshot(t *testing.T) { + for _, serials := range [][2]string{{"", "USB-SERIAL"}, {"USB-SERIAL", ""}, {"", ""}, {"UNKNOWN", "USB-SERIAL"}, {"USB-SERIAL", "UNKNOWN"}} { + t.Run(fmt.Sprintf("%s/%s", serials[0], serials[1]), func(t *testing.T) { + snapshot := usbAliasSnapshot(serials[0], serials[1]) + adapter := NewMonitorAdapter(NewRegistry(nil)) + var id string + for cycle := 0; cycle < 4; cycle++ { + adapter.PopulateFromSnapshot(snapshot) + views := adapter.PhysicalDisks() + if len(views) != 1 { + t.Fatalf("cycle %d: disks = %d, want one linked USB device", cycle, len(views)) + } + if cycle == 0 { + id = views[0].ID() + } else if views[0].ID() != id { + t.Fatal("canonical ID changed") + } + payload, err := json.Marshal(adapter.GetAll()) + if err != nil { + t.Fatal(err) + } + var resources []Resource + if err := json.Unmarshal(payload, &resources); err != nil { + t.Fatal(err) + } + count := 0 + for _, r := range resources { + if r.Type != ResourceTypePhysicalDisk { + continue + } + count++ + if !hasDataSource(r.Sources, SourceAgent) || !hasDataSource(r.Sources, SourceProxmox) || r.ParentID == nil { + t.Fatalf("missing source/parent: %+v", r) + } + if r.PhysicalDisk == nil || r.PhysicalDisk.SizeBytes != 239_000_000_000 || r.PhysicalDisk.Temperature != 31 { + t.Fatalf("lost disk facts: %+v", r.PhysicalDisk) + } + if serials[0] == "USB-SERIAL" || serials[1] == "USB-SERIAL" { + if r.PhysicalDisk.Serial != "USB-SERIAL" { + t.Fatalf("lost serial: %+v", r.PhysicalDisk) + } + } + } + if count != 1 { + t.Fatalf("JSON disks = %d", count) + } + } + }) + } +} + +func TestIssue2076USBDoesNotMergeUnsafeAliases(t *testing.T) { + for _, tc := range []struct { + name string + change func(*models.StateSnapshot) + }{ + {"conflicting serials", func(s *models.StateSnapshot) { s.Hosts[0].Sensors.SMART[0].Serial = "OTHER-SERIAL" }}, + {"different device same size", func(s *models.StateSnapshot) { s.Hosts[0].Sensors.SMART[0].Device = "sdy" }}, + {"controller member", func(s *models.StateSnapshot) { s.Hosts[0].Sensors.SMART[0].Target = "megaraid,0" }}, + {"conflicting controllers", func(s *models.StateSnapshot) { + s.Hosts[0].Sensors.SMART[0].Controller = "controller-a" + s.PhysicalDisks[0].Controller = "controller-b" + }}, + {"different hosts", func(s *models.StateSnapshot) { + s.Hosts[0].LinkedNodeID = "" + s.Hosts[0].Hostname = "other" + s.Nodes[0].LinkedAgentID = "" + }}, + } { + t.Run(tc.name, func(t *testing.T) { + s := usbAliasSnapshot("", "USB-SERIAL") + tc.change(&s) + rr := NewRegistry(nil) + rr.IngestSnapshot(s) + if got := len(rr.ListByType(ResourceTypePhysicalDisk)); got != 2 { + t.Fatalf("disks=%d, want separate observations", got) + } + }) + } +} + +func TestIssue2076USBLinkedDiskAmbiguity(t *testing.T) { + for _, source := range []DataSource{SourceAgent, SourceProxmox} { + t.Run(string(source), func(t *testing.T) { + other := SourceAgent + if source == SourceAgent { + other = SourceProxmox + } + rr := NewRegistry(nil) + parent := "host:linked" + incoming := Resource{Type: ResourceTypePhysicalDisk, ParentID: &parent, PhysicalDisk: &PhysicalDiskMeta{DevPath: "sdx", DiskType: "usb"}} + for _, id := range []string{"a", "b"} { + rr.resources[id] = &Resource{ID: id, Type: ResourceTypePhysicalDisk, ParentID: &parent, Sources: []DataSource{other}, PhysicalDisk: &PhysicalDiskMeta{DevPath: "/dev/sdx", Serial: "SERIAL-" + id, DiskType: "usb"}} + } + if got := rr.resolveLinkedPhysicalDisk(source, incoming); got != "" { + t.Fatalf("ambiguous path matched %q", got) + } + delete(rr.resources, "b") + if got := rr.resolveLinkedPhysicalDisk(source, incoming); got != "a" { + t.Fatalf("unique missing-identity alias matched %q", got) + } + incoming.PhysicalDisk.WWN = "DIFFERENT-WWN" + if got := rr.resolveLinkedPhysicalDisk(source, incoming); got != "" { + t.Fatalf("conflicting hardware identity matched %q", got) + } + }) + } +}