Extend NetworkInterface status with firmware version and other details#983
Extend NetworkInterface status with firmware version and other details#983afritzler wants to merge 3 commits into
NetworkInterface status with firmware version and other details#983Conversation
github-actions
Bot
added
size/L
api-change
documentation
afritzler
changed the title
NetworkInterface status with firmware vesion and other detailsNetworkInterface status with firmware version and other details
Signed-off-by: Andreas Fritzler <andreas.fritzler@sap.com>
📝 WalkthroughWalkthroughThis PR adds new NIC hardware metadata fields across the NetworkInterface API, generated applyconfiguration and schema code, CRD/docs, registry and probe data, and server controller logic that merges registry-reported NIC and LLDP details into server status. ChangesNIC hardware detail fields
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/v1alpha1/server_types.go (1)
336-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a more structured type for
Speed.
Speedis a free-form string (e.g. registry sends"10000"), which loses unit semantics and can't be validated/sorted/compared like a numeric quantity. Since this is new API surface, using an integer (Mbps) orresource.Quantitywould be more idiomatic and future-proof, though string is workable if the Redfish/ethtool source data itself is inconsistent in units.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/server_types.go` around lines 336 - 338, The Speed field on the server status type is currently a free-form string, which makes it hard to validate and compare consistently. Update the Server type’s Speed field in the server_types definition to use a more structured representation such as an integer in Mbps or resource.Quantity, and adjust any related JSON/schema handling and consumers of Speed so the API surface stays future-proof and semantically clear.internal/controller/server_controller.go (1)
1028-1052: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the MAC-fallback match path with a unit test.
The merge logic here has three distinct branches (name match, MAC-fallback match, no match), but the added integration test in
server_controller_test.goonly exercises the name-match path (NIC name equals interface name). SincebuildNetworkInterfacesis a standalone, easily-testable function, a small table-driven unit test covering the MAC-fallback and no-match cases would guard against regressions in this non-trivial matching logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/server_controller.go` around lines 1028 - 1052, Add a focused table-driven unit test for buildNetworkInterfaces that covers the MAC fallback and no-match branches in the NIC merge logic. Use the existing name-match test as a reference, then create cases where registryNICs matches a NIC by MACAddress only and where no registry NIC matches at all; assert that hardware fields are merged in the fallback case and remain unchanged in the no-match case. This should exercise the nicsByMAC lookup and the loop over nics so the matching behavior is protected from regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/v1alpha1/server_types.go`:
- Around line 336-338: The Speed field on the server status type is currently a
free-form string, which makes it hard to validate and compare consistently.
Update the Server type’s Speed field in the server_types definition to use a
more structured representation such as an integer in Mbps or resource.Quantity,
and adjust any related JSON/schema handling and consumers of Speed so the API
surface stays future-proof and semantically clear.
In `@internal/controller/server_controller.go`:
- Around line 1028-1052: Add a focused table-driven unit test for
buildNetworkInterfaces that covers the MAC fallback and no-match branches in the
NIC merge logic. Use the existing name-match test as a reference, then create
cases where registryNICs matches a NIC by MACAddress only and where no registry
NIC matches at all; assert that hardware fields are merged in the fallback case
and remain unchanged in the no-match case. This should exercise the nicsByMAC
lookup and the loop over nics so the matching behavior is protected from
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2edf0503-5186-4ed4-b0c4-6e130e19bf7a
⛔ Files ignored due to path filters (1)
dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (8)
api/v1alpha1/applyconfiguration/api/v1alpha1/networkinterface.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/server_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/server_controller.gointernal/controller/server_controller_test.go
Signed-off-by: Andreas Fritzler <andreas.fritzler@sap.com>
There was a problem hiding this comment.
works in vm devenv, there just is no channel information in a qemu vm when using the jaypipes/ethtool:
kubectl get servers -o yaml
...
networkInterfaces:
- carrierStatus: up
device: "0x0001"
ips:
- 172.19.100.3
- fe80::5054:ff:fede:5965
macAddress: 52:54:00:de:59:65
name: enp0s2
pciAddress: unknown
speed: "-1"
vendor: "0x1af4"
No |
Signed-off-by: Andreas Fritzler <andreas.fritzler@sap.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/probe/nics_linux.go (1)
64-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent swallow of
GetChannelserrors.Channel-capacity errors are dropped without logging, so real failures (e.g. driver doesn't support ethtool
-g, or a permission issue) are indistinguishable from the field being legitimately absent. Given the PR's intent ("optional sysfs-based details when available"), this is acceptable behavior-wise, but consider a debug-level log for observability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/probe/nics_linux.go` around lines 64 - 69, The GetChannels call in probeNICs silently ignores errors, making real failures indistinguishable from missing data. Update the nics_linux.go logic around ethHandle.GetChannels in the NIC probe path to keep the optional behavior but add a debug-level log when the call fails, including the NIC name and the error. Use the existing probe flow in the NIC collection code so observability improves without changing the fallback behavior.internal/controller/server_controller.go (1)
1052-1052: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnchecked narrowing conversions can wrap hardware counters into corrupted status values.
hw.NUMANode(int) andhw.MaxRx/MaxTx/MaxOther/MaxCombined(uint32) are cast directly toint32with no bounds check. If a device ever reports a queue count abovemath.MaxInt32, the resulting persisted status field silently wraps to a negative number, corrupting the CR status without any error surfaced.🛡️ Proposed fix: clamp before narrowing
+func clampToInt32(v uint32) int32 { + if v > math.MaxInt32 { + return math.MaxInt32 + } + return int32(v) +} + nics[i].PCIAddress = hw.PCIAddress nics[i].Speed = hw.Speed nics[i].LinkModes = hw.LinkModes nics[i].SupportedPorts = hw.SupportedPorts nics[i].FirmwareVersion = hw.FirmwareVersion nics[i].NUMANode = int32(hw.NUMANode) nics[i].Vendor = hw.Vendor nics[i].SubsystemVendor = hw.SubsystemVendor nics[i].Device = hw.Device - nics[i].MaxRx = int32(hw.MaxRx) - nics[i].MaxTx = int32(hw.MaxTx) - nics[i].MaxOther = int32(hw.MaxOther) - nics[i].MaxCombined = int32(hw.MaxCombined) + nics[i].MaxRx = clampToInt32(hw.MaxRx) + nics[i].MaxTx = clampToInt32(hw.MaxTx) + nics[i].MaxOther = clampToInt32(hw.MaxOther) + nics[i].MaxCombined = clampToInt32(hw.MaxCombined)Practically, real-world channel/queue counts stay well below this bound, but the fix is cheap and removes the CWE-190 risk flagged by static analysis.
Also applies to: 1056-1059
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/server_controller.go` at line 1052, The status population in server_controller’s NIC handling is doing unchecked narrowing from hw.NUMANode and hw.MaxRx/MaxTx/MaxOther/MaxCombined into int32 fields, which can silently wrap large hardware values. Update the NIC status assignment logic around the nics slice population to clamp or validate these hardware counters before casting, and only persist bounded values into the CR status. Use the existing NIC-building loop and the NUMANode/MaxRx/MaxTx/MaxOther/MaxCombined assignments as the focal points for the fix.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/server_controller.go`:
- Line 1052: The status population in server_controller’s NIC handling is doing
unchecked narrowing from hw.NUMANode and hw.MaxRx/MaxTx/MaxOther/MaxCombined
into int32 fields, which can silently wrap large hardware values. Update the NIC
status assignment logic around the nics slice population to clamp or validate
these hardware counters before casting, and only persist bounded values into the
CR status. Use the existing NIC-building loop and the
NUMANode/MaxRx/MaxTx/MaxOther/MaxCombined assignments as the focal points for
the fix.
In `@internal/probe/nics_linux.go`:
- Around line 64-69: The GetChannels call in probeNICs silently ignores errors,
making real failures indistinguishable from missing data. Update the
nics_linux.go logic around ethHandle.GetChannels in the NIC probe path to keep
the optional behavior but add a debug-level log when the call fails, including
the NIC name and the error. Use the existing probe flow in the NIC collection
code so observability improves without changing the fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b1513e7-7afd-42e4-b4c0-9d14b54f1238
⛔ Files ignored due to path filters (1)
dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (11)
api/v1alpha1/applyconfiguration/api/v1alpha1/networkinterface.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/server_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/api/registry/nic.gointernal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/probe/nics_darwin.gointernal/probe/nics_linux.go
✅ Files skipped from review due to trivial changes (3)
- docs/api-reference/api.md
- api/v1alpha1/applyconfiguration/api/v1alpha1/networkinterface.go
- api/v1alpha1/applyconfiguration/internal/internal.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/crd/bases/metal.ironcore.dev_servers.yaml
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1alpha1/server_types.go
- internal/controller/server_controller_test.go
2 participants
Proposed Changes
Extend
NetworkInterfacestatus with firmware vesion and other details.Fixes #981
Summary by CodeRabbit