Support reading OvnPort from args.cni#424
Support reading OvnPort from args.cni#424mnaser wants to merge 1 commit intok8snetworkplumbingwg:mainfrom
Conversation
Summary of ChangesHello @mnaser, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the flexibility and integration of the ovs-cni plugin by implementing support for reading Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for reading OvnPort and MAC from args.cni in the CNI configuration, which is a good improvement for compatibility with tools like Multus. The implementation for CmdAdd and CmdDel is correct, and new types and tests have been added.
However, I've identified a critical omission in CmdCheck which will cause it to fail when OvnPort is provided via args.cni. Additionally, the new tests only cover the ADD command, leaving CHECK and DEL for the new configuration path untested. Please see my detailed comments for suggestions on how to address these issues.
| if netconf.Args != nil && netconf.Args.CNI != nil { | ||
| if netconf.Args.CNI.OvnPort != "" { | ||
| ovnPort = netconf.Args.CNI.OvnPort | ||
| } | ||
| if netconf.Args.CNI.MAC != "" { | ||
| mac = netconf.Args.CNI.MAC | ||
| } | ||
| } |
There was a problem hiding this comment.
While this logic correctly handles args.cni for the ADD command, the CmdCheck function has not been updated with similar logic. CmdCheck also uses ovnPort to determine the bridge name, but it currently only reads it from CNI_ARGS. This will cause CHECK to fail when OvnPort is provided via args.cni, as it won't be able to determine the correct bridge name for validation. Please update CmdCheck to also read ovnPort from args.cni with the same precedence to ensure consistent behavior.
| Context("specified OvnPort via args.cni", func() { | ||
| It("should configure an ovs interface with iface-id from args.cni", func() { | ||
| const ovsOutput = "external_ids : {iface-id=test-port-from-args}" | ||
|
|
||
| // OvnPort specified in args.cni (CNI convention) instead of CNI_ARGS | ||
| conf := fmt.Sprintf(`{ | ||
| "cniVersion": "%s", | ||
| "name": "mynet", | ||
| "type": "ovs", | ||
| "bridge": "%s", | ||
| "args": { | ||
| "cni": { | ||
| "OvnPort": "test-port-from-args" | ||
| } | ||
| }}`, version, bridgeName) | ||
|
|
||
| targetNs := newNS() | ||
| defer func() { | ||
| closeNS(targetNs) | ||
| }() | ||
|
|
||
| // Pass empty ovnPort to attach() - it should come from args.cni in config | ||
| result := attach(targetNs, conf, IFNAME, "", "") | ||
| hostIface := result.Interfaces[0] | ||
| output, err := exec.Command("ovs-vsctl", "--column=external_ids", "find", "Interface", fmt.Sprintf("name=%s", hostIface.Name)).CombinedOutput() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(string(output[:len(output)-1])).To(Equal(ovsOutput)) | ||
| }) | ||
| It("should prefer args.cni OvnPort over CNI_ARGS", func() { | ||
| // args.cni should take precedence per CNI conventions | ||
| const ovsOutput = "external_ids : {iface-id=args-cni-port}" | ||
|
|
||
| conf := fmt.Sprintf(`{ | ||
| "cniVersion": "%s", | ||
| "name": "mynet", | ||
| "type": "ovs", | ||
| "bridge": "%s", | ||
| "args": { | ||
| "cni": { | ||
| "OvnPort": "args-cni-port" | ||
| } | ||
| }}`, version, bridgeName) | ||
|
|
||
| targetNs := newNS() | ||
| defer func() { | ||
| closeNS(targetNs) | ||
| }() | ||
|
|
||
| // Pass a different ovnPort via CNI_ARGS - args.cni should win | ||
| result := attach(targetNs, conf, IFNAME, "", "cni-args-port") | ||
| hostIface := result.Interfaces[0] | ||
| output, err := exec.Command("ovs-vsctl", "--column=external_ids", "find", "Interface", fmt.Sprintf("name=%s", hostIface.Name)).CombinedOutput() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(string(output[:len(output)-1])).To(Equal(ovsOutput)) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These new tests for args.cni only cover the ADD command. To ensure the CHECK and DEL commands also function correctly with this new configuration path, please extend these tests to call testCheck and testDel. This follows the pattern of other test cases in this file and will provide more comprehensive test coverage for your changes.
mnaser
force-pushed
the
feat/args-cni-ovnport
branch
2 times, most recently
from
f26ec03 to
eeae2a0
Compare
Add support for reading OvnPort and MAC from args.cni in the CNI configuration, following the CNI conventions specification. Multus and other CNI meta-plugins pass runtime arguments via the args.cni field in the network configuration rather than the CNI_ARGS environment variable. This change enables ovs-cni to work properly when invoked through Multus with cni-args specified in the NetworkAttachmentDefinition. Per the CNI spec, args.cni takes precedence over the deprecated CNI_ARGS environment variable when both are present. This ensures consistent behavior with the specification while maintaining backward compatibility with existing CNI_ARGS users. Changes: - Add CNIArgs struct and Args field to NetConf in types.go - Update CmdAdd and CmdDel to check args.cni for OvnPort/MAC - Add integration tests for args.cni OvnPort handling - Add unit tests for JSON parsing of args.cni Signed-off-by: Mohammed Naser <mnaser@vexxhost.com>
mnaser
force-pushed
the
feat/args-cni-ovnport
branch
from
eeae2a0 to
107d5dd
Compare
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mnaser 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 |
|
Thanks Mohammed, this is exactly what I was looking for, excited for this to land. |
3 participants
What this PR does / why we need it:
Adds support for reading
OvnPortandMACfromargs.cniin the CNI configuration, following CNI conventions.Multus passes
cni-argsfrom pod annotations via theargs.cnifield in the JSON config, not throughCNI_ARGSenvironment variable. This enables ovs-cni to work with Multus annotations like:Per the CNI spec,
args.cnitakes precedence over the deprecatedCNI_ARGSwhen both are present. Full backward compatibility is maintained.Special notes for your reviewer:
This follows the same pattern used by other CNI plugins like
host-localIPAM for readingargs.cni.Release note: