From 509ab2fc6d78c043c306f85149c3242e8d64d257 Mon Sep 17 00:00:00 2001 From: eriknordmark Date: Thu, 16 Jul 2026 18:23:37 -0700 Subject: [PATCH 1/2] pillar: report I/O-bundle errors per assignment group Rework how errors recorded on an assignable-adapter (I/O bundle) are modeled and reported to the controller. Each entry now carries a warning vs hard-error classification and a group-scoped vs member-scoped scope, and every source reconciles only its own entries via SetSourceErrors so a persistent error keeps a stable timestamp and one source can no longer clear another's. zedagent aggregates a group's members into the single ZioBundle error slot with the appropriate severity: group-scoped entries (such as a PCI/USB collision) are reported once and unattributed, member-scoped entries are attributed to their member. The existing collision and assignment-group-conflict checks are converted to this model. No device behavior changes. Signed-off-by: eriknordmark Co-Authored-By: Claude Opus 4.8 --- pkg/pillar/cmd/zedagent/reportinfo.go | 54 ++- pkg/pillar/types/assignableadapters.go | 382 +++++++++++++++----- pkg/pillar/types/assignableadapters_test.go | 133 ++++++- 3 files changed, 473 insertions(+), 96 deletions(-) diff --git a/pkg/pillar/cmd/zedagent/reportinfo.go b/pkg/pillar/cmd/zedagent/reportinfo.go index 09c98b07b13..cb813338a97 100644 --- a/pkg/pillar/cmd/zedagent/reportinfo.go +++ b/pkg/pillar/cmd/zedagent/reportinfo.go @@ -535,12 +535,18 @@ func PublishDeviceInfoToZedCloud(ctx *zedagentContext, dest destinationBitset) { } else if ib.KeepInHost { reportAA.UsedByBaseOS = true } - if !ib.Error.Empty() { + // Combine all group members' errors into the single ZioBundle error slot. + aggErr := types.AggregateIoBundleGroupErrors(list) + if !aggErr.Empty { errInfo := new(info.ErrorInfo) - errInfo.Description = ib.Error.String() - errInfo.Severity = info.Severity_SEVERITY_ERROR - if !ib.Error.ErrorTime().IsZero() { - errInfo.Timestamp = timestamppb.New(ib.Error.ErrorTime()) + errInfo.Description = aggErr.Description + if aggErr.OnlyWarnings { + errInfo.Severity = info.Severity_SEVERITY_WARNING + } else { + errInfo.Severity = info.Severity_SEVERITY_ERROR + } + if !aggErr.ErrorTime.IsZero() { + errInfo.Timestamp = timestamppb.New(aggErr.ErrorTime) } reportAA.Err = errInfo } @@ -893,6 +899,11 @@ func encodeNetworkPortStatus(ctx *zedagentContext, devicePort.Err = encodeTestResults(port.TestResults) if ioBundle != nil { devicePort.Usage = ioBundle.Usage + // A port that is also an assignable adapter can carry model / + // pciback errors on its IoBundle in addition to any connectivity + // error from network testing. Fold those into the port error so + // they surface in the port view, not only under assignableAdapters. + mergeIoBundleErrIntoPort(devicePort.Err, ioBundle) } devicePort.Cost = uint32(port.Cost) devicePort.IsMgmt = port.IsMgmt @@ -1008,6 +1019,39 @@ func encodeNetworkPortStatus(ctx *zedagentContext, return devicePort } +// mergeIoBundleErrIntoPort folds an assignable-adapter port's IoBundle error +// (model inconsistency, rename, pciback op, ...) into the port ErrorInfo, which +// otherwise carries only the network TestResults error. Descriptions are +// concatenated and the reported severity is the more severe of the two, so a +// model error is never masked by a healthy network result and vice versa. +func mergeIoBundleErrIntoPort(errInfo *info.ErrorInfo, ioBundle *types.IoBundle) { + aggErr := types.AggregateIoBundleGroupErrors([]*types.IoBundle{ioBundle}) + if aggErr.Empty { + return + } + bundleSeverity := info.Severity_SEVERITY_ERROR + if aggErr.OnlyWarnings { + bundleSeverity = info.Severity_SEVERITY_WARNING + } + if errInfo.Description == "" { + // Port carries no network error; adopt the IoBundle error wholesale. + errInfo.Description = aggErr.Description + errInfo.Severity = bundleSeverity + if !aggErr.ErrorTime.IsZero() { + errInfo.Timestamp = timestamppb.New(aggErr.ErrorTime) + } + return + } + errInfo.Description += "; " + aggErr.Description + if errInfo.Severity < bundleSeverity { + errInfo.Severity = bundleSeverity + } + if !aggErr.ErrorTime.IsZero() && + (errInfo.Timestamp == nil || aggErr.ErrorTime.After(errInfo.Timestamp.AsTime())) { + errInfo.Timestamp = timestamppb.New(aggErr.ErrorTime) + } +} + func encodeBondStatus(bs *types.BondStatus) *info.BondStatus { pbBond := &info.BondStatus{ Mode: evecommon.BondMode(bs.Mode), diff --git a/pkg/pillar/types/assignableadapters.go b/pkg/pillar/types/assignableadapters.go index a017c9f2535..46acdba8452 100644 --- a/pkg/pillar/types/assignableadapters.go +++ b/pkg/pillar/types/assignableadapters.go @@ -32,6 +32,12 @@ type AssignableAdapters struct { type ioBundleErrorBase struct { ErrStr string `json:",omitempty"` TypeStr string `json:",omitempty"` + // Warning marks an advisory entry: a model inconsistency EVE worked around. + // Reported to the controller as a warning, not an error. + Warning bool `json:",omitempty"` + // GroupScoped marks an entry describing the whole assignment group (e.g. a + // collision). Stored on every member but reported once, without attribution. + GroupScoped bool `json:",omitempty"` } func (i ioBundleErrorBase) Error() string { @@ -60,21 +66,106 @@ func (iobe *IOBundleError) String() string { return strings.Join(errorStrings, "; ") } -// Append converts an error to ioBundleErrorBase and adds it -func (iobe *IOBundleError) Append(err error) { +// appendEntry adds entry unless an identical one exists, refreshing the +// timestamp. Dedup keeps the list bounded when a condition is re-detected. +func (iobe *IOBundleError) appendEntry(entry ioBundleErrorBase) { if iobe.Errors == nil { iobe.Errors = make([]ioBundleErrorBase, 0, 1) } + for _, e := range iobe.Errors { + if e.ErrStr == entry.ErrStr && e.TypeStr == entry.TypeStr && + e.Warning == entry.Warning && e.GroupScoped == entry.GroupScoped { + // Already present; leave the timestamp so a persistent error + // keeps its original time. + return + } + } + iobe.Errors = append(iobe.Errors, entry) + iobe.TimeOfError = time.Now() +} - typeStr := reflect.TypeOf(err).String() - baseErr := ioBundleErrorBase{ - ErrStr: err.Error(), - TypeStr: typeStr, +// SetSourceErrors reconciles the entries owned by owner's type to exactly the +// desired strings (all classified alike by warning/groupScoped). Unchanged +// entries are left in place; TimeOfError advances only when an entry is added, +// and resets when the last entry is removed. An empty desired clears the source. +// This lets each source refresh its own errors every reconciliation pass without +// churning the timestamp of a persistent error or touching other sources. +// Returns true if the entry set changed. +func (iobe *IOBundleError) SetSourceErrors(owner error, warning, groupScoped bool, desired []string) bool { + typeStr := reflect.TypeOf(owner).String() + want := make(map[string]bool, len(desired)) + for _, s := range desired { + want[s] = true + } + have := make(map[string]bool) + changed := false + kept := iobe.Errors[:0] + for _, e := range iobe.Errors { + if e.TypeStr == typeStr && !want[e.ErrStr] { + changed = true // stale entry of this source: drop + continue + } + if e.TypeStr == typeStr { + have[e.ErrStr] = true + } + kept = append(kept, e) + } + iobe.Errors = kept + added := false + for _, s := range desired { + if !have[s] { + iobe.Errors = append(iobe.Errors, ioBundleErrorBase{ + ErrStr: s, TypeStr: typeStr, Warning: warning, GroupScoped: groupScoped, + }) + added = true + } + } + if len(iobe.Errors) == 0 { + iobe.TimeOfError = time.Time{} + } else if added { + iobe.TimeOfError = time.Now() } + return changed || added +} + +// Append adds a member-scoped hard error. +func (iobe *IOBundleError) Append(err error) { + iobe.appendEntry(ioBundleErrorBase{ + ErrStr: err.Error(), + TypeStr: reflect.TypeOf(err).String(), + }) +} - iobe.Errors = append(iobe.Errors, baseErr) +// AppendWarning adds a member-scoped advisory warning (see Warning). +func (iobe *IOBundleError) AppendWarning(err error) { + iobe.appendEntry(ioBundleErrorBase{ + ErrStr: err.Error(), + TypeStr: reflect.TypeOf(err).String(), + Warning: true, + }) +} - iobe.TimeOfError = time.Now() +// AppendGroupError adds a group-scoped hard error (see GroupScoped). +func (iobe *IOBundleError) AppendGroupError(err error) { + iobe.appendEntry(ioBundleErrorBase{ + ErrStr: err.Error(), + TypeStr: reflect.TypeOf(err).String(), + GroupScoped: true, + }) +} + +// IsOnlyWarnings returns true if there is at least one entry and every entry is +// an advisory warning (no hard errors). Used to pick the reported severity. +func (iobe *IOBundleError) IsOnlyWarnings() bool { + if len(iobe.Errors) == 0 { + return false + } + for _, err := range iobe.Errors { + if !err.Warning { + return false + } + } + return true } // Empty returns true if no error has been added @@ -117,12 +208,89 @@ func (iobe *IOBundleError) removeByType(e error) { } } +// RemoveByType clears entries of type e, leaving other errors and warnings. +func (iobe *IOBundleError) RemoveByType(e error) { + iobe.removeByType(e) +} + // Clear clears all errors func (iobe *IOBundleError) Clear() { iobe.Errors = make([]ioBundleErrorBase, 0) iobe.TimeOfError = time.Time{} } +// AggregatedIoBundleError is the combined error state of an assignment group, +// ready for reporting in a single ZioBundle. +type AggregatedIoBundleError struct { + Description string // combined text + OnlyWarnings bool // every entry is a warning (picks WARNING vs ERROR) + Empty bool // no member carries an entry + ErrorTime time.Time // most recent error time across members +} + +// AggregateIoBundleGroupErrors combines a group's members' entries for reporting: +// group-scoped entries once and unattributed, member-scoped entries prefixed with +// their member's label. Duplicates are suppressed; nil members ignored. +func AggregateIoBundleGroupErrors(members []*IoBundle) AggregatedIoBundleError { + var parts []string + seenGroup := map[string]bool{} + onlyWarnings := true + anyEntry := false + var latest time.Time + // Group-scoped entries first, deduplicated across members. + for _, m := range members { + if m == nil { + continue + } + if m.Error.TimeOfError.After(latest) { + latest = m.Error.TimeOfError + } + for _, e := range m.Error.Errors { + if !e.GroupScoped { + continue + } + key := e.TypeStr + "\x00" + e.ErrStr + if seenGroup[key] { + continue + } + seenGroup[key] = true + anyEntry = true + if !e.Warning { + onlyWarnings = false + } + parts = append(parts, e.ErrStr) + } + } + // Member-scoped entries, attributed to the owning member. + for _, m := range members { + if m == nil { + continue + } + seenMember := map[string]bool{} + for _, e := range m.Error.Errors { + if e.GroupScoped { + continue + } + key := e.TypeStr + "\x00" + e.ErrStr + if seenMember[key] { + continue + } + seenMember[key] = true + anyEntry = true + if !e.Warning { + onlyWarnings = false + } + parts = append(parts, fmt.Sprintf("%s: %s", m.Logicallabel, e.ErrStr)) + } + } + return AggregatedIoBundleError{ + Description: strings.Join(parts, "; "), + OnlyWarnings: anyEntry && onlyWarnings, + Empty: !anyEntry, + ErrorTime: latest, + } +} + // IoBundle has one entry per individual receptacle with a reference // to a group name. Those sharing a group name needs to be assigned // together. @@ -604,76 +772,103 @@ func (ErrCycleDetected) Error() string { return "Cycle detected, please check provided parentassigngrp/assigngrp" } -// CheckParentAssigngrp finds dependency loops between ioBundles and sets/clears the error +// The following empty types are owner markers for SetSourceErrors: they identify +// which source produced an entry so each source clears only its own. + +// ErrIoBundleAssignmentGroupConflict owns CheckBadAssignmentGroups errors. +type ErrIoBundleAssignmentGroupConflict struct{} + +func (ErrIoBundleAssignmentGroupConflict) Error() string { return "assignment-group conflict" } + +// ErrIoBundleModelInconsistency owns updatePortAndPciBackIoBundle warnings. +type ErrIoBundleModelInconsistency struct{} + +func (ErrIoBundleModelInconsistency) Error() string { return "device-model inconsistency" } + +// ErrIoBundleRename owns the interface-rename warning from IoBundleToPci. +type ErrIoBundleRename struct{} + +func (ErrIoBundleRename) Error() string { return "interface renamed to match model" } + +// ErrIoBundlePcibackOp owns errors from moving a device in/out of pciback. +type ErrIoBundlePcibackOp struct{} + +func (ErrIoBundlePcibackOp) Error() string { return "pciback operation failed" } + +// ErrIoBundleMissingDevice means the device backing an IoBundle was not found. +// Typed so callers can clear it (RemoveByType) once resolvable, keeping warnings. +type ErrIoBundleMissingDevice struct { + msg string +} + +func (e ErrIoBundleMissingDevice) Error() string { + return e.msg +} + +// CheckParentAssigngrp validates the parentassigngrp/assigngrp graph and records +// the applicable per-bundle error (self-parent, parent mismatch, empty-group with +// parent, or a dependency cycle). Errors are reconciled through SetSourceErrors so +// a persistent error keeps a stable timestamp across reconciliation passes instead +// of being churned by a remove-then-re-add; a churning timestamp republishes +// AssignableAdapters every pass and spins nim's DPC verification. Returns true if +// the error set changed. func (aa *AssignableAdapters) CheckParentAssigngrp() bool { assigngrp2parent := make(map[string]string) + ownParent := make(map[int]bool) + mismatch := make(map[int]bool) + emptyWithParent := make(map[int]bool) for i := range aa.IoBundleList { - ioBundle := &aa.IoBundleList[i] - for _, parentAssigngrpErr := range []error{ - ErrOwnParent{}, - ErrParentAssigngrpMismatch{}, - ErrEmptyAssigngrpWithParent{}, - ErrCycleDetected{}, - } { - ioBundle.Error.removeByType(parentAssigngrpErr) - } - } - - var cycleDetectedAssigngrp string - for i := range aa.IoBundleList { - ioBundle := &aa.IoBundleList[i] - - if ioBundle.AssignmentGroup == ioBundle.ParentAssignmentGroup && ioBundle.AssignmentGroup != "" { - ioBundle.Error.Append(ErrOwnParent{}) - return true + ib := &aa.IoBundleList[i] + if ib.AssignmentGroup == ib.ParentAssignmentGroup && ib.AssignmentGroup != "" { + ownParent[i] = true + continue } - parentassigngrp, ok := assigngrp2parent[ioBundle.AssignmentGroup] - if ok && parentassigngrp != ioBundle.ParentAssignmentGroup { - ioBundle.Error.Append(ErrParentAssigngrpMismatch{}) - return true + if parent, ok := assigngrp2parent[ib.AssignmentGroup]; ok && parent != ib.ParentAssignmentGroup { + mismatch[i] = true + continue } - - if ioBundle.AssignmentGroup == "" && ioBundle.ParentAssignmentGroup != "" { - ioBundle.Error.Append(ErrEmptyAssigngrpWithParent{}) - return true + if ib.AssignmentGroup == "" && ib.ParentAssignmentGroup != "" { + emptyWithParent[i] = true + continue } - assigngrp2parent[ioBundle.AssignmentGroup] = ioBundle.ParentAssignmentGroup + assigngrp2parent[ib.AssignmentGroup] = ib.ParentAssignmentGroup } + // A group is in a cycle if following parent links from it returns to an + // already-visited group. Self-parents are excluded above, so they are + // reported as ErrOwnParent rather than ErrCycleDetected. + cycleGroups := make(map[string]bool) for assigngrp := range assigngrp2parent { - visitedAssigngrp := make(map[string]struct{}) - visitedAssigngrp[assigngrp] = struct{}{} - - for { - if assigngrp == "" { + visited := map[string]struct{}{assigngrp: {}} + for g := assigngrp; g != ""; { + g = assigngrp2parent[g] + if _, seen := visited[g]; seen { + cycleGroups[g] = true break } - - assigngrp = assigngrp2parent[assigngrp] - _, visitedBefore := visitedAssigngrp[assigngrp] - if visitedBefore { - // cycle detected - cycleDetectedAssigngrp = assigngrp - break - } - - visitedAssigngrp[assigngrp] = struct{}{} + visited[g] = struct{}{} } } - if cycleDetectedAssigngrp == "" { - return false + changed := false + setErr := func(ib *IoBundle, owner error, want bool) { + var desired []string + if want { + desired = []string{owner.Error()} + } + if ib.Error.SetSourceErrors(owner, false, false, desired) { + changed = true + } } - for i := range aa.IoBundleList { - ioBundle := &aa.IoBundleList[i] - if ioBundle.AssignmentGroup == cycleDetectedAssigngrp { - ioBundle.Error.Append(ErrCycleDetected{}) - } + ib := &aa.IoBundleList[i] + setErr(ib, ErrOwnParent{}, ownParent[i]) + setErr(ib, ErrParentAssigngrpMismatch{}, mismatch[i]) + setErr(ib, ErrEmptyAssigngrpWithParent{}, emptyWithParent[i]) + setErr(ib, ErrCycleDetected{}, cycleGroups[ib.AssignmentGroup]) } - - return true + return changed } // IOBundleCollision has the members IoBundles can collide on @@ -686,7 +881,23 @@ type IOBundleCollision struct { } func (i IOBundleCollision) String() string { - return fmt.Sprintf("phylabel %s - usbaddr: %s usbproduct: %s pcilong: %s assigngrp: %s", i.Phylabel, i.USBAddr, i.USBProduct, i.PCILong, i.Assigngrp) + var parts []string + if i.USBAddr != "" { + parts = append(parts, "usbaddr "+i.USBAddr) + } + if i.USBProduct != "" { + parts = append(parts, "usbproduct "+i.USBProduct) + } + if i.PCILong != "" { + parts = append(parts, "pcilong "+i.PCILong) + } + if i.Assigngrp != "" { + parts = append(parts, "assigngrp "+i.Assigngrp) + } + if len(parts) == 0 { + return i.Phylabel + } + return fmt.Sprintf("%s (%s)", i.Phylabel, strings.Join(parts, ", ")) } // ErrIOBundleCollision describes an error where an IoBundle collides with another IoBundle @@ -695,15 +906,11 @@ type ErrIOBundleCollision struct { } func (i ErrIOBundleCollision) Error() string { - collisionErrStrPrefix := "ioBundle collision:" - collisionStrs := make([]string, 0, len(i.Collisions)) for _, collision := range i.Collisions { collisionStrs = append(collisionStrs, collision.String()) } - collisionErrStrBody := strings.Join(collisionStrs, "||") - - return fmt.Sprintf("%s||%s||", collisionErrStrPrefix, collisionErrStrBody) + return "ioBundle collision: " + strings.Join(collisionStrs, "; ") } func newIoBundleCollisionErr() ErrIOBundleCollision { @@ -715,11 +922,6 @@ func newIoBundleCollisionErr() ErrIOBundleCollision { // CheckBadUSBBundles sets and clears ib.Error/ErrorTime if bundle collides in regards of USB func (aa *AssignableAdapters) CheckBadUSBBundles() { usbProductsAddressMap := make(map[[4]string][]*IoBundle) - for i := range aa.IoBundleList { - ioBundle := &aa.IoBundleList[i] - ioBundle.Error.removeByType(ErrIOBundleCollision{}) - } - for i := range aa.IoBundleList { ioBundle := &aa.IoBundleList[i] if ioBundle.UsbAddr == "" && ioBundle.UsbProduct == "" && ioBundle.PciLong == "" { @@ -727,19 +929,17 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() { } id := [4]string{ioBundle.UsbAddr, ioBundle.UsbProduct, ioBundle.PciLong, ioBundle.AssignmentGroup} - if usbProductsAddressMap[id] == nil { - usbProductsAddressMap[id] = make([]*IoBundle, 0) - } usbProductsAddressMap[id] = append(usbProductsAddressMap[id], ioBundle) } + // Collision text per colliding bundle (a group-scoped error listing every + // colliding member; identical for all members of the collision). + collisionText := make(map[*IoBundle]string) for _, bundles := range usbProductsAddressMap { if len(bundles) <= 1 { continue } - collisionErr := newIoBundleCollisionErr() - for _, bundle := range bundles { collisionErr.Collisions = append(collisionErr.Collisions, IOBundleCollision{ Phylabel: bundle.Phylabel, @@ -750,9 +950,18 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() { }) } for _, bundle := range bundles { - bundle.Error.Append(collisionErr) + collisionText[bundle] = collisionErr.Error() } } + + for i := range aa.IoBundleList { + ib := &aa.IoBundleList[i] + var desired []string + if s, ok := collisionText[ib]; ok { + desired = []string{s} + } + ib.Error.SetSourceErrors(ErrIOBundleCollision{}, false, true, desired) + } } // CheckBadAssignmentGroups sets ib.Error/ErrorTime if two IoBundles in different @@ -762,6 +971,7 @@ func (aa *AssignableAdapters) CheckBadAssignmentGroups(log *base.LogObject, PCIS changed := false for i := range aa.IoBundleList { ib := &aa.IoBundleList[i] + var desired []string for _, ib2 := range aa.IoBundleList { if ib2.Phylabel == ib.Phylabel { continue @@ -777,13 +987,15 @@ func (aa *AssignableAdapters) CheckBadAssignmentGroups(log *base.LogObject, PCIS continue } if PCISameController != nil && PCISameController(ib.PciLong, ib2.PciLong) { - err := fmt.Errorf("CheckBadAssignmentGroup: %s same PCI controller as %s; pci long %s vs %s", + s := fmt.Sprintf("CheckBadAssignmentGroup: %s same PCI controller as %s; pci long %s vs %s", ib2.Ifname, ib.Ifname, ib2.PciLong, ib.PciLong) - log.Error(err) - ib.Error.Append(err) - changed = true + log.Error(s) + desired = append(desired, s) } } + if ib.Error.SetSourceErrors(ErrIoBundleAssignmentGroupConflict{}, false, true, desired) { + changed = true + } } return changed || aa.CheckParentAssigngrp() @@ -817,8 +1029,12 @@ func (aa *AssignableAdapters) ExpandControllers(log *base.LogObject, list []*IoB continue } if PCISameController != nil && PCISameController(ib.PciLong, ib2.PciLong) { - log.Warnf("ExpandController found %s matching %s; long %s long %s", - ib2.Phylabel, ib.Phylabel, ib2.PciLong, ib.PciLong) + log.Warnf("ExpandController: adapter %s (logicallabel %s, ifname %q, "+ + "PCI %s) shares a PCI controller with %s (logicallabel %s, ifname %q, "+ + "PCI %s) and is pulled into the same assignment group %q, which the "+ + "controller's model did not include", + ib2.Phylabel, ib2.Logicallabel, ib2.Ifname, ib2.PciLong, + ib.Phylabel, ib.Logicallabel, ib.Ifname, ib.PciLong, ib.AssignmentGroup) elist = append(elist, ib2) } } diff --git a/pkg/pillar/types/assignableadapters_test.go b/pkg/pillar/types/assignableadapters_test.go index a71393eb34f..eeaed602f83 100644 --- a/pkg/pillar/types/assignableadapters_test.go +++ b/pkg/pillar/types/assignableadapters_test.go @@ -7,6 +7,7 @@ import ( "bytes" "errors" "fmt" + "strings" "testing" "time" @@ -541,6 +542,122 @@ func TestExpandControllers(t *testing.T) { } } +// TestIOBundleErrorWarning covers the advisory-warning path used to report +// device-model inconsistencies EVE works around: IsOnlyWarnings distinguishes a +// warnings-only bundle from one that also carries a hard error. +func TestIOBundleErrorWarning(t *testing.T) { + var e IOBundleError + assert.False(t, e.IsOnlyWarnings(), "empty error should not be only-warnings") + + e.AppendWarning(errors.New("model ifname does not match kernel; matched by PCI")) + assert.False(t, e.Empty()) + assert.True(t, e.IsOnlyWarnings(), "a lone warning should be only-warnings") + assert.True(t, strings.Contains(e.String(), "matched by PCI")) + + e.Append(errors.New("hard error")) + assert.False(t, e.IsOnlyWarnings(), "a hard error must downgrade from only-warnings") +} + +// TestAppendEntryDedup covers the duplicate-suppression in the append path: +// re-adding an identical entry does not grow the list, but entries differing in +// text, warning flag, or group scope are kept distinct. +func TestAppendEntryDedup(t *testing.T) { + var e IOBundleError + e.Append(errors.New("same")) + e.Append(errors.New("same")) + assert.Equal(t, 1, len(e.Errors), "identical hard errors should dedup") + + e.AppendWarning(errors.New("same")) + assert.Equal(t, 2, len(e.Errors), "warning differs from hard error of same text") + + e.AppendGroupError(errors.New("same")) + assert.Equal(t, 3, len(e.Errors), "group-scoped differs from member-scoped") + + e.AppendGroupError(errors.New("same")) + assert.Equal(t, 3, len(e.Errors), "identical group-scoped errors should dedup") +} + +// TestRemoveByTypePreservesWarning verifies that clearing a specific error type +// leaves advisory warnings (and other error types) in place. +func TestRemoveByTypePreservesWarning(t *testing.T) { + var e IOBundleError + e.AppendWarning(errors.New("renamed to match model")) + e.Append(ErrIoBundleMissingDevice{msg: "PCI device does not exist"}) + assert.Equal(t, 2, len(e.Errors)) + + e.RemoveByType(ErrIoBundleMissingDevice{}) + assert.Equal(t, 1, len(e.Errors), "only the missing-device error should be removed") + assert.True(t, e.IsOnlyWarnings(), "the surviving entry is the warning") + assert.Contains(t, e.String(), "renamed to match model") +} + +// TestAggregateIoBundleGroupErrors covers the reporting aggregation: group-scoped +// entries reported once without attribution, member-scoped entries attributed to +// their member, and severity derived from whether every entry is a warning. +func TestAggregateIoBundleGroupErrors(t *testing.T) { + // Empty group. + agg := AggregateIoBundleGroupErrors(nil) + assert.True(t, agg.Empty) + assert.False(t, agg.OnlyWarnings) + + // A group-scoped collision stored (identically) on both members must be + // reported once, unattributed; a member-scoped warning on one member must be + // attributed to that member. + m1 := &IoBundle{Logicallabel: "eth0"} + m2 := &IoBundle{Logicallabel: "eth1"} + m1.Error.AppendGroupError(errors.New("pci collision among group members")) + m2.Error.AppendGroupError(errors.New("pci collision among group members")) + m1.Error.AppendWarning(errors.New("renamed to match model")) + + agg = AggregateIoBundleGroupErrors([]*IoBundle{m1, m2}) + assert.False(t, agg.Empty) + // The collision is a hard (group-scoped) error, so severity is error. + assert.False(t, agg.OnlyWarnings) + assert.Equal(t, 1, strings.Count(agg.Description, "pci collision among group members"), + "group-scoped entry reported exactly once") + assert.Contains(t, agg.Description, "eth0: renamed to match model", + "member-scoped entry attributed to its member") + + // Warnings-only group -> OnlyWarnings true. + w1 := &IoBundle{Logicallabel: "eth0"} + w1.Error.AppendWarning(errors.New("matched by PCI")) + aggW := AggregateIoBundleGroupErrors([]*IoBundle{w1}) + assert.True(t, aggW.OnlyWarnings) + assert.Contains(t, aggW.Description, "eth0: matched by PCI") +} + +// TestSetSourceErrors covers the reconcile semantics a source uses to refresh +// its own entries each pass: add/keep/remove, a stable timestamp while the set +// is unchanged (or only shrinks), a bump on add, reset when emptied, and no +// effect on other sources' entries. +func TestSetSourceErrors(t *testing.T) { + var e IOBundleError + owner := ErrIoBundleModelInconsistency{} + + // Initial add. + assert.True(t, e.SetSourceErrors(owner, true, false, []string{"a", "b"})) + assert.Equal(t, 2, len(e.Errors)) + t0 := e.TimeOfError + assert.False(t, t0.IsZero()) + + // Re-set with the same desired set: no change, timestamp untouched. + assert.False(t, e.SetSourceErrors(owner, true, false, []string{"a", "b"})) + assert.Equal(t, t0, e.TimeOfError, "unchanged set must not move the timestamp") + + // Remove one (no add): changed, but timestamp not bumped. + assert.True(t, e.SetSourceErrors(owner, true, false, []string{"a"})) + assert.Equal(t, 1, len(e.Errors)) + assert.Equal(t, t0, e.TimeOfError, "removal-only must not bump the timestamp") + + // A different source's entry is untouched by this source's reconcile. + e.Append(ErrOwnParent{}) + assert.True(t, e.HasErrorByType(ErrOwnParent{})) + e.SetSourceErrors(owner, true, false, nil) // clear this source + assert.False(t, e.HasErrorByType(owner), "own entries cleared") + assert.True(t, e.HasErrorByType(ErrOwnParent{}), "other source preserved") + assert.False(t, e.TimeOfError.IsZero(), "still has the other source's error") +} + func alternativeCheckBadUSBBundlesImpl(bundles []IoBundle) { for i := range bundles { for j := range bundles { @@ -855,11 +972,11 @@ func TestCheckBadUSBBundles(t *testing.T) { bundleWithError: []bundleWithError{ { bundle: IoBundle{Phylabel: "1", UsbAddr: "1:1", UsbProduct: "a:a", PciLong: "1:1"}, - expectedError: "ioBundle collision:||phylabel 1 - usbaddr: 1:1 usbproduct: a:a pcilong: 1:1 assigngrp: ||phylabel 2 - usbaddr: 1:1 usbproduct: a:a pcilong: 1:1 assigngrp: ||", + expectedError: "ioBundle collision: 1 (usbaddr 1:1, usbproduct a:a, pcilong 1:1); 2 (usbaddr 1:1, usbproduct a:a, pcilong 1:1)", }, { bundle: IoBundle{Phylabel: "2", UsbAddr: "1:1", UsbProduct: "a:a", PciLong: "1:1"}, - expectedError: "ioBundle collision:||phylabel 1 - usbaddr: 1:1 usbproduct: a:a pcilong: 1:1 assigngrp: ||phylabel 2 - usbaddr: 1:1 usbproduct: a:a pcilong: 1:1 assigngrp: ||", + expectedError: "ioBundle collision: 1 (usbaddr 1:1, usbproduct a:a, pcilong 1:1); 2 (usbaddr 1:1, usbproduct a:a, pcilong 1:1)", }, }, }, @@ -867,11 +984,11 @@ func TestCheckBadUSBBundles(t *testing.T) { bundleWithError: []bundleWithError{ { bundle: IoBundle{Phylabel: "3", UsbAddr: "1:1", UsbProduct: "a:a"}, - expectedError: "ioBundle collision:||phylabel 3 - usbaddr: 1:1 usbproduct: a:a pcilong: assigngrp: ||phylabel 4 - usbaddr: 1:1 usbproduct: a:a pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 3 (usbaddr 1:1, usbproduct a:a); 4 (usbaddr 1:1, usbproduct a:a)", }, { bundle: IoBundle{Phylabel: "4", UsbAddr: "1:1", UsbProduct: "a:a"}, - expectedError: "ioBundle collision:||phylabel 3 - usbaddr: 1:1 usbproduct: a:a pcilong: assigngrp: ||phylabel 4 - usbaddr: 1:1 usbproduct: a:a pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 3 (usbaddr 1:1, usbproduct a:a); 4 (usbaddr 1:1, usbproduct a:a)", }, { bundle: IoBundle{Phylabel: "5", UsbAddr: "1:1", UsbProduct: ""}, @@ -883,11 +1000,11 @@ func TestCheckBadUSBBundles(t *testing.T) { bundleWithError: []bundleWithError{ { bundle: IoBundle{Phylabel: "6", UsbAddr: "1:1", UsbProduct: ""}, - expectedError: "ioBundle collision:||phylabel 6 - usbaddr: 1:1 usbproduct: pcilong: assigngrp: ||phylabel 7 - usbaddr: 1:1 usbproduct: pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 6 (usbaddr 1:1); 7 (usbaddr 1:1)", }, { bundle: IoBundle{Phylabel: "7", UsbAddr: "1:1", UsbProduct: ""}, - expectedError: "ioBundle collision:||phylabel 6 - usbaddr: 1:1 usbproduct: pcilong: assigngrp: ||phylabel 7 - usbaddr: 1:1 usbproduct: pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 6 (usbaddr 1:1); 7 (usbaddr 1:1)", }, }, }, @@ -895,11 +1012,11 @@ func TestCheckBadUSBBundles(t *testing.T) { bundleWithError: []bundleWithError{ { bundle: IoBundle{Phylabel: "8", UsbAddr: "", UsbProduct: "a:a"}, - expectedError: "ioBundle collision:||phylabel 8 - usbaddr: usbproduct: a:a pcilong: assigngrp: ||phylabel 9 - usbaddr: usbproduct: a:a pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 8 (usbproduct a:a); 9 (usbproduct a:a)", }, { bundle: IoBundle{Phylabel: "9", UsbAddr: "", UsbProduct: "a:a"}, - expectedError: "ioBundle collision:||phylabel 8 - usbaddr: usbproduct: a:a pcilong: assigngrp: ||phylabel 9 - usbaddr: usbproduct: a:a pcilong: assigngrp: ||", + expectedError: "ioBundle collision: 8 (usbproduct a:a); 9 (usbproduct a:a)", }, }, }, From c9ebaa0c2674dd03fb881b7968e16a0baacaf02d Mon Sep 17 00:00:00 2001 From: eriknordmark Date: Thu, 16 Jul 2026 18:37:15 -0700 Subject: [PATCH 2/2] domainmgr: keep in-use ports out of pciback Make EVE robust against a device model whose PhysicalIO disagrees with what the kernel presents. A network port already in use is now matched to its device by PCI address as well as interface name, so a port is kept in the host (not reserved to pciback/vfio-pci) even when the kernel-assigned name differs from the model (e.g. ethN vs enpNsN), and even when the model declares a non-network type for the PCI address that in fact backs a live port. Each such adjustment, and an interface rename forced to match the model, is reported to the controller as an advisory warning. The port-vs-pciback loop now reconciles only its own errors instead of clearing the whole bundle every pass, so the collision and assignment-group-conflict errors and the advisory warnings survive to the reported info. Cellular ports take their PCI address from the wwan microservice, which resolves a modem's address where the interface-based lookup returns the enclosing PCIe bridge. Signed-off-by: eriknordmark Co-Authored-By: Claude Opus 4.8 --- pkg/pillar/cmd/domainmgr/domainmgr.go | 72 ++++++++++++++++--- pkg/pillar/cmd/domainmgr/domainmgr_test.go | 49 +++++++++++++ pkg/pillar/dpcmanager/dns.go | 19 +++++ pkg/pillar/dpcmanager/dpcmanager_test.go | 77 +++++++++++++++++++++ pkg/pillar/types/assignableadapters.go | 6 -- pkg/pillar/types/assignableadapters_test.go | 3 +- pkg/pillar/types/dns.go | 25 +++++-- pkg/pillar/types/dns_test.go | 49 +++++++++++-- pkg/pillar/types/ifnametopci.go | 38 ++++++---- pkg/pillar/types/ifnametopci_test.go | 52 ++++++++++++++ 10 files changed, 348 insertions(+), 42 deletions(-) diff --git a/pkg/pillar/cmd/domainmgr/domainmgr.go b/pkg/pillar/cmd/domainmgr/domainmgr.go index 8ac5e6b9643..38ae4ce07c5 100644 --- a/pkg/pillar/cmd/domainmgr/domainmgr.go +++ b/pkg/pillar/cmd/domainmgr/domainmgr.go @@ -99,7 +99,7 @@ var currentTTY = 0 func isPort(ctx *domainContext, ifname string) bool { ctx.dnsLock.Lock() defer ctx.dnsLock.Unlock() - return types.IsPort(ctx.deviceNetworkStatus, ifname) + return types.IsPort(ctx.deviceNetworkStatus, ifname, "") } // Information for handleCreate/Modify/Delete @@ -3573,11 +3573,11 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string, ib := types.IoBundleFromPhyAdapter(log, phyAdapter) // Fill in PCIlong, macaddr, unique _, err := checkAndFillIoBundle(ib) + var missing []string if err != nil { - ib.Error.Append(err) - } else { - ib.Error.Clear() + missing = []string{err.Error()} } + ib.Error.SetSourceErrors(types.ErrIoBundleMissingDevice{}, false, false, missing) // We assume AddOrUpdateIoBundle will preserve any // existing IsPort/IsPCIBack/UsedByUUID aa.AddOrUpdateIoBundle(log, *ib) @@ -3643,11 +3643,11 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string, ib := types.IoBundleFromPhyAdapter(log, phyAdapter) // Fill in PCIlong, macaddr, unique _, err := checkAndFillIoBundle(ib) + var missing []string if err != nil { - ib.Error.Append(err) - } else { - ib.Error.Clear() + missing = []string{err.Error()} } + ib.Error.SetSourceErrors(types.ErrIoBundleMissingDevice{}, false, false, missing) currentIbPtr := aa.LookupIoBundlePhylabel(phyAdapter.Phylabel) if currentIbPtr == nil || currentIbPtr.HasAdapterChanged(log, phyAdapter) { @@ -3850,6 +3850,17 @@ func updatePortAndPciBackIoBundle(ctx *domainContext, ib *types.IoBundle) (chang list = append(list, ib) } + // The group being processed, and the members the controller's model actually + // listed in it, so members EVE adds via ExpandControllers can be flagged. + reqGroup := ib.AssignmentGroup + origGroup := make(map[*types.IoBundle]bool, len(list)) + for _, m := range list { + origGroup[m] = true + } + // Per-bundle advisories for model inconsistencies worked around below; + // recorded as warnings on ib.Error so the controller is informed. + modelWarnings := map[*types.IoBundle][]string{} + keepInHostUsbControllers := usbControllersWithoutPCIReserve(ctx.assignableAdapters.IoBundleList) // Is any member a network port? @@ -3866,9 +3877,42 @@ func updatePortAndPciBackIoBundle(ctx *domainContext, ib *types.IoBundle) (chang // EVE controller doesn't know it list = aa.ExpandControllers(log, list, hyper.PCISameController) for _, ib := range list { - if types.IsPort(ctx.deviceNetworkStatus, ib.Ifname) && ib.Type.IsNet() { + if !origGroup[ib] { + // EVE pulled this member into the group (ExpandControllers) because + // it shares a PCI controller with a member the controller listed. + modelWarnings[ib] = append(modelWarnings[ib], fmt.Sprintf( + "adapter %s (logicallabel %s, ifname %q, PCI %s) was added to "+ + "assignment group %q because it shares a PCI controller with a "+ + "group member, though the controller's model did not list it there", + ib.Phylabel, ib.Logicallabel, ib.Ifname, ib.PciLong, reqGroup)) + } + switch { + case ib.Type.IsNet() && + types.IsPort(ctx.deviceNetworkStatus, ib.Ifname, ib.PciLong): + // Match by PCI as well as ifname: recognize an in-use port even + // when the kernel name differs from the model (ethN vs enpNsN), so + // it is not wrongly reserved to pciback. isPort = true keepInHost = true + if ib.PciLong != "" && + !types.IsPort(ctx.deviceNetworkStatus, ib.Ifname, "") { + // Matched by PCI only — model ifname doesn't match the kernel. + modelWarnings[ib] = append(modelWarnings[ib], fmt.Sprintf( + "adapter %s (logicallabel %s, model ifname %q, PCI %s) does not "+ + "match the kernel-assigned interface name; matched to the "+ + "in-use network port by PCI address and kept in the host", + ib.Phylabel, ib.Logicallabel, ib.Ifname, ib.PciLong)) + } + case ib.PciLong != "" && !ib.Type.IsNet() && + types.IsPort(ctx.deviceNetworkStatus, "", ib.PciLong): + // Model types this device as non-network, but its PCI backs a live + // network port; keep it in the host rather than reserve to pciback. + keepInHost = true + modelWarnings[ib] = append(modelWarnings[ib], fmt.Sprintf( + "adapter %s (logicallabel %s, ifname %q, PCI %s) is modeled as "+ + "non-network type %d but that PCI address is in use as a network "+ + "port; kept in the host instead of assigning it to pciback", + ib.Phylabel, ib.Logicallabel, ib.Ifname, ib.PciLong, ib.Type)) } if ib.Type == types.IoNetWLAN || ib.Type == types.IoNetWWAN { // Do not put unused wireless devices (unassigned and not associated with any network) into pciback, @@ -3916,11 +3960,17 @@ func updatePortAndPciBackIoBundle(ctx *domainContext, ib *types.IoBundle) (chang } changed, err := updatePortAndPciBackIoMember(ctx, ib, isPort, keepInHost) anyChanged = anyChanged || changed + // Reconcile only this loop's own error sources; leave others + // (collision, assignment-group, missing-device, rename) to their owners. + var pcibackErr []string if err != nil { - ib.Error.Append(err) + pcibackErr = []string{err.Error()} log.Error(err) - } else { - ib.Error.Clear() + } + ib.Error.SetSourceErrors(types.ErrIoBundlePcibackOp{}, false, false, pcibackErr) + ib.Error.SetSourceErrors(types.ErrIoBundleModelInconsistency{}, true, false, modelWarnings[ib]) + for _, w := range modelWarnings[ib] { + log.Warn(w) } } return anyChanged diff --git a/pkg/pillar/cmd/domainmgr/domainmgr_test.go b/pkg/pillar/cmd/domainmgr/domainmgr_test.go index 5dd103a2223..9ecdb9fe619 100644 --- a/pkg/pillar/cmd/domainmgr/domainmgr_test.go +++ b/pkg/pillar/cmd/domainmgr/domainmgr_test.go @@ -652,3 +652,52 @@ func TestConfigEnableUsbUpdatePortAndPciBackIoBundle(t *testing.T) { } } } + +// TestMistypedNetworkPortKeptInHost covers a device model that assigns a device +// a non-network type (here a GPU) even though its PCI address is in fact backing +// a network port. The device must be kept in the host rather than reserved to +// pciback, which would unbind the live port. +func TestMistypedNetworkPortKeptInHost(t *testing.T) { + const pciLong = "0000:06:00.0" + assignableAdapters := types.AssignableAdapters{ + IoBundleList: []types.IoBundle{ + { + Phylabel: "mislabeled", + Logicallabel: "mislabeled", + Type: types.IoHDMI, + AssignmentGroup: "1", + PciLong: pciLong, + }, + }, + } + ctx := &domainContext{ + assignableAdapters: &assignableAdapters, + deviceNetworkStatus: types.DeviceNetworkStatus{ + Ports: []types.NetworkPortStatus{ + {IfName: "eth0", PciLong: pciLong}, + }, + }, + } + ib := &types.IoBundle{AssignmentGroup: "1"} + + updatePortAndPciBackIoBundle(ctx, ib) + + for _, b := range ctx.assignableAdapters.IoBundleList { + if b.Phylabel != "mislabeled" { + continue + } + if !b.KeepInHost { + t.Fatalf("IoBundle %+v should be kept in host: its PCI address is in "+ + "use as a network port", b) + } + // The model/hardware inconsistency must be reported to the controller as + // an advisory warning (not a hard error). + if b.Error.Empty() || !b.Error.IsOnlyWarnings() { + t.Fatalf("IoBundle %+v should carry a warning about the type mismatch, got %q", + b, b.Error.String()) + } + if !strings.Contains(b.Error.String(), pciLong) { + t.Fatalf("warning should identify the PCI address %s, got %q", pciLong, b.Error.String()) + } + } +} diff --git a/pkg/pillar/dpcmanager/dns.go b/pkg/pillar/dpcmanager/dns.go index 7b5038edb52..811ef88c8fb 100644 --- a/pkg/pillar/dpcmanager/dns.go +++ b/pkg/pillar/dpcmanager/dns.go @@ -51,6 +51,12 @@ func (m *DpcManager) updateDNS() { m.deviceNetStatus.Ports[ix].IfName = port.IfName m.deviceNetStatus.Ports[ix].Phylabel = port.Phylabel m.deviceNetStatus.Ports[ix].Logicallabel = port.Logicallabel + // Record the PCI address backing this port so consumers (e.g. domainmgr + // deciding whether to reserve a device to pciback) can match a port to + // a physical device by stable PCI identity rather than by the + // kernel-assigned interface name. Seed from the config; the live value + // resolved from the interface below takes precedence when available. + m.deviceNetStatus.Ports[ix].PciLong = port.PCIAddr m.deviceNetStatus.Ports[ix].SharedLabels = port.SharedLabels m.deviceNetStatus.Ports[ix].Alias = port.Alias m.deviceNetStatus.Ports[ix].IsMgmt = port.IsMgmt @@ -90,6 +96,11 @@ func (m *DpcManager) updateDNS() { wwanNetStatus := m.wwanStatus.GetNetworkStatus(port.Logicallabel) if wwanNetStatus != nil { m.deviceNetStatus.Ports[ix].WirelessStatus.Cellular = *wwanNetStatus + // A modem's PCI can't be derived from its interface; use the + // address the wwan microservice resolved. + if wwanNetStatus.PhysAddrs.PCI != "" { + m.deviceNetStatus.Ports[ix].PciLong = wwanNetStatus.PhysAddrs.PCI + } } } // Do not try to get state data for interface which is in PCIback. @@ -121,6 +132,14 @@ func (m *DpcManager) updateDNS() { } continue } + // Prefer the PCI resolved from the live interface: it reflects the + // device actually backing the port, regardless of the model's ifname. + // Cellular is excluded (see above; IfNameToPci can't resolve a modem). + if port.WirelessCfg.WType != types.WirelessTypeCellular { + if pciLong, _, err := types.IfNameToPciAndUsbAddr(m.Log, port.IfName); err == nil { + m.deviceNetStatus.Ports[ix].PciLong = pciLong + } + } ifAttrs, err := m.NetworkMonitor.GetInterfaceAttrs(ifindex) if err != nil { m.Log.Warnf( diff --git a/pkg/pillar/dpcmanager/dpcmanager_test.go b/pkg/pillar/dpcmanager/dpcmanager_test.go index 52fa1cf0c05..f3a7bfc7f64 100644 --- a/pkg/pillar/dpcmanager/dpcmanager_test.go +++ b/pkg/pillar/dpcmanager/dpcmanager_test.go @@ -964,6 +964,83 @@ func makeAA(intfs selectedIntfs) types.AssignableAdapters { return aa } +// TestPortPciLongInDNS verifies that a port's PCI address is recorded in the +// device network status. The interface name is chosen so it cannot resolve to +// a real PCI device on the test host, which exercises the fallback to the PCI +// address carried in the port config. This PCI identity is what lets domainmgr +// recognize a live port even when its kernel-assigned name differs from the +// model name, instead of reserving the device to pciback. +func TestPortPciLongInDNS(test *testing.T) { + t := initTest(test) + + const ifname = "nictest0" + const pciAddr = "0000:06:00.0" + + mockIf := netmonitor.MockInterface{ + Attrs: netmonitor.IfAttrs{ + IfIndex: 42, + IfName: ifname, + IfType: "device", + WithBroadcast: true, + AdminUp: true, + LowerUp: true, + }, + IPAddrs: []*net.IPNet{ipAddress("192.168.77.5/24")}, + HwAddr: macAddress("02:00:00:00:00:77"), + } + networkMonitor.AddOrUpdateInterface(mockIf) + + dpcManager.UpdateGCP(globalConfig()) + + aa := types.AssignableAdapters{ + Initialized: true, + IoBundleList: []types.IoBundle{ + { + Type: types.IoNetEth, + Phylabel: ifname, + Logicallabel: "mock-nic", + Usage: evecommon.PhyIoMemberUsage_PhyIoUsageMgmtAndApps, + Ifname: ifname, + PciLong: pciAddr, + MacAddr: mockIf.HwAddr.String(), + IsPort: true, + }, + }, + } + dpc := types.DevicePortConfig{ + Version: types.DPCIsMgmt, + Key: "zedagent", + TimePriority: time.Now(), + Ports: []types.NetworkPortConfig{ + { + IfName: ifname, + Phylabel: ifname, + Logicallabel: "mock-nic", + PCIAddr: pciAddr, + IsMgmt: true, + IsL3Port: true, + DhcpConfig: types.DhcpConfig{ + Dhcp: types.DhcpTypeClient, + Type: types.NetworkTypeIPv4, + }, + ConfigSource: types.PortConfigSource{ + Origin: types.NetworkConfigOriginController, + }, + }, + }, + } + dpcManager.UpdateAA(aa) + dpcManager.AddDPC(dpc) + + t.Eventually(func() string { + dns := getDNS() + if len(dns.Ports) == 0 { + return "" + } + return dns.Ports[0].PciLong + }).Should(Equal(pciAddr)) +} + func TestSingleDPC(test *testing.T) { t := initTest(test) t.Expect(dpcManager.GetDNS().DPCKey).To(BeEmpty()) diff --git a/pkg/pillar/types/assignableadapters.go b/pkg/pillar/types/assignableadapters.go index 46acdba8452..f1959cfc905 100644 --- a/pkg/pillar/types/assignableadapters.go +++ b/pkg/pillar/types/assignableadapters.go @@ -213,12 +213,6 @@ func (iobe *IOBundleError) RemoveByType(e error) { iobe.removeByType(e) } -// Clear clears all errors -func (iobe *IOBundleError) Clear() { - iobe.Errors = make([]ioBundleErrorBase, 0) - iobe.TimeOfError = time.Time{} -} - // AggregatedIoBundleError is the combined error state of an assignment group, // ready for reporting in a single ZioBundle. type AggregatedIoBundleError struct { diff --git a/pkg/pillar/types/assignableadapters_test.go b/pkg/pillar/types/assignableadapters_test.go index eeaed602f83..bb7cbbe681c 100644 --- a/pkg/pillar/types/assignableadapters_test.go +++ b/pkg/pillar/types/assignableadapters_test.go @@ -762,8 +762,9 @@ func TestClearingUSBCollision(t *testing.T) { } } + // Break the collision; CheckBadUSBBundles must clear the stale collision + // error on its own (reconcile), without a manual clear. aa.IoBundleList[0].UsbAddr = "1:2" - aa.IoBundleList[0].Error.Clear() aa.CheckBadUSBBundles() for _, ioBundle := range aa.IoBundleList { diff --git a/pkg/pillar/types/dns.go b/pkg/pillar/types/dns.go index b1c91dd4cca..8fe330926be 100644 --- a/pkg/pillar/types/dns.go +++ b/pkg/pillar/types/dns.go @@ -33,6 +33,12 @@ type NetworkPortStatus struct { IfName string Phylabel string // Physical name set by controller/model Logicallabel string + // PciLong is the long-form PCI address (e.g. "0000:06:00.0") of the + // physical device backing this port, when it has one. It gives the port a + // stable identity independent of the kernel-assigned interface name, which + // can differ from the model (e.g. ethN vs enpNsN) or change across driver + // re-binds. Empty for ports not backed by a PCI device (e.g. USB NICs). + PciLong string // Unlike the logicallabel, which is defined in the device model and unique // for each port, these user-configurable "shared" labels are potentially // assigned to multiple ports so that they can be used all together with @@ -675,13 +681,22 @@ func getLocalAddrListImpl(dns DeviceNetworkStatus, return addrs, nil } -// Check if an interface name is a port owned by nim -func IsPort(dns DeviceNetworkStatus, ifname string) bool { +// IsPort reports whether the named interface, or the physical device at the +// given PCI address, is currently used as a device port owned by nim. Matching +// on pciLong (when non-empty) in addition to the interface name makes the check +// robust to the kernel-assigned name differing from the model (e.g. ethN vs +// enpNsN) or changing across driver re-binds. Either argument may be empty and +// an empty argument never matches, so passing an empty ifname with a non-empty +// pciLong matches purely on the PCI address (useful for a device whose modeled +// type is not network but which is in fact backing a network port). +func IsPort(dns DeviceNetworkStatus, ifname, pciLong string) bool { for _, us := range dns.Ports { - if us.IfName != ifname { - continue + if ifname != "" && us.IfName == ifname { + return true + } + if pciLong != "" && us.PciLong == pciLong { + return true } - return true } return false } diff --git a/pkg/pillar/types/dns_test.go b/pkg/pillar/types/dns_test.go index be48b5fbc8d..8d93705d593 100644 --- a/pkg/pillar/types/dns_test.go +++ b/pkg/pillar/types/dns_test.go @@ -222,14 +222,49 @@ func TestDeviceNetworkStatusLookupPortByLogicallabel(t *testing.T) { func TestIsPort(t *testing.T) { dns := DeviceNetworkStatus{ Ports: []NetworkPortStatus{ - {IfName: "eth0"}, - {IfName: "eth1"}, - }, + // The physical NIC the model labels "enp6s0" but which the kernel + // named "eth0" (predictable naming is off); its PCI address is + // recorded so it can be matched independent of the name. + {IfName: "eth0", PciLong: "0000:06:00.0"}, + // A port with no PCI address recorded (e.g. a USB NIC), used to + // check that an empty pciLong query does not match it. + {IfName: "wlan0"}, + // A port whose interface name is not yet known, used to check that + // an empty ifname query does not spuriously match it. + {IfName: "", PciLong: ""}, + }, + } + + tests := []struct { + name string + ifname string + pciLong string + want bool + }{ + {"interface name match", "eth0", "", true}, + {"no match", "eth2", "", false}, + // The regression this guards: the model calls the port "enp6s0" but + // the live port is named "eth0"; matching on the PCI address still + // recognizes it as a port so it is not reserved to pciback. + {"pci match despite ifname mismatch", "enp6s0", "0000:06:00.0", true}, + {"ifname match wins even with unknown pci", "eth0", "0000:99:00.0", true}, + {"neither ifname nor pci match", "enp7s0", "0000:07:00.0", false}, + // An empty pciLong must never match a port that also has no PCI + // recorded, otherwise every unknown device would look like a port. + {"empty pci does not match empty-pci port", "eth9", "", false}, + // A non-network device (empty/irrelevant ifname) still recognized when + // its PCI address is in use as a port. + {"empty ifname matches by pci only", "", "0000:06:00.0", true}, + // An empty ifname must not match the port with an unknown interface + // name, nor must empty+empty match anything. + {"empty ifname does not match empty-ifname port", "", "0000:aa:00.0", false}, + {"empty ifname and empty pci never match", "", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsPort(dns, tt.ifname, tt.pciLong)) + }) } - - assert.True(t, IsPort(dns, "eth0")) - assert.True(t, IsPort(dns, "eth1")) - assert.False(t, IsPort(dns, "eth2")) } // IsMgmtPort diff --git a/pkg/pillar/types/ifnametopci.go b/pkg/pillar/types/ifnametopci.go index aa82d781923..c8c32948d7b 100644 --- a/pkg/pillar/types/ifnametopci.go +++ b/pkg/pillar/types/ifnametopci.go @@ -7,7 +7,6 @@ package types import ( - "errors" "fmt" "os" "path" @@ -21,8 +20,11 @@ import ( "github.com/vishvananda/netlink" ) -const basePath = "/sys/class/net" -const pciPath = "/sys/bus/pci/devices" +// basePath and pciPath are the sysfs roots for interface and PCI lookups. +// They are variables (not constants) so tests can redirect them at a +// fabricated sysfs tree; production never changes them. +var basePath = "/sys/class/net" +var pciPath = "/sys/bus/pci/devices" // ExtractUSBBusnumPort extracts busnum and port number out of a sysfs device path func ExtractUSBBusnumPort(path string) (uint16, string, error) { @@ -45,8 +47,11 @@ func ExtractUSBBusnumPort(path string) (uint16, string, error) { return busnum, port, nil } -// Returns the long PCI IDs and the USB address (if available) -func ifNameToPciAndUsbAddr(log *base.LogObject, ifName string) (string, string, error) { +// IfNameToPciAndUsbAddr returns the long-form PCI address and USB address (if +// any) of the device backing ifName. An L3 port bridged by nim (renamed to +// "k"+ifName) is resolved transparently. Not reliable for cellular modems +// (returns the PCIe bridge, not the modem); use WwanNetworkStatus.PhysAddrs.PCI. +func IfNameToPciAndUsbAddr(log *base.LogObject, ifName string) (string, string, error) { // Match for PCI IDs re := regexp.MustCompile("([0-9a-f]){4}:([0-9a-f]){2}:([0-9a-f]){2}.[ls0-9a-f]") var usbAddr string @@ -251,7 +256,7 @@ func IoBundleToPci(log *base.LogObject, ib *IoBundle) (string, error) { //nolint if ib.Type == IoNetEthVF { l, err = vfIfNameToPci(ib.Ifname) } else { - l, ib.UsbAddr, err = ifNameToPciAndUsbAddr(log, ib.Ifname) + l, ib.UsbAddr, err = IfNameToPciAndUsbAddr(log, ib.Ifname) } rename := false if err == nil { @@ -263,35 +268,44 @@ func IoBundleToPci(log *base.LogObject, ib *IoBundle) (string, error) { //nolint } else { rename = true } + var renameWarn []string if rename { found, ifname := PciLongToIfname(log, long) if found && ib.Ifname != ifname { log.Warnf("%s/%s moved to %s", ib.Ifname, long, ifname) + // Renaming to match the model is an auto-adjustment; warn so + // the controller sees it. + renameWarn = []string{fmt.Sprintf( + "adapter %s (logicallabel %s): model interface name %q does "+ + "not match kernel name %q for PCI %s; renamed to match the model", + ib.Phylabel, ib.Logicallabel, ib.Ifname, ifname, long)} IfRename(log, ifname, ib.Ifname) } } + // Reconcile the rename warning (self-clears when no longer renaming). + ib.Error.SetSourceErrors(ErrIoBundleRename{}, true, false, renameWarn) } } else if ib.Ifname != "" { var err error if ib.Type == IoNetEthVF { long, err = vfIfNameToPci(ib.Ifname) if err != nil { - return long, err + return long, ErrIoBundleMissingDevice{msg: err.Error()} } } else { - long, ib.UsbAddr, err = ifNameToPciAndUsbAddr(log, ib.Ifname) + long, ib.UsbAddr, err = IfNameToPciAndUsbAddr(log, ib.Ifname) if err != nil { - return long, err + return long, ErrIoBundleMissingDevice{msg: err.Error()} } } } else { return "", nil } if !pciLongExists(long) { - errStr := fmt.Sprintf("PCI device %s/%s long %s does not exist", - ib.Phylabel, ib.Logicallabel, long) - return long, errors.New(errStr) + return long, ErrIoBundleMissingDevice{msg: fmt.Sprintf( + "PCI device %s/%s long %s does not exist", + ib.Phylabel, ib.Logicallabel, long)} } return long, nil } diff --git a/pkg/pillar/types/ifnametopci_test.go b/pkg/pillar/types/ifnametopci_test.go index f704ba6709e..833ce06ae2f 100644 --- a/pkg/pillar/types/ifnametopci_test.go +++ b/pkg/pillar/types/ifnametopci_test.go @@ -1,9 +1,14 @@ package types import ( + "os" + "path/filepath" "testing" + "github.com/lf-edge/eve/pkg/pillar/base" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPciLongExists(t *testing.T) { @@ -27,3 +32,50 @@ func TestPciLongExists(t *testing.T) { assert.Equal(t, test.val, output) } } + +// TestIoBundleToPciRenamesShiftedIfname covers the app-direct release path: an +// assignable Ethernet port (model ifname "eth97", stable PciLong) returns from +// PCI passthrough under a kernel-shifted name ("eth98") — deleting an app frees +// a lower ethN index, so the kernel can give the returning NIC a different name +// than the model expects. IoBundleToPci must recognize this by the stable PCI +// address, rename the kernel interface back to the model name, and record the +// adjustment as an advisory ErrIoBundleRename warning for the controller. This +// path is not reachable end-to-end under QEMU (the reverse PCI->ifname sysfs +// lookup does not resolve there), so it is verified against a fabricated sysfs +// tree by redirecting basePath/pciPath. +func TestIoBundleToPciRenamesShiftedIfname(t *testing.T) { + tmp := t.TempDir() + origBase, origPci := basePath, pciPath + basePath = filepath.Join(tmp, "net") // empty: the model ifname does not resolve + pciPath = filepath.Join(tmp, "pci") + defer func() { basePath, pciPath = origBase, origPci }() + + const ( + pci = "0000:00:09.0" + modelName = "eth97" // name in the controller's model + kernelName = "eth98" // shifted name the kernel now gives the returned NIC + ) + // The stable PCI address currently backs the kernel-shifted interface name. + require.NoError(t, os.MkdirAll(filepath.Join(pciPath, pci, "net", kernelName), 0o755)) + require.NoError(t, os.MkdirAll(basePath, 0o755)) + + log := base.NewSourceLogObject(logrus.StandardLogger(), t.Name(), 0) + ib := &IoBundle{ + Type: IoNetEth, + Phylabel: modelName, + Logicallabel: modelName, + Ifname: modelName, + PciLong: pci, + } + + long, err := IoBundleToPci(log, ib) + require.NoError(t, err) + assert.Equal(t, pci, long) + + // The rename is reported as an advisory warning (not a hard error) that names + // the shifted kernel interface EVE renamed back to the model. + assert.True(t, ib.Error.IsOnlyWarnings(), + "rename should be an advisory warning, got %q", ib.Error.String()) + assert.Contains(t, ib.Error.String(), "renamed to match the model") + assert.Contains(t, ib.Error.String(), kernelName) +}