domainmgr: match device ports by PCI, not just interface name - #6169
domainmgr: match device ports by PCI, not just interface name#6169eriknordmark wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6169 +/- ##
==========================================
+ Coverage 22.64% 23.15% +0.51%
==========================================
Files 507 517 +10
Lines 93195 95089 +1894
==========================================
+ Hits 21103 22017 +914
- Misses 70367 71146 +779
- Partials 1725 1926 +201 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // "k"-prefixed device is resolved transparently. Returns a non-nil error if no | ||
| // PCI device backs the interface (e.g. a purely virtual interface). | ||
| func IfNameToPci(log *base.LogObject, ifName string) (string, error) { | ||
| long, _, err := ifNameToPciAndUsbAddr(log, ifName) |
There was a problem hiding this comment.
I'm not sure if this method works correctly for cellular modems. The algorithm to determine PCI address for a modem (that we use today) is a bit different.
There was a problem hiding this comment.
Good catch — confirmed, and fixed in 6ac597f.
ifNameToPciAndUsbAddr walks /sys/class/net/<if>/device and, for a modem, falls through to its regex fallback which returns the first PCI component in the path — i.e. the enclosing PCIe bridge — not the modem's own address. getSysDevAddr (the algorithm you linked) instead walks up until it finds the parent whose subsystem resolves to pci, returning the deepest node on the PCI bus (the modem itself). So the two disagree exactly for the modem case.
The fix stops relying on IfNameToPci for cellular ports: dpcmanager now stamps PciLong from WwanNetworkStatus.PhysAddrs.PCI (resolved by the wwan service via ModemManager) and skips the interface-based override for cellular ports. I also documented the limitation on IfNameToPci so it isn't reused for modems.
|
I think the testing instructions should be more comprehensive.
|
Does the device lose connectivity permanently, or does it fall back to the previous configuration? I'm wondering whether making EVE accept an incorrect model is really the right approach. Wouldn't it be safer for EVE to reject such a configuration and report an error back to the controller instead? |
I think from a user point of view that makes sense. |
@milan-zededa Two different failure behavior for the two cases.
NOTE: This is not an issue with the SystemAdapter and its networking fallback - the issues are with PCI address false matches.
With the current state of the PR it doesn't accept and apply what is wrong, but I'll check if any error is reported (I suspect not) and add that in the appropriate place. Maybe setting a new maintenance mode makes sense to make it very visible that the model is incorrect? |
|
@milan-zededa Turns out the tests couldn't force a failure due to "the model has incorrect PCI addresses for the I/O devices". Apparently the ExpandControllers silently ignores such cases by making the assignment group be larger. That's good, but it still makes sense to have a test for this - and get this reported as a warning. |
359377c to
9e830a1
Compare
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 <erik@zededa.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 <erik@zededa.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9e830a1 to
c9ebaa0
Compare
| } | ||
| continue | ||
| } | ||
| // Prefer the PCI resolved from the live interface: it reflects the |
There was a problem hiding this comment.
I think that this should be higher up, above m.NetworkMonitor.GetInterfaceIndex (line 126)
If the interface name from the model is wrong, m.NetworkMonitor.GetInterfaceIndex will fail, and this code will not be reached.
|
Claude finished @milan-zededa's task in 4m 50s —— View job Code reviewI reviewed the two commits (error-model rework + the pciback fix). This is a solid, well-tested change; below are the findings, all non-blocking. Posted as inline comments:
Findings
Also noted (no action needed): The severity picking ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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 { |
There was a problem hiding this comment.
Building on the relocation point already raised above: moving this block above GetInterfaceIndex (line 126) is necessary but not sufficient. IfNameToPciAndUsbAddr itself resolves via /sys/class/net/<port.IfName>/device, so for the exact case this PR targets — a model IfName that doesn't match the kernel-assigned name — this lookup fails too, and PciLong is left at the config-seeded port.PCIAddr (which in the "model has wrong PCI" scenario is also wrong).
To actually recover the live PCI when the model name is wrong, resolution needs to start from a stable handle (the kernel-observed name / ifindex), not port.IfName. Worth confirming what port.IfName contains here on a device where predictable naming is off — if nim already normalizes it to the kernel name, GetInterfaceIndex won't fail and the relocation is moot; if it carries the raw model name, neither the guard nor this lookup helps.
| for _, s := range desired { | ||
| if !have[s] { | ||
| iobe.Errors = append(iobe.Errors, ioBundleErrorBase{ | ||
| ErrStr: s, TypeStr: typeStr, Warning: warning, GroupScoped: groupScoped, | ||
| }) | ||
| added = true | ||
| } | ||
| } |
There was a problem hiding this comment.
have[s] is never set after an append, so if desired contains the same string twice, it is appended twice — the add loop only dedups against pre-existing entries, not within desired itself. appendEntry dedups fully, but SetSourceErrors does not. No current caller passes duplicate strings, so this is latent, but it's cheap to close:
| for _, s := range desired { | |
| if !have[s] { | |
| iobe.Errors = append(iobe.Errors, ioBundleErrorBase{ | |
| ErrStr: s, TypeStr: typeStr, Warning: warning, GroupScoped: groupScoped, | |
| }) | |
| added = true | |
| } | |
| } | |
| for _, s := range desired { | |
| if !have[s] { | |
| iobe.Errors = append(iobe.Errors, ioBundleErrorBase{ | |
| ErrStr: s, TypeStr: typeStr, Warning: warning, GroupScoped: groupScoped, | |
| }) | |
| have[s] = true | |
| added = true | |
| } | |
| } |
| ib.Error.SetSourceErrors(types.ErrIoBundleMissingDevice{}, false, false, missing) | ||
| // We assume AddOrUpdateIoBundle will preserve any | ||
| // existing IsPort/IsPCIBack/UsedByUUID | ||
| aa.AddOrUpdateIoBundle(log, *ib) |
There was a problem hiding this comment.
Note: AddOrUpdateIoBundle overwrites the stored bundle's Error with this fresh ib.Error (it preserves IsPort/IsPCIBack/PciLong/... but not Error). So on every PhysicalIOAdapterList update, the collision / assignment-group-conflict / model-inconsistency entries previously reconciled onto the stored bundle are dropped, along with their stable TimeOfError — the very timestamp stability SetSourceErrors was added to preserve. It's recovered because the checks re-run afterward, but each such update resets those timestamps. Since PhysicalIOAdapterList republishes only on model change this is low-impact, but worth a comment or preserving Error in AddOrUpdateIoBundle so the intent isn't quietly undone.
3 participants
Description
The purpose of this PR is to make EVE more robust against incorrect models received from the controller in the PhysIo part of the API. We have seen two cases where odd things can happen:
In those cases, once the device has been onboarded and receives the PhysIo from the controller it might loose its connectivity due to the device with the PCI address of the management Ethernet ports being assigned to pciback/vfio-pci, or the set confusion where NIM+kernel and the PhysIo have end up detaching and renaming the Ethernet's ifnames.
Prior to this PR we had:
On a device where the kernel assigns network interface names that differ from
the names in the controller's device model — for example the model labels a
port
enp6s0but the kernel names iteth0because predictable naming is notenabled — domainmgr treated the in-use management NIC as an unused assignable
adapter and reserved it to pciback. Reserving unbinds the host driver; on
release the NIC re-probed under a fresh name, which nim re-adopted and
re-bridged, so the interface count kept growing (
eth0,eth2,eth3, …) andthe device repeatedly lost its connection to the controller.
If the model had some other I/O type (e.g., USB controller) specified as having the PCI address of the eth0 I/O device, then as part of moving that I/O device to pciback/vfio-pci it would move eth0, meaning that the management connectivity would be lost.
The change is two commits:
pillar: report I/O-bundle errors per assignment group(foundation) —reworks how errors on an assignable-adapter (I/O bundle) are modeled and
reported: each entry carries a warning-vs-hard-error classification and a
group-vs-member scope, and every source reconciles only its own entries so a
persistent error keeps a stable timestamp and one source cannot clear
another's. zedagent aggregates a group's members into the single ZioBundle
error slot with the right severity — group-scoped entries (e.g. a collision)
once, member-scoped entries attributed to the member. This also fixes a
pre-existing gap where per-member / non-representative errors were dropped
from the report. No device behavior changes.
domainmgr: keep in-use ports out of pciback(the fix) — an in-usenetwork port is matched to its device by PCI address as well as interface
name, so it is kept in the host even when the kernel-assigned name differs
from the model (ethN vs enpNsN) or the model declares a non-network type for a
PCI address that in fact backs a live port. Each such adjustment, and an
interface rename forced to match the model, is reported as an advisory
warning. The port-vs-pciback loop reconciles only its own errors instead of
clearing the whole bundle each pass, so the collision / assignment-conflict
errors and warnings survive to the report. Cellular ports take their PCI from
the wwan microservice (ModemManager), which resolves a modem's address where
the interface-based lookup returns the enclosing PCIe bridge.
How to test and validate this PR
Automated (unit tests):
go test ./pkg/pillar/types/ -run TestIsPort— interface-name / PCI-addressmatch matrix, including the empty-argument semantics.
go test ./pkg/pillar/dpcmanager/ -run TestPortPciLongInDNS— the port's PCIaddress is surfaced in the device network status.
go test ./pkg/pillar/cmd/domainmgr/ -run TestMistypedNetworkPortKeptInHost—a non-network-typed device whose PCI backs a network port is kept in the host.
go test ./pkg/pillar/types/ -run TestIoBundleToPciRenamesShiftedIfname— anassignable NIC returning from passthrough under a kernel-shifted name (a freed
lower ethN index) is renamed back to the model name via its stable PCI address
and the adjustment is reported as an advisory warning.
Manual, on a device whose model interface names differ from the kernel names
(e.g. model uses
enpNsNbut the kernel names portsethN, predictable namingoff):
domainmgrrepeatedly logsAssigning <port> (<pci>) to pciback,the kernel logs repeated
igb … removed PHC/added PHC on ethN, theinterface list grows (
eth0,eth2, …), and controller connectivity flaps.churn, the interface count is stable, and the controller connection holds.
End-to-end (evetest), automated:
TestPcibackErrorSuite(evetest: add assignable-adapter error reporting tests #6186). Through the controller it pushesdevice models with inconsistent PhysicalIO entries and asserts the reported
ZioBundle errors/warnings and severities: a missing device, a self-parent
assignment group, a multi-node
parentassigngrpcycle, and a USB-addresscollision as errors; an interface-name mismatch matched by PCI as a warning
(the port is kept in the host); the aggregation of a warning and a hard error
as an error; and the warning clearing once the model is corrected.
pubsub: size ReadWithMaxSize buffer to the file, not maxReadSize #6185 and the evetest: add assignable-adapter error reporting tests #6186 tests), all subtests pass. The suite reaches ~76% of
these PRs' added lines on its own and ~94% combined with the unit tests above.
fails:
TestReportMissingDevicetimes out waiting for the ZioBundle errorthat master never reports, confirming the change is load-bearing.
Hardware configurations to validate
The port-vs-model matching runs on every network interface type, so the change
should be validated across the range of supported interfaces. The key check in
each case is the same: a port that is in use as a management port must stay
bound in the host (not reserved to pciback) even when the model's ifname/PCI/type
disagrees with what the kernel reports, and the PCI address recorded in
DeviceNetworkStatus must be the one actually backing the port.
PciLongpath (USB address, no own PCI)PciLongpath (deepest PCI node, not the bridge)The cellular rows specifically confirm the fix for the modem PCI-resolution issue
raised in review: for those ports
PciLongmust come from the wwan microservice,not the interface-based lookup.
Changelog notes
Fixed a bug where a network port could be repeatedly reset — losing the
connection to the controller — on devices whose interface names assigned by the
kernel differ from the names in the device model.
PR Backports
This is a long-standing device-manageability bug (it can leave a device unable
to reach the controller). Candidate for backport to all current LTS branches;
please confirm applicability per branch:
Checklist