Fix OVS bridge grouping to create single bridge with multiple uplinks

[almaslennikov]

When SriovNetworkNodePolicy specifies groupingPolicy: all, the operator should create a single OVS bridge with all matching PFs as uplinks. Previously, it was incorrectly creating separate bridges for each PF.

Changes:

• Update ApplyBridgeConfig to propagate GroupingPolicy to node state
• Add applyGroupedBridgeConfig() to create single bridge configuration with all matching PFs as uplinks when groupingPolicy is "all"
• Modify CreateOVSBridge() to support multiple uplinks per bridge
• Remove ConfigureBridgesGrouping() as grouped bridge creation is now handled entirely by the controller via ApplyBridgeConfig
• Add unit tests for grouped bridge functionality

Fixes issue where applying a policy like:

  bridge:
    ovs:
      bridge:
        groupingPolicy: all

Would create multiple bridges (br-ens2f0np0, br-ens2f1np1, etc.) instead of a single bridge (br-) containing all PFs.

[github-actions]

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

• /test-all: To run all tests for all vendors.
• /test-e2e-all: To run all E2E tests for all vendors.
• /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

• /skip-all: To skip all tests for all vendors.
• /skip-e2e-all: To skip all E2E tests for all vendors.
• /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
  Best regards.

[greptile-apps]

Greptile Summary

This PR fixes OVS bridge grouping when groupingPolicy: all is specified, ensuring a single bridge is created with multiple PFs as uplinks instead of separate bridges per PF.

Key changes:

• Controller-side grouping: Bridge grouping logic moved from daemon (ConfigureBridgesGrouping) to controller (applyGroupedBridgeConfig), which creates a single bridge spec with all uplinks upfront
• Bridge naming: Grouped bridges now use policy name (br-{policyName}) instead of generic suffix ({name}-all)
• Cleanup logic: Automatically removes existing per-PF bridges when applying grouped configuration
• Multi-uplink support: CreateOVSBridge() now validates len(uplinks) >= 1 (was != 1) and iterates to add all uplinks

Testing:

• Comprehensive unit tests cover grouped bridge creation and cleanup scenarios
• Tests verify uplink sorting (by PCI address) for consistent ordering

Architecture improvement:
The refactor is cleaner - the controller generates the complete desired state, and the daemon simply applies it, removing the need for special-case daemon-side grouping logic.

Confidence Score: 4/5

• This PR is safe to merge with minor documentation fix needed
• The implementation is well-architected with good separation of concerns. The controller generates complete desired state, and the daemon applies it cleanly. Comprehensive test coverage validates both creation and cleanup scenarios. One minor comment fix needed for accuracy. The logic properly handles edge cases like sorting uplinks and cleaning up existing bridges.
• api/v1/helper.go needs a minor comment correction (already noted in inline comment)

Important Files Changed

Filename	Overview
api/v1/helper.go	Adds applyGroupedBridgeConfig() to handle single bridge creation with multiple uplinks when groupingPolicy: all. Sets GroupingPolicy in state and cleans up existing per-PF bridges. Logic appears sound.
api/v1/helper_test.go	Comprehensive test coverage for grouped bridge functionality, including creation and cleanup of existing per-PF bridges. Tests validate expected behavior.
pkg/host/internal/bridge/ovs/ovs.go	Modified CreateOVSBridge() to support multiple uplinks per bridge (changed validation from != 1 to < 1). Iterates through all uplinks to add them to the bridge.
pkg/plugins/generic/generic_plugin.go	Removed call to ConfigureBridgesGrouping(). Added comment explaining grouping is now handled in controller.

Sequence Diagram

sequenceDiagram
    participant Controller
    participant Policy as SriovNetworkNodePolicy
    participant State as SriovNetworkNodeState
    participant Daemon as GenericPlugin
    participant Bridge as bridge.ConfigureBridges
    participant OVS as ovs.CreateOVSBridge
    
    Note over Controller,Policy: Controller applies policy to node state
    Controller->>Policy: ApplyBridgeConfig(state)
    
    alt groupingPolicy == "all"
        Policy->>State: Set GroupingPolicy in state.Spec.Bridges
        Policy->>Policy: applyGroupedBridgeConfig(state)
        Policy->>State: Collect all matching interfaces as uplinks
        Policy->>State: Sort uplinks by PCI address
        Policy->>State: Delete existing per-PF bridges for these uplinks
        Policy->>State: Create single bridge entry (br-{policyName}) with all uplinks
    else no grouping policy
        loop For each matching interface
            Policy->>State: Create individual bridge (br-{pciAddr}) with single uplink
        end
    end
    
    Note over Daemon,OVS: Daemon applies state to host
    Daemon->>Bridge: ConfigureBridges(state.Spec.Bridges, state.Status.Bridges)
    loop For each bridge in spec
        Bridge->>OVS: CreateOVSBridge(bridgeConfig)
        OVS->>OVS: Validate uplinks list (len >= 1)
        OVS->>OVS: Save config to store
        OVS->>OVS: Delete all uplink interfaces from any existing bridges
        OVS->>OVS: Delete/recreate bridge if config changed
        OVS->>OVS: Create bridge with specified config
        loop For each uplink in config
            OVS->>OVS: Add uplink interface to bridge
        end
    end

Loading

[greptile-apps]

_{8 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

syntax: Comment is outdated - all uplinks are added in CreateOVSBridge() via the loop at ovs.go:182-196, not via separate AddInterfaceToOVSBridge calls

Suggested change

	// applyGroupedBridgeConfig creates a single OVS bridge entry with all matching interfaces as uplinks
	// when groupingPolicy is "all". The first uplink will be used to create the bridge,
	// and the remaining uplinks will be added by the daemon using AddInterfaceToOVSBridge.
	// applyGroupedBridgeConfig creates a single OVS bridge entry with all matching interfaces as uplinks
	// when groupingPolicy is "all". All uplinks will be added to the bridge by CreateOVSBridge.