-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to virtio-vDPA devices #2664
Conversation
/cc @zshi-redhat |
i think it makes sense. maybe govdpa should go to npwg. |
Agree, wasn't there an initiative to move some common stuff to a library in npwg that could be used by more projects? Should sriovnet also go to npwg too? |
e9d23be
to
77d4b68
Compare
Hi @adrianchiris , as we discussed, having sriovnet depend on govdpa does not make much sense since the goal of vdpa is to (eventually) work with all possible vdpa parents (sriov is just one of them). I've moved the govdpa-related stuff to an independent vdpa_linux.go wrapper so it can be mocked and added unit tests. PTAL |
@girishmg @dcbw @danwinship any comments on this? |
@amorenoz could you rebase the PR? Looks OK to me, just needs the rebase. |
@dcbw I rebased and pushed the PR again, but got an error in the ovn-ci / e2e-dual-conversion. Hope this is an expected failure and not due to my changes? |
/retest |
Oops, something went wrong:
|
Interesting, retest doesn't work for long living PR, @dcbw do you know how to rerun the test w/o creating a new PR (the CI failure doesn't seem to be related to the vdpa change)? |
go-controller/go.sum
Outdated
@@ -497,6 +497,8 @@ github.com/juju/utils v0.0.0-20180808125547-9dfc6dbfb02b/go.mod h1:6/KLg8Wz/y2KV | |||
github.com/juju/version v0.0.0-20161031051906-1f41e27e54f2/go.mod h1:kE8gK5X0CImdr7qpSKl3xB2PmpySSmfj7zVbkZFs81U= | |||
github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= | |||
github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= | |||
github.com/k8snetworkplumbingwg/govdpa v0.1.3 h1:FZRhTMB1e3yWwSEy+l4eS73WioyMaL+vmFZ8JNwn+Uk= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmilleri I vaguely remembered that you mentioned we need latest changes in govdpa lib, but this is still using 0.1.3 version, is this expected?
@zshi-redhat I can restart it, but i think it wont restart now because there are conflicting files. If this PR is still important can you rebase @amorenoz and we will review. |
@lmilleri could you please rebase? |
@zshi-redhat there are some changes in the govdpa library under review: k8snetworkplumbingwg/govdpa#15. |
Add support to sriov-based virtio-vdpa devices. When a sr-iov device has a vdpa device created, and it is bound to virtio_vdpa driver, use the associated virtio-net device instead of the vendor one. In order to determine whether a device is vdpa or not as well as its device path, use external library github.com/k8snetworkplumbingwg/govdpa Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
vdpa is an addition on top of ovs_offload so it makes sense to document it as part of it. Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Rebased from master and updated dependency to the newly govdpa v0.1.4 release. |
Please consider this PR on hold, I found a potential issue in govdpa v0.1.4 |
Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
Fixed integration issue with govdpa v.0.1.4. |
/retest |
Oops, something went wrong:
|
@trozet @zshi-redhat @dcbw Can you please take a look at this PR? |
/lgtm mostly just helper code |
Yes, it is mostly helper code apart from go-controller/pkg/cni/cni.go, in which resides the logic for returning the virtio/vdpa device if vdpa device exists. |
@girishmg can you please review it? |
- What this PR does and why is it needed
This PR works in combination with this PR in the SR-IOV Network Device Plugin to provide support for vDPA devices.
This functionality is based on OvS Hardware Offload and allows ovn-kubernetes to expose an open standard netdev such as a virtio-net to the pod.
Most of the heavy-lifting (detecting vdpa devices, ensuring they are bound the right driver, etc) is done by the SR-IOV Network Device Plugin. From a ovn-kubernetes perspective, it's just a matter of selecting the right netdev to move to the pod's namespace.
- Special notes for reviewers
I would like feedback on a number of topics:
If that is not the case we would need to add support to the Device Information Spec so we can read the information coming from the Device Plugin and unambiguously determine the type of device we're handling
pkg/util/sriovnet_linux.go
. It might not be the perfect place as it seems more of a wrapper aroundhttps://github.com/Mellanox/sriovnet
. So this is a question for @moshe010 and @adrianchiris: WDYT? Would you accept this logic intohttps://github.com/Mellanox/sriovnet
?- How to verify it
You need a working OvS Hardware Offload setup with a ConnectX6-DX NIC. Then, follow the instructions that are added in
docs/ovs_offload.md
as part of this PR to configure virtio-vDPA devices.- Description for the changelog
Add support for virtio-vdpa devices