Add support for network interface alternative names #967
Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 19855289298Details
💛 - Coveralls |
cgoncalves
force-pushed
the
altnames
branch
2 times, most recently
from
817c0a8 to
75fb312
Compare
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
LGTM
just a small comment :)
This commit adds support for network interface alternative names (altnames) throughout the SR-IOV Network Operator. Changes: - Add AltNames field to InterfaceExt type in the CRD - Fix netlink library to use correct AltNames field (was AlternativeNames) - Enhance webhook validation to match pfNames against both primary interface names and alternative names - Add GetAlternativeNames method to netlink library interface - Add comprehensive unit tests covering both good and bad paths: * Matching by primary name * Matching by alternative name * Multiple alternative names * Empty/nil alternative names * VF range syntax with alternative names * Non-matching names This allows SR-IOV policies to reference interfaces by their alternative names, providing more flexibility in interface selection. Signed-off-by: Carlos Goncalves <[email protected]>
Document how to discover interface alternative names in node state and use them in SR-IOV network policies with examples. Signed-off-by: Carlos Goncalves <[email protected]>
WalkthroughIntroduces support for alternative interface names (altNames) in SR-IOV networking. Changes include discovering altNames via netlink during SR-IOV discovery, storing them in node state, adding helper functions for name resolution and matching, updating webhook validation to resolve interface names using both primary and alternative names, and updating device plugin resource creation with resolved names. Changes
Sequence DiagramsequenceDiagram
participant Netlink
participant Discovery as SR-IOV Discovery
participant NodeState as Node State
participant Webhook as Policy Validation
participant Controller as Device Plugin Controller
rect rgb(200, 220, 255)
Note over Discovery,NodeState: Discovery & Storage Phase
Discovery->>Netlink: GetAltNames(pf_name)
Netlink-->>Discovery: altNames []
Discovery->>NodeState: Store Interface + AltNames
end
rect rgb(220, 240, 220)
Note over Webhook: Validation Phase
Webhook->>NodeState: Get current node state
Webhook->>Webhook: ResolveInterfaceName(name, nodeState)
Webhook->>Webhook: NameOrAltNameMatchesPfNames(name, altNames, pfNames)
end
rect rgb(255, 240, 200)
Note over Controller: Device Plugin Creation Phase
Controller->>NodeState: Get interface names
Controller->>Controller: resolvePfNames(pfNames, nodeState)
Controller->>Controller: Preserve VF range suffixes
Controller->>Controller: Create DevicePluginResource with resolved names
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (1)
291-313: Shell snippet trips markdownlint MD014 (prompt char).The
bashexample includes a leading$prompt without showing output, which violates MD014. Consider dropping the$so the block contains just the command:kubectl get sriovnetworknodestate -n sriov-network-operator <node-name> -o yamlpkg/host/internal/lib/netlink/netlink.go (1)
73-75: GetAltNames implementation is fine; consider adding targeted tests.The new
GetAltNameswrapper correctly handles errors, nil attrs, and empty AltNames. To avoid regressions and improve coverage, it would be good to add a small unit test that exercises:
- link not found (propagated error),
- link with nil/empty
Attrs,- link with non-empty
AltNames.You can mock via the generated NetlinkLib mock or by testing a caller that uses this method.
Also applies to: 198-210
api/v1/helper.go (1)
324-336: Alt-name–aware matching helpers are correct and integrate cleanly.
NameOrAltNameMatchesPfNamesand its use inSriovNetworkNicSelector.Selectedcorrectly extend matching toAltNameswhile preserving existing behavior for primary names and VF ranges.ResolveInterfaceNameprovides the expected “best-effort” mapping from an altName to the canonical interface name and safely falls back whennodeStateor matches are absent.If you find more call sites that need this pattern (e.g., additional validations), consider centralizing the PF-name–without-range extraction into a small helper to avoid repeating the
strings.Contains(p, "#")logic, but it’s not urgent.Also applies to: 617-630, 1028-1049
pkg/webhook/validate_test.go (1)
1330-1580: AltName-focused tests give good coverage; minor redundancy only.The new tests around alt-name handling (
TestValidatePolicyForNodeStateWithPfNameMatchingAlternativeName, VF-range + altName, andTestValidatePolicyForNodePolicyWithAltNameConflict) thoroughly cover:
- Selection via primary vs alternative names.
- Empty vs nil vs populated
AltNames.- VF-range syntax combined with altNames.
- Cross-policy conflicts when one policy uses the primary name and another uses an altName, with and without nodeState.
This is very helpful for guarding the new behavior. The only nit is that
TestValidatePolicyForNodeStateWithPfNameMatchingInterfaceNamelargely duplicates the earlier “valid PfName” test; you could merge them in the future to reduce duplication, but it’s not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(2 hunks)api/v1/helper.go(3 hunks)api/v1/helper_test.go(1 hunks)api/v1/sriovnetworknodestate_types.go(1 hunks)api/v1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/sriovnetwork.openshift.io_sriovnetworknodestates.yaml(1 hunks)controllers/sriovnetworknodepolicy_controller.go(3 hunks)controllers/sriovnetworknodepolicy_controller_test.go(1 hunks)deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovnetworknodestates.yaml(1 hunks)pkg/host/internal/lib/netlink/mock/mock_netlink.go(1 hunks)pkg/host/internal/lib/netlink/netlink.go(2 hunks)pkg/host/internal/sriov/sriov.go(2 hunks)pkg/host/internal/sriov/sriov_test.go(2 hunks)pkg/webhook/validate.go(5 hunks)pkg/webhook/validate_test.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
api/v1/helper.go (1)
api/v1/sriovnetworknodestate_types.go (2)
SriovNetworkNodeState(142-148)Interfaces(33-33)
controllers/sriovnetworknodepolicy_controller_test.go (1)
api/v1/sriovnetworknodestate_types.go (4)
SriovNetworkNodeState(142-148)SriovNetworkNodeStateStatus(126-132)Interfaces(33-33)InterfaceExt(56-74)
pkg/webhook/validate.go (3)
api/v1/sriovnetworknodestate_types.go (1)
SriovNetworkNodeState(142-148)api/v1/sriovnetworknodepolicy_types.go (1)
SriovNetworkNodePolicy(145-151)api/v1/helper.go (2)
ResolveInterfaceName(1031-1050)NameOrAltNameMatchesPfNames(326-336)
pkg/webhook/validate_test.go (2)
api/v1/sriovnetworknodepolicy_types.go (3)
SriovNetworkNodePolicy(145-151)SriovNetworkNodePolicySpec(27-68)SriovNetworkNicSelector(70-81)api/v1/sriovnetworknodestate_types.go (4)
Interfaces(33-33)SriovNetworkNodeState(142-148)SriovNetworkNodeStateStatus(126-132)InterfaceExt(56-74)
controllers/sriovnetworknodepolicy_controller.go (2)
api/v1/sriovnetworknodestate_types.go (1)
SriovNetworkNodeState(142-148)api/v1/helper.go (2)
ResolveInterfaceName(1031-1050)UniqueAppend(351-358)
🪛 markdownlint-cli2 (0.18.1)
README.md
296-296: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: test-coverage
- GitHub Check: Golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
api/v1/zz_generated.deepcopy.go (1)
120-124: LGTM! DeepCopy implementation follows the correct pattern.The AltNames deep copy logic correctly mirrors the existing VFs handling, with proper nil checks and slice allocation.
deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovnetworknodestates.yaml (1)
331-334: LGTM! CRD schema correctly defines altNames.The altNames field is properly defined as an array of strings under the interface properties.
config/crd/bases/sriovnetwork.openshift.io_sriovnetworknodestates.yaml (1)
331-334: LGTM! CRD schema matches deployment chart.The altNames definition is consistent across both CRD locations.
pkg/host/internal/sriov/sriov.go (2)
286-286: AltNames correctly included in InterfaceExt.The AltNames field is properly populated in the InterfaceExt structure, assuming the discovery succeeds.
263-267: Verify error handling strategy for GetAltNames.Skipping the entire device when GetAltNames fails seems overly strict for a new, potentially optional feature. If the system doesn't support alternative names or encounters a transient error, valid SR-IOV devices would be excluded from discovery.
Consider one of these approaches:
- Log the error but continue with empty AltNames:
altNames = []string{}- Only skip if the error indicates a critical issue (not just "feature not supported")
This ensures backward compatibility and graceful degradation on systems without alternative name support.
api/v1/sriovnetworknodestate_types.go (1)
73-73: LGTM! AltNames field properly defined.The field is correctly added with appropriate JSON tags for backward compatibility.
pkg/host/internal/sriov/sriov_test.go (1)
78-78: LGTM! Test correctly validates AltNames discovery.The mock setup and assertion properly verify that AltNames are retrieved and stored in the InterfaceExt structure.
Also applies to: 135-135
api/v1/helper_test.go (1)
1536-1647: LGTM! Comprehensive test coverage for ResolveInterfaceName.The test suite thoroughly covers all scenarios including:
- Nil and empty states
- Primary and alternative name matching
- Multiple interfaces with different AltNames
- Edge cases with empty AltNames
pkg/host/internal/lib/netlink/mock/mock_netlink.go (1)
155-168: LGTM! Mock implementation follows standard gomock pattern.The auto-generated mock correctly implements the GetAltNames method with proper controller integration.
controllers/sriovnetworknodepolicy_controller_test.go (1)
50-148: resolvePfNames test matrix looks solid.The table-driven
TestResolvePfNamescovers empty input, nil nodeState, direct name hits, alt-name resolution, mixed names, “not found” passthrough, and VF-range preservation. This gives good confidence in the helper’s behavior.controllers/sriovnetworknodepolicy_controller.go (1)
543-567: PF-name resolution logic is correct and VF-range–safe.
resolvePfNames+ the changes increateDevicePluginResource/updateDevicePluginResourcedo the right thing:
- Strip any
#range, resolve viaResolveInterfaceName, then re-attach the original suffix.- Handle
nilor interface-missing nodeState by returning the original names.- Feed resolved names through
UniqueAppend, avoiding duplicate entries in the device-plugin selectors.This should let users write policies using altnames while still emitting canonical PF names to the device plugin.
Also applies to: 596-599, 665-668
pkg/webhook/validate_test.go (1)
441-443: validatePolicyForNodePolicy signature updates are consistent.All existing tests now pass
nilas the newnodeStateparameter, preserving the previous behavior for those scenarios while allowing newer tests to opt into state-aware validation. This keeps backward-compat coverage intact.Also applies to: 467-469, 614-616, 640-642, 661-665, 688-692, 736-740
pkg/webhook/validate.go (2)
285-308: State-aware PF-name conflict detection is correctly wired.
validatePolicyForNodeStateAndPolicynow passes the node’sSriovNetworkNodeStateintovalidatePolicyForNodePolicy, and that flows intovalidatePfNames.validatePfNamesresolves both current and previous PF names throughResolveInterfaceNamewhennodeStateis non-nil, so policies using different names (primary vs altName) for the same PF are treated as conflicting, including VF-range overlaps.- The nil checks around
nodeStatekeep behavior unchanged when node state is missing.This matches the new tests and gives the expected stricter conflict detection when node state is available.
Also applies to: 360-420
456-478: NIC selection now properly considers AltNames.The change in
validateNicModelto useNameOrAltNameMatchesPfNames(iface.Name, iface.AltNames, pfNames)means:
- PF selection works when policies specify either the primary interface name or any of its alternative names.
- The updated error message (“(and alternative names) not found…”) more accurately reflects what’s being checked.
This keeps NIC model validation aligned with the selector logic in
api/v1and the controller.
4 participants
Add support for network interface alternative names
Overview
This PR adds support for network interface alternative names (altnames) throughout the SR-IOV Network Operator. Interface alternative names provide additional flexibility when selecting network interfaces in policies, particularly useful in environments where interfaces may have predictable alternative naming schemes.
This change is backward compatible. Existing policies using primary interface names will continue to work without modification.
How to Use
Alternative names are automatically discovered and stored in the
SriovNetworkNodeStateCR. When defining aSriovNetworkNodePolicy, you can now reference interfaces using either their primary name or any of their alternative names in thenicSelector.pfNamesfield.Example:
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.