Support vhost_vdpa devices#427
Conversation
kubevirt-bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bgartzi 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
needs-rebase
|
Hi @bgartzi. 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 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. |
We will need govdpa later to find pciaddr-vdpa relationships and setting mac addresses to vdpa devices. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
This is some logic that we might need to reuse in other use-cases such as vdpa-vhost devices. Move the logic into a function so we're not rewriting it. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Other device types such as vdpa-vhost need to move the VF into the pod's network namespace. Make the function publicly available for those cases to use. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
As in previous commits, others might need to run similar steps, so make it public to be reused. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
This commit implements ovs-cni's support for vhost-vdpa attachments. These devices are provisioned by the sriov-network-operator in switchdev mode [0]. That means that the process to attach these devices to pods is similar to the one followed for sriov devices that work on switchdev mode: attach the representor to the target ovs bridge and move the VF into the target pod's network namespace. There is one little difference though: the mac address received through the environment variables (if any) needs to be set onto the vdpa device, not the VF. This is possible since vdpa dev set mac supported in kernels 6.12 and above. This commit adds a skeleton for a vdpa subpackage, and wires everything up to the CmdAdd command. Note that this only adds support for kernel vhost-vdpa devices (those whose mgmtdev is a sriov VF), but that in the future this could be extended to virtio-vdpa and vduse vdpa. [0] https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/4d96f5c4d18ac4fbbdbbd0bde4b78383955ba285/doc/vdpa.md?plain=1#L13C1-L13C35 Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
kubevirt-bot
removed
the
needs-rebase
|
Hi @SchSeba, could you have a look into this patch? |
Summary of ChangesHello @bgartzi, 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 extends ovs-cni's capabilities by integrating support for vhost-vdpa devices. The changes enable the CNI plugin to properly configure and manage these virtualized network interfaces, adapting existing SR-IOV patterns where applicable while accounting for the unique characteristics of vdpa devices, particularly concerning MAC address assignment and network namespace interactions. This enhancement broadens the range of high-performance networking options available to Kubernetes pods. 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 vhost_vdpa devices to ovs-cni, which is a valuable enhancement. The changes are well-structured, introducing a new vdpa package for device-specific logic and refactoring the sriov package for better code reuse. The main logic in CmdAdd correctly prioritizes vdpa device handling. My feedback includes a couple of suggestions to improve error messages for better debuggability.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to run vdpa netlink command") | ||
| } |
There was a problem hiding this comment.
The error message here is a bit generic. Wrapping the original error with more context, such as the device ID, would be more helpful for debugging.
| if err != nil { | |
| return nil, fmt.Errorf("failed to run vdpa netlink command") | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get vdpa devices for PCI address %q: %w", deviceID, err) | |
| } |
| switch (*vdpaDev).Driver() { | ||
| case kvdpa.VhostVdpaDriver: | ||
| return VdpaDeviceTypeKernelVhost, nil | ||
| default: | ||
| return VdpaDeviceTypeNone, fmt.Errorf("unknown vdpa device type") | ||
| } |
There was a problem hiding this comment.
The error message "unknown vdpa device type" could be more informative. Including the actual driver name that was not recognized would aid in debugging.
| switch (*vdpaDev).Driver() { | |
| case kvdpa.VhostVdpaDriver: | |
| return VdpaDeviceTypeKernelVhost, nil | |
| default: | |
| return VdpaDeviceTypeNone, fmt.Errorf("unknown vdpa device type") | |
| } | |
| switch driver := (*vdpaDev).Driver(); driver { | |
| case kvdpa.VhostVdpaDriver: | |
| return VdpaDeviceTypeKernelVhost, nil | |
| default: | |
| return VdpaDeviceTypeNone, fmt.Errorf("unknown vdpa device type: %q", driver) | |
| } |
2 participants
This PR adds support for vhost_vdpa devices to ovs-cni.
Most of the steps needed to set up a vhost_vdpa device are similar to the ones of sriov devices. However, instead of setting the mac address to the VF that is moved into the pod namespace, it is the vdpa device that needs to be updated.
Setting a mac address to vdpa devices is supported from kernel 6.12 on.
If I understand correctly, there's no proper way to make IPAM work directly as the vdpa interface would be under the VM, not the host or the pod.