fix(plugin): honor OvnPort/MAC from CNI args.cni in StdinData - #542
Conversation
multus-cni propagates per-pod cni-args from the `k8s.v1.cni.cncf.io/networks[].cni-args` pod annotation by injecting them into the delegate plugin's StdinData under `args.cni.<Key>`, NOT via the CNI_ARGS environment variable. ovs-cni reads OvnPort and MAC only from CNI_ARGS via cnitypes.LoadArgs, so values set through the standard multus annotation path silently never reach the plugin: the veth is added to the bridge but external_ids:iface-id is never set, ovn-controller never binds the LSP to a chassis, and the pod has no L2/L3 connectivity through OVN. Add an Args field to NetConf that captures the CNI 0.4.0+ `args.cni` map from StdinData, and have the veth/sriov/vdpa CmdAdd/CmdDel/CmdCheck paths fall back to it (via a new common.ApplyConfArgsFallback helper) when CNI_ARGS did not supply OvnPort or MAC. CNI_ARGS keeps priority, so callers that already pass values via env (e.g. kubevirt virt-launcher) are unaffected. Design choices reflect review feedback on the original patch: * PluginArgs.Cni uses map[string]json.RawMessage rather than map[string]string, so an unrelated array/object entry under args.cni (for example the IPAM-style `ips` array) does not break NetConf unmarshal — values are decoded lazily only for the keys the fallback consumes. * Key matching is case-insensitive: pod annotations written as `ovnPort`, `OvnPort`, or `ovnport` all resolve. * The helper accepts nil pointers for either output, so CmdDel and CmdCheck (which only care about ovnPort) need no dummy variables. End-to-end verified on a Neutron+OVN cluster with multus-cni 4.2.4: created Neutron ports on a geneve tenant network and a flat provider network, attached pods via the `cni-args.OvnPort` annotation, confirmed external_ids:iface-id is set automatically, ovn-controller binds both LSPs (chassis populated, up=true, ovn-installed=true), and L2 ping between pods on the same network works for both overlay and underlay. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Simon Zhou <jp.zdm2008@gmail.com>
kubevirt-bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fivetime The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubevirt-bot
added
the
needs-ok-to-test
WalkthroughAdds typed CNI ChangesCNI Args Fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin command
participant Fallback as ApplyConfArgsFallback
participant Args as NetConf.Args.Cni
participant Decoder as decodeArgString
Plugin->>Fallback: Resolve empty mac or ovnPort
Fallback->>Args: Match CNI argument key
Args-->>Fallback: Return json.RawMessage
Fallback->>Decoder: Decode JSON string
Decoder-->>Fallback: Return value or invalid result
Fallback-->>Plugin: Populate resolved pointer
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Hi @fivetime. Thanks for your PR. I'm waiting for a k8snetworkplumbingwg member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
LGTM can you please extend the readme documentation to explain this new feature.
also another point that I am not sure here is if we want to have the strings.ToLower or be specific on the parameter name
|
/ok-to-test |
kubevirt-prow
Bot
added
ok-to-test
Explain, in docs/cni-plugin.md and the README, how the per-invocation mac and ovnPort parameters are supplied — either via CNI_ARGS or, under Multus, via a network's cni-args (delivered as args.cni in StdinData) — and note that key matching is case-insensitive. Addresses review feedback on k8snetworkplumbingwg#542. Signed-off-by: Simon Zhou <jp.zdm2008@gmail.com>
|
@SchSeba thanks for the review! I've pushed a follow-up commit (c2afce3) adding documentation for this feature — a new Per-Pod Arguments ( On
On determinism: the loop takes the first non-empty match and returns, and a single annotation realistically won't carry two differently-cased spellings of the same key — but if you'd prefer, I'm happy to add an explicit guard (or restrict to a small documented canonical set: If you'd still rather have strict exact-match, that's an easy change — just let me know and I'll switch it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/cni-plugin.md`:
- Around line 65-74: Update the earlier bridge description to state that ovnPort
may be supplied through either CNI_ARGS or Multus args.cni, and that bridge can
be omitted in both supported per-invocation paths. Keep the existing bridge and
ovnPort behavior descriptions unchanged otherwise.
- Around line 132-138: Update the CNI_ARGS documentation example so it is valid
copyable shell syntax: show CNI_ARGS assigned as part of an actual ovs-cni
invocation, or export it and then provide a commented invocation instruction; do
not leave the placeholder “...” as the command to execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b9bb36f-7960-4f50-be4b-50ecce6df93b
📒 Files selected for processing (2)
README.mddocs/cni-plugin.md
| The following are *per-invocation* arguments rather than static network | ||
| configuration. They are not set in the `NetworkAttachmentDefinition` `config` | ||
| body, but passed per pod through CNI args (see | ||
| [Per-Pod Arguments](#per-pod-arguments-mac--ovnport)): | ||
|
|
||
| * `mac` (string, optional): MAC address to assign to the container interface. | ||
| * `ovnPort` (string, optional): value written to the OVS interface's | ||
| `external_ids:iface-id`, used to bind the port to an OVN logical switch port. | ||
| When set, `bridge` may be omitted (it is derived from the port). | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the earlier bridge omission reference to include Multus cni-args.
The existing bridge description at Line 53 still says ovnPort must be set in CNI_ARGS, although this PR also supports args.cni from Multus. Update that reference so users do not incorrectly configure bridge when supplying ovnPort through the documented Multus path.
🤖 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 `@docs/cni-plugin.md` around lines 65 - 74, Update the earlier bridge
description to state that ovnPort may be supplied through either CNI_ARGS or
Multus args.cni, and that bridge can be omitted in both supported per-invocation
paths. Keep the existing bridge and ovnPort behavior descriptions unchanged
otherwise.
| ### 1. `CNI_ARGS` environment variable | ||
|
|
||
| When invoking the plugin directly, pass them as `CNI_ARGS`: | ||
|
|
||
| ```shell | ||
| CNI_ARGS="mac=0a:58:0a:f4:00:07;ovnPort=my-logical-port" ... | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the CNI_ARGS example valid shell syntax.
CNI_ARGS="..." ... attempts to execute ... as a command when copied. Show an actual invocation or use export CNI_ARGS=... followed by a comment instructing the reader to invoke ovs-cni.
🤖 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 `@docs/cni-plugin.md` around lines 132 - 138, Update the CNI_ARGS documentation
example so it is valid copyable shell syntax: show CNI_ARGS assigned as part of
an actual ovs-cni invocation, or export it and then provide a commented
invocation instruction; do not leave the placeholder “...” as the command to
execute.
Explain, in docs/cni-plugin.md and the README, how the per-invocation mac and ovnPort parameters are supplied — either via CNI_ARGS or, under Multus, via a network's cni-args (delivered as args.cni in StdinData) — and note that key matching is case-insensitive. Addresses review feedback on k8snetworkplumbingwg#542. Signed-off-by: Simon Zhou <jp.zdm2008@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fivetime, SchSeba The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-prow
Bot
added
the
approved
4229115
into
k8snetworkplumbingwg:main
3 participants
What this PR does / why we need it:
Fixes a silent integration bug between ovs-cni and multus-cni that prevents the
ovnPortfeature from working when invoked through the standard multus annotation path.Background. ovs-cni's plugin reads
OvnPort(andMAC) only from theCNI_ARGSenvironment variable viacnitypes.LoadArgs(args.Args, &EnvArgs{}). multus-cni, however, takes thecni-argsmap from a pod'sk8s.v1.cni.cncf.io/networks[].cni-argsannotation and injects it into the delegate plugin's StdinData underargs.cni.<Key>, not viaCNI_ARGS. As a result:external_ids:iface-idis never set,ovn-controllernever binds the OVN logical switch port to the chassis,There is no warning anywhere in the stack: multus passes its own
IgnoreUnknown=1to delegates, so even if a user tries the lower-casecni-args.ovnPortkey the unknown-args check is suppressed and the request appears to succeed.Fix. Add an
Argsfield toNetConfcapturing the CNI 0.4.0+args.cnimap from StdinData, and have the veth/sriov/vdpaCmdAdd/CmdDel/CmdCheckpaths fall back to it (via a newcommon.ApplyConfArgsFallbackhelper) whenCNI_ARGSdid not supplyOvnPortorMAC.CNI_ARGSkeeps priority, so callers that already pass values via env (e.g. kubevirt virt-launcher) are unaffected.This PR supersedes #529. That PR was filed before the vhost_vdpa refactor (#427) split the plugin into per-backend packages; this rebased version applies the same fix in all three backends and addresses the automated review feedback on #529:
PluginArgs.Cniismap[string]json.RawMessageinstead ofmap[string]string, so an unrelated array/object entry underargs.cni(for example the IPAM-styleipsarray) does not breakNetConfunmarshal — values are decoded lazily only for the keys the fallback consumes.ovnPort,OvnPort, orovnportall resolve.nilpointers for either output, soCmdDel/CmdCheck(which only care aboutovnPort) need no dummy variables.Special notes for your reviewer:
End-to-end verified on a real Neutron + OVN cluster (multus-cni 4.2.4, OVN 24.x, Open vSwitch 3.x):
provider:network_type=geneve) and a flat provider network (provider:network_type=flat, mapped tobr-exvia OVNbridge_mappings).br-intwith emptyexternal_ids, OVN SBport_binding.chassis=[], ping 100 % loss. The fix only worked when an operator manually ranovs-vsctl set interface <veth> external_ids:iface-id=<uuid>.external_ids:iface-idandovn-installed=trueshow up automatically, OVN SB binds both LSPs to the local chassis (up=true), and ping between pods works for both overlay (geneve) and underlay (flat localnet) — 0 % loss, RTT 0.1–7 ms.The change is intentionally minimal: a
nil-guarded helper plus nine one-line call sites (three backends x ADD/DEL/CHECK). CNI_ARGS still wins when both paths are present, so downstream consumers like kubevirt are not affected.Release note:
Summary by CodeRabbit
Release Notes
New Features
macandovnPortwhen not provided via environment/config.Improvements
mac/ovnPortinitialization across veth, SR-IOV, and vDPA workflows, including DEL/CHECK paths.macandovnPortviacni-args/CNI_ARGS, including case-insensitive matching and handling of unrecognized/invalid entries.