-
Notifications
You must be signed in to change notification settings - Fork 790
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
host-device: Add support for DPDK device #490
Conversation
Is there a reason you can't just use the noop plugin instead? (https://github.com/containernetworking/cni/tree/master/plugins/test/noop) |
@matthewdupre yes, we (@JanScheurich) were also looking for such "dummy" CNI plugin for this purpose. Thanks for pointing it out. |
@pperiyasamy should this PR be closed then? |
Actually, it would simplify life considerably if the host-device plugin could transparently handle the case that the PCI networking device is not bound to a kernel driver and just skip the actions tied to a kernel netdev (e.g. IPAM) in a similar way as the SRIOV CNI plugin on the host skips these actions if the SRIOV VF is not bound to a kernel driver.
Then the user of Kubernetes wouldn’t have to worry about using a special CNI plugin for PCI device NADs in case of Intel VFs or virtio-net devices bound to igb_uio drivers, while in all other cases the host-device CNI is just fine.
|
@@ -43,6 +43,8 @@ const ( | |||
sysBusPCI = "/sys/bus/pci/devices" | |||
) | |||
|
|||
var userspaceDrivers = []string{"vfio-pci", "uio_pci_generic", "igb_uio"} |
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.
I'm a bit ignorant of DPDK; is there no more generic way to detect it than a hard-coded list of names?
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.
AFAIK, this is the way to find the dpdk device as per the driver bound to it (https://doc.dpdk.org/guides/linux_gsg/linux_drivers.html)
if isDpdkMode { | ||
result := current.Result{ | ||
CNIVersion: current.ImplementedSpecVersion, | ||
Interfaces: []*current.Interface{ |
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.
Arguably Interfaces should be empty; given that the interface doesn't exist...
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.
ok, now handling the error returned from hasDpdkDriver method, hope that suffices.
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.
Not exactly. AIUI, DPDK doesn't actually create an interface (that would show up in ip link
), right? So, we shouldn't return it in Result. Instead, we should just return an empty result.
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.
ah! yes, good find. now changed it to return empty result for dpdk device.
@@ -91,6 +93,23 @@ func cmdAdd(args *skel.CmdArgs) error { | |||
} | |||
defer containerNs.Close() | |||
|
|||
var result *current.Result |
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.
There's no need to add this here; you initialize a new result on line 100 with the :=
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.
I've just moved this variable here from line no. 104 (in existing code), Now updated :=, thanks.
Also, please add a note to the plugin's README documenting this behavior. |
Yes, added now. |
/lgtm |
@dcbw is this good to merge now ? |
@dcbw Any update on this patch? Thanks Dan. |
This commit would make host-device plugin as a placeholder for DPDK device when applications wants to attach it with a pod container through network attachment definition. Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
@martinkennelly Thanks for following it up, just rebased the PR to resolve conflicts for README.md. Let me raise another PR separately for README.md changes in cni.dev repo. |
Guys, could you please merge this MR? This is mandatory use case for DPDK people with SRIOV environment in VMs. Thanks |
Hi @bboreham, thanks for you approval.
and everything works without any issue. Could you please let us know when your amazing Team will merge this MR in master branch? Thanks |
Hello Team, Thanks |
I would like @squeed to take one more look since he had a lot of comments earlier. |
It would be nice to have at least test case which would describe such use type with explanation why in a particular case this plugin should work basically as a no-op. |
Thanks for merging the fix. can you also merge its readme ? |
Is there any plan to support IPAM for DPDK interface in host-device cni? like k8snetworkplumbingwg/sriov-cni#142 |
This commit would make host-device plugin as a placeholder for DPDK device when applications wants to attach it with a pod container through network attachment definition. Signed-off-by: Periyasamy Palanisamy <[email protected]>
This commit would make host-device plugin as a placeholder
for DPDK device when applications wants to attach it with
a pod container through network attachment definition.
Signed-off-by: Periyasamy Palanisamy [email protected]