Skip to content

fix(webhook): allow non-bridge NADs - #444

Open
abonillabeeche wants to merge 1 commit into
harvester:masterfrom
abonillabeeche:fix/nad-webhook-non-bridge-cni
Open

abonillabeeche wants to merge 1 commit into
harvester:masterfrom
abonillabeeche:fix/nad-webhook-non-bridge-cni

Conversation

@abonillabeeche

Copy link
Copy Markdown

Problem:
The NAD mutator and validator treat every NAD that is not kube-ovn as a bridge NAD and look up the cluster network from its bridge name. ipvlan, macvlan, host-device and SR-IOV NADs have no bridge field, so they are rejected with "the length of bridge name is less than 3" and cannot be created on Harvester at all. This blocks attaching Longhorn instance managers to an RDMA NIC (harvester/harvester#11989), among other uses.

Solution:
Add NetConf.IsHarvesterManagedCNI() (bridge or kube-ovn) and apply the bridge-specific logic only to those NADs:

  • mutator: skip the MTU patch on create, and the cluster network labels and route annotations on update;
  • validator: skip the VLAN overlap, bridge name and cluster network/MTU checks.

For other CNI types the webhook still validates the route annotation, keeps the CNI type immutable, and keeps the in-use VM/VMI and system network checks on update. Delete is unchanged.

Related Issue:
harvester/harvester#11990

Test plan:

  • New unit tests (pkg/webhook/nad/non_bridge_test.go): ipvlan, macvlan and host-device NADs pass create; an invalid route annotation is still rejected; a non-bridge NAD can be updated but its CNI type cannot change; a bridge NAD on a missing cluster network is still rejected; the mutator returns no patch for non-bridge NADs. Existing webhook tests pass; golangci-lint v2.12.2 reports 0 issues.
  • On a 3-node Harvester v1.9.1-rc3 cluster, with the webhook image rebuilt from v1.9.1-rc3 plus this commit:
    • ipvlan NAD: created without any workaround, stored unmodified;
    • updating its IPAM range: accepted;
    • changing its type to macvlan: rejected;
    • bridge NAD pointing at a non-existent cluster network: still rejected ("clusternetworks... not found").

The NAD mutator and validator treated every non-kube-ovn NAD as a bridge
NAD and looked up the cluster network from the bridge name, so ipvlan,
macvlan, host-device and SR-IOV NADs were rejected with "the length of
bridge name is less than 3".

Apply the bridge-specific logic (MTU patch, cluster network labels and
route annotations, VLAN overlap, bridge name and cluster network checks)
only to Harvester-managed NADs (bridge and kube-ovn). For other CNI
types, still validate the route annotation, keep the CNI type immutable,
and keep the in-use VM and system network checks on update.

Related: harvester/harvester#11990
Signed-off-by: Alejandro Bonilla <abonilla@suse.com>

@w13915984028 w13915984028 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonillabeeche Thanks for the updating, webhook part looks good, did you check the controller code like below:

// nad manager controller ensures all labels and sync cn
func (h Handler) OnChange(_ string, nad *cniv1.NetworkAttachmentDefinition) (*cniv1.NetworkAttachmentDefinition, error) {
if nad == nil || nad.DeletionTimestamp != nil {
return nil, nil
}
logrus.Infof("nad configuration %s/%s has been changed: %s", nad.Namespace, nad.Name, nad.Spec.Config)
netconf, updated, err := h.ensureLabels(nad)
if err != nil {
return nil, fmt.Errorf("ensure labels of nad %s/%s failed, error: %w", nad.Namespace, nad.Name, err)

@w13915984028

Copy link
Copy Markdown
Member

@abonillabeeche As those non-bridge/OVN NADs are not managed by Harvester mutators/controllers, we could consider to add a new label/annotation (e.g. network.harvesterhci.io/external-nad

KeyVlanLabel = network.GroupName + "/vlan-id"
) upon them, and network controller skips checking/processing on any OOB NADs with this label.

@abonillabeeche

Copy link
Copy Markdown
Author

Thanks for catching this, @w13915984028 — you're right, I'd only checked the webhook path.

I traced it through: OnChange (controller.go:162) runs unconditionally for every NAD and calls ensureLabels, which calls SetNetworkInfoToLabels/GetClusterNetworkName (pkg/utils/nad.go). Both hard-error for anything that isn't bridge/empty or kube-ovn, so once this PR lets ipvlan/macvlan/host-device/SR-IOV NADs through the webhook, the manager controller will reconcile-error on them forever ("cannot determine the network type from netconf type %v").

OnCNChange and OnRemove look safe in practice — both only act on NADs that already carry KeyClusterNetworkLabel, which non-Harvester-managed NADs never get (since ensureLabels errors before setting it).

Two ways I see to fix the controller side, and I'd like your take before I push either:

  1. Reuse the IsHarvesterManagedCNI() helper this PR already adds (pkg/utils/nad.go) and early-return in OnChange (and OnRemove for symmetry/defensiveness) for non-Harvester-managed NADs — same pattern the webhook mutator/validator already use. Minimal diff, no new persisted state.
  2. Your suggestion: add a network.harvesterhci.io/external-nad label (stamped by the mutator) and gate the controller on that label instead of re-decoding spec.Config. More surface area (new label + mutator write), but makes the classification queryable directly off the NAD without decoding JSON.

Since these NADs were rejected outright before this PR, there's no pre-existing-NAD migration concern either way. Which direction would you prefer?

@w13915984028

Copy link
Copy Markdown
Member

@abonillabeeche

Need to understand a bit more about the user stories. How are such new NADs created and used? We also need to ensure the UI can correctly filter them, e.g. while create/edit VM network, create/edit VM to refer the VM network (both on Harvester mode and Rancher imported mode). The current solution for Storge-network/rwx-network is to hide those specific NADs.

In short:
(1) The lifecycle management. who creates/updates/deletes, who uses it?
(2) The related CNI driver,is it shipped by RKE2 by default or packaged to Harvester OS or installed on day-2? thanks.

@abonillabeeche

Copy link
Copy Markdown
Author

@w13915984028 thanks, good questions.

Why this is needed: Longhorn's V2 data engine is gaining an RDMA (RoCEv2) transport (harvester/harvester#11989). For that to work, the instance-manager pods need an interface directly on the RDMA NIC. A Linux bridge can't provide one: the pod's veth is a software device with no RDMA capability (no RoCE GID), so the storage traffic falls back to TCP. An ipvlan (or macvlan/host-device/SR-IOV) attachment puts the pod on the physical RDMA port, so it can use the NIC's verbs device (/dev/infiniband). This is RoCE, meaning RDMA over our normal Ethernet NICs. It uses the same verbs/"infiniband" stack, but no InfiniBand fabric is involved. Today the webhook rejects any non-bridge NAD, so this can't be set up at all.

(1) Lifecycle: this isn't a VM network. In the target design (HEP #11480), the storage-network controller creates, updates and deletes the NAD in longhorn-system when the storage network is set to RDMA mode, just as it manages the bridge storage-network NAD today. The only consumers are the Longhorn instance-manager pods. Until that lands, an admin creates it by hand. So the UI should hide these NADs the same way it hides the storage-network/rwx NADs. Your network.harvesterhci.io/external-nad label idea fits this well: the controller skips anything carrying the label and the UI filters on it, in both Harvester and Rancher-imported modes. I'm happy to go that way.

(2) CNI drivers: nothing is installed day-2. ipvlan, macvlan and host-device are standard CNI reference plugins, already on every Harvester node next to bridge, and whereabouts IPAM is already there too. I created the ipvlan NAD on a stock v1.9.1-rc3 cluster without adding any binaries. SR-IOV would need sriov-cni, but it's optional and not required for this use case.

@w13915984028

Copy link
Copy Markdown
Member

@abonillabeeche Thanks. It becomes cleaner now. A few additional questions. Is RDMA an replacement of SN? Is whereabouts/IP management still involved ? How does LH know it's RDMA or not. How to check each node is meeting the requirements to have RDMA-ready nics for usage. Without the help of network controller, user needs to manually ensure such things.

@abonillabeeche

Copy link
Copy Markdown
Author

@w13915984028 good questions; the full design is in HEP harvester/harvester#11480. Short answers:

  1. Is RDMA a replacement for the storage network? No, it's a variant of it. The HEP's current draft adds mode: rdma to the existing storage-network setting and reuses clusterNetwork + VlanConfig (nodeSelector) to resolve the per-node uplink, so mixed NIC names work as they do today. In that mode the controller builds a bridgeless macvlan/ipvlan NAD instead of the <cn>-br bridge NAD and syncs it to Longhorn the same way. Whether it should be a mode on the setting or a dedicated CRD (like HostNetworkConfig) is an open question in the HEP, and your input there would be very welcome. One RDMA storage network carries both TCP and RDMA volumes.
  2. Is Whereabouts still involved? Yes, unchanged. It's chained purely as the NAD's ipam plugin, independent of the main CNI type (confirmed with ipvlan-over-PF).
  3. How does Longhorn know it's RDMA? The transport is chosen per volume: StorageClass parameter dataEngineTransport: tcp|rdma, changeable only while detached ([FEATURE] V2 Engine Supports RDMA (RoCEv2) NVMe-oF transport - Experimental longhorn/longhorn#13796, implemented in feat(volume): RDMA (RoCEv2) transport support for the v2 data engine longhorn/longhorn-manager#5107). Longhorn sets an RDMACapable node condition from /sys/class/infiniband, rejects RDMA volumes unless they are V2 and a capable node exists, rejects attaching to non-capable nodes, and places RDMA replicas only on capable nodes.
  4. How are nodes checked for RDMA-ready NICs? Before configuring, longhornctl check preflight --enable-rdma (feat(preflight): check and install RDMA prerequisites for the V2 data engine longhorn/cli#660) checks the modules and devices. In rdma mode the HEP adds a storage-network webhook branch that validates the uplink exists and is RoCE-capable on all non-witness nodes, that the subnet has at least node-count addresses, and keeps the volumes-detached / VMs-stopped guard. Harvester surfaces Longhorn's RDMACapable condition to the UI instead of re-detecting it.

You're right that until the rdma mode lands, all of this is manual. This PR only stops the webhook from rejecting the NAD. The admin-created NAD would carry the network.harvesterhci.io/external-nad label, so the controller skips it and the UI hides it like the storage-network/rwx NADs. If that matches your expectations, I'll push the label approach here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants