docs: add DPDK-accelerated underlay enhancement - #549

The table lists the mode as CNIDevice, but the actual enum value in the API types is CNI (as defined in UnderlayInterface and used in the YAML examples of the previous enhancement). It would be clearer to use CNI in the table to avoid confusion.

Suggested change:

| Mode | Behavior | IPAM | Datapath |
|------|----------|------|----------|
| `NetworkDevice` | Moves host device; grout creates TAP+`remote=` | Native (CIDR-derived) | TAP PMD (kernel TC redirect) |
| `CNI` | Invokes CNI plugin | Delegated to CNI | Kernel or TAP PMD |
| `GroutPort` | Binds a VF to grout as a DPDK port | Inline in spec | DPDK PMD (user-space) |

I think gemini hallucinated.

Let's align everything w/ the Device suffix - please call this new attribute GroutDevice.

The VFSelector union struct defines NetdevName as a valid selector option, and the resolution flow describes how it is resolved. However, it is missing from the "VF Selection" summary table. It should be added for completeness.

Suggested change:

| Selector | Use case |
|----------|----------|
| `pciAddress` | Exact VF PCI Address (e.g. `0000:03:02.0`) |
| `netdevName` | VF Netdev Name (e.g. `eth1`) |
| `pfName`, `vfIndex` | PF name + VF index (e.g. `enp3s0f0` + `2`) |

The YAML example does not match the proposed Go API types. GroutPortConfig contains a nested VFSelector struct under the vfSelector field, which is a union and requires a type discriminator. The fields pciAddress and ipam are currently shown directly under groutPort without the vfSelector wrapper.

Similar to the first example, the YAML here does not match the proposed Go API types. The pfName and vfIndex fields should be nested under vfSelector.pfVFIndex, and the type discriminator PFVFIndex must be specified.

The comment states "At most one IPv4 and one IPv6 (dual-stack)." However, the kubebuilder validation only limits the slice size to 2 (MaxItems=2). To enforce the dual-stack constraint (preventing two IPv4 or two IPv6 addresses), CEL validation rules should be added, similar to those used in TunnelEndpointConfig.

Suggested change:

type GroutPortIPAM struct {
	// At most one IPv4 and one IPv6 (dual-stack).
	// +kubebuilder:validation:MinItems=1
	// +kubebuilder:validation:MaxItems=2
	// +kubebuilder:validation:XValidation:rule="self.all(c, isCIDR(c))",message="all entries must be valid CIDRs"
	// +kubebuilder:validation:XValidation:rule="self.filter(c, isCIDR(c) && cidr(c).ip().family() == 4).size() <= 1",message="at most one IPv4 address is allowed"
	// +kubebuilder:validation:XValidation:rule="self.filter(c, isCIDR(c) && cidr(c).ip().family() == 6).size() <= 1",message="at most one IPv6 address is allowed"
	Addresses []string `json:"addresses"`
}

There is a placeholder comment with a typo CTODO instead of TODO. This should be cleaned up or properly defined as part of the proposed API.

Slight precision needed: bifurcated DPDK drivers do have a kernel netdev which is used (required in fact) only for control plane operations (MTU change, link state, mac address filter, etc.). But the IP address must not be configured on it.

What component is expected to allocate the VFs? Should it be delegated to nmstate?

If so, who will take care of re-binding the device to vfio-pci (most DPDK drivers) or move the linux netdev to the router namespace (bifurcated drivers, e.g. mlx5)?

What component is expected to allocate the VFs? Should it be delegated to nmstate?

yes, either nmstate or any other configuration would be on the user. At least at this stage of the development.

If so, who will take care of re-binding the device to vfio-pci (most DPDK drivers) or move the linux netdev to the router namespace (bifurcated drivers, e.g. mlx5)?

For vfio-pci driver, the controller will do nothing on the VF during setup/teardown. Just passing the PCIAddress to a grcli interaface add ...

For bifurcated drivers, the controller will also move the netlink in and out of the perouter namespace during setup/teardown

why a user would want this if they have story 3? Also, the pci address is likely to be different on each node I think

In real applications, this lets you bind PFs without needing to use SR-IOV or VFs.

for testing purposes, this will become useful when we'll deal with qEMU environment.

In real applications, this lets you bind PFs without needing to use SR-IOV or VFs.

not sure I understood what you mean by "needing to user sr-iov"

for testing purposes, this will become useful when we'll deal with qEMU environment.

In real applications, this lets you bind PFs without needing to use SR-IOV or VFs.

not sure I understood what you mean by "needing to user sr-iov"

for testing purposes, this will become useful when we'll deal with qEMU environment.

A DPDK application can run on a Physical Function by using its PCI address. The fact that the SR-IOV technology has been invented is to let multiple DPDK application use the same NIC, each of them using a different VF. But it's not mandatory.

Also, there are NICs that does not support SR-IOV (i.e. VFs) but can be used by Grout and DPDK.
@rjarry do you mind checking this last statement?

fine, but then we need to reword where we talk about vfs because it's not really only about vfs.

rewording

Sorry I had missed this notification. Yes it would make sense you allow selecting interfaces by explicit PCI address for qemu testing purposes (qemu igc emulated sriov devices do not handle VF to VF communication unless the same vlan id is explicitly configured on the two VFs hardware filters (which grout does not do). Since we need one of the VFs to be a "trunk", we cannot use that solution to run virtualized tests.

The alternative solution is to use plain virtio interfaces and give them to grout as PFs. Then plug the host side tap interfaces to a Linux bridge that will perform the vlan switching (emulating the internal igc VF switch).

I feel like groutport is a bit generic. We should mention it's a sriov vf, and optionally that it relates to grout. Something like acceleratedVF? FastDatapathVF? dpdkVF?

It can also be a Physical Function.
Besides that, I'm open to suggestions

but the enhancement describes vfs only, mind expanding then?

updated the description

how are addresses assigned? The proposal doesn't say that

That's a good point, and I'm still in the process of defining it.
My current ideas are:
a. put an GroutDevice.Addresses map[string]string[] map array with all the addresses to add to the port for each node (the map is nodeName -> list of addresses)

b. add a GroutDevice.CIDR field and use the nodeIndex to assign IP Address.

c. The user puts the addresses on the network device and the perouter reads them. This is my favorite, but it also implies
c1. The perouter takes care of binding/unbinding the driver of the device (i.e. an Intel device is configured by the user as a netlink device, then the controller saves the addresses to a file and binds the vfio-pci driver)
c2. The controller needs to store the IP addresses in some persistence layer, make the SetupUnderlay a stateful operation

we don't have underlay status

ok, removing

do we envision allowing dhcp too?

integrating grout w/ any CNI based approach will not be straightforward.

Grout takes away the IPs to the user space, which will cause CNI CHECK to fail.

This integration deserves - IMHO - a separate enhancement (or a new chapter focusing on the grout integration).

I think gemini hallucinated.

Let's align everything w/ the Device suffix - please call this new attribute GroutDevice.

integrating grout w/ any CNI based approach will not be straightforward.

Grout takes away the IPs to the user space, which will cause CNI CHECK to fail.

This integration deserves - IMHO - a separate enhancement (or a new chapter focusing on the grout integration).

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the Markdown heading hierarchy.

Use level-3 headings for the four user stories under ## User Stories. Use level-4 headings for the API examples under ### API Examples. This resolves markdownlint MD001.

Also applies to: 62-62, 66-66, 71-71, 140-140, 164-164, 186-186

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@enhancements/grout-dpdk-underlay.md` at line 57, The four user story headings
(including Story 1: High-Throughput Underlay and the other stories at lines 62,
66, 71) are currently using level-4 heading syntax (####) but should use level-3
heading syntax (###) since they are direct children under the ## User Stories
section. Update each of these four story headings from #### to ### to establish
the correct hierarchy. The API Examples sections that appear under each story
(at lines 140, 164, 186) should remain as level-4 headings (####) as they are
nested under the story sections.

Source: Linters/SAST tools

yes, this should be using level 3 headings (remove one hash).

@maiqueb, agreed. Change each user story heading from #### to ###. Keep the nested #### API Examples headings unchanged.

_{You are interacting with an AI system.}

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Reserve the PCI device before writing shared state.

Two Underlays can write the same /var/run/openperouter/grout/<pci-address>.json before grcli reports the device as busy. The later write can replace originalDriver and addresses; teardown can then restore the wrong state or disrupt the active owner. Acquire a per-PCI reservation and atomically create state with the owning resource identity before any host mutation.

Also applies to: 257-272, 315-315

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@enhancements/grout-dpdk-underlay.md` around lines 120 - 123, Update the
Underlay setup flow around saving original state to reserve each PCI device
before any host mutation or shared state write. Acquire a per-PCI reservation,
then atomically create the state file with the owning resource identity; reject
competing owners before allowing them to overwrite originalDriver or addresses.
Apply the same reservation and ownership handling to the corresponding flows
identified near the other referenced sections, and release the reservation
during teardown.

this is needed IMHO.

@maiqueb, agreed. The proposal should define a per-PCI reservation before it writes the state file or changes the device. The state file should record the owning Underlay identity and use atomic creation. Teardown should verify ownership before restoration and release the reservation after successful cleanup.

🐇 ✅

_{You are interacting with an AI system.}

are you sure? do we handle multiple Underlays host configuration in parallel for the kernel datapath?
also, do we need to handle the case two different Underlays objects target the same device? Isn't it handled by webhooks?

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

are you sure? do we handle multiple Underlays host configuration in parallel for the kernel datapath? also, do we need to handle the case two different Underlays objects target the same device? Isn't it handled by webhooks?

you're right to push back - we can't have multiple underlays targeting the same node. My bad on not realizing the implications of this comment.

fine, but then we need to reword where we talk about vfs because it's not really only about vfs.

is this vf only?

fixing

wrong indentation? github is showing this differently

I am having second thoughs about this. We are mixing how we are selecting the interface and what we are going to do with them.
Basically, when we run in grout mode, the differences between networkdevice and groutdevice.netlink name is only how we are going to operate it.

I wonder if the selection mechanism should be part of networkdevice in any case, with networkdevice being expanded as what you are suggesting for groutdevice, with an underlayacceleratedmode knob that tells how to use it (with the proper validation of what can and can't be done on each mode).

If I got your idea, it would become:

Underlay
 .Spec
  .NetworkDevice
     // selects the device by netlink name. kernel mode stays like it is right now. 
     // grout mode would create a TAP device over it unless .AcceleratedDatapath.Enabled is active)
    .InterfaceName     

     // select the device by PF netlink name and VF index number. 
     // Once the device is selected, kernel and grout mode acts as per InterfaceName
    .PFName, .VFIndex

     // select the device via its PCI address. in kernel mode, it's mandatory that 
     //  the device binds a kernel driver. in grout mode, it can even be a DPDK driver. 
     //(though if it's bound to a kernel driver, the controller can steal the addresses)
    .PCIAddress         

   .AcceleratedDatapath   
       .Enabled        // Valid only if in grout mode. if true, create grout PCI ports instead of TAP devices
       .MTU, RXQueues, QSize // tune grout interface creation

I don't see any advantage or drawback in moving to this format.

@maiqueb, any thoughts on these API shapes?

shouldn't PFName and VFIndex be attributes of AcceleratedDatapath ?

also, I think the enabled attribute redundant - if you're specifying the AcceleratedDatapath configuration, then, you want it enabled. If you don't, well ... you don't want it.

shouldn't PFName and VFIndex be attributes of AcceleratedDatapath ?

it selects a device, so it can also be used for the kernel datapath, in case we want to use a VF. But it's probably a scenario that has absolutely no meaning in the real world.

What if we start with just the netlink name only (maybe specified via .NetworkDevice.InterfaceName) ?

in general, the semantics of networkdevice is "I want to select this device"
on the other hand, the previous version had the device selection semantic (pci address, netlink name, etc) and also the implicit semantic of doing acceletared datapath for that.

My proposal was to keep the concerns separate, the selection on one side and the acceleration on the other.

I update the doc to meet this new approach. I dropped the PCIAddress and PFName+VFIndex device selectors.

I think this is a way simpler initial approach. the user has to put the ip address on the netlink device, as he would for the kernel datapath.

If the Underlay.Spec.Interfaces[].NetworkDevice.Grout field is not nil, the controller knows it has to set up a DPDK port.

I chose Grout as the field name because AcceleratedDatapath is too generic. subfields there are meaningful to grout, and other accelerated implementations (if there will be any) will probably need other knobs.