Add vDPA support #306

amorenoz · 2020-12-10T17:27:26Z

vDPA devices can be exposed to Pods in two different ways:
If they are bound to the virtio_vdpa driver, they are exposed as normal netdevs
If they are bound to the vhost_vdpa driver they are exposed as vhost-vdpa devices. These are char devices that support the vhost-vdpa extensions to the vhost uAPI

The implementation of the vDPA support is done by implementing the DeviceInfoProvider interface.
A new string field is added to the 'NetDeviceSelectors': vdpaType. It can optionally have two values: vhost or virtio. This selector is used to determine the way the devices are exposed to the Pods as well as to filter the discovered devices based on their vdpa driver.
For the low level vdpa bus handling, we depend on github.com/redhat-virtio-net/govdpa a golang library to interact with the vdpa subsystem.

This PR also adds support for writing the vdpa DeviceInfo file as per the specification.
Fixes: #305

amorenoz · 2020-12-11T08:00:15Z

Still in WIP state because I'd like to post a PR on the SRIOV-CNI with some code that is needed to test the E2E solution.
But reviews and comments can begin.

One specific design decision that I'm not 100% sure about the selector value, current state is:

{ ...
"vdpaType": "vhost"
or
"vdpaType": "virtio"
...}

We could use vdpaDriver: ["vhost_vdpa" , "virtio_vdpa"]. I finally went with the former in the hope of:

Avoid redundancy of the "_vdpa" part
Convey the message that this is not only a selector (i.e: a filter) based on the vDPA driver. It also modifies the way the device is exposed.

The not so good side of this is: we have to add some vdpaType to vdpaDriver boilerplate code here and there

WDYT?

amorenoz · 2020-12-14T16:34:25Z

Works in combination with k8snetworkplumbingwg/sriov-cni#163

mcoquelin

Only minor cosmetic comments, otherwise loks good to me.

Thanks,
Maxime

docs/vdpa/README.md

pkg/netdevice/vdpa.go

pkg/netdevice/netResourcePool.go

zshi-redhat

@amorenoz just to understand the scope, this vDPA support is for SR-IOV + vDPA, it will/can not be used for SF/ADI vDPA, correct?

pkg/factory/factory.go

docs/vdpa/README.md

amorenoz · 2021-01-07T15:43:23Z

@amorenoz just to understand the scope, this vDPA support is for SR-IOV + vDPA, it will/can not be used for SF/ADI vDPA, correct?

Correct, this PR implements SR-IOV + vDPA

SF/ADI is a different story: will it even be supported by DevicePlugin?

zshi-redhat · 2021-01-08T01:25:40Z

@amorenoz just to understand the scope, this vDPA support is for SR-IOV + vDPA, it will/can not be used for SF/ADI vDPA, correct?

Correct, this PR implements SR-IOV + vDPA

SF/ADI is a different story: will it even be supported by DevicePlugin?

Haven't discussed this, my understanding is perhaps not.

zshi-redhat · 2021-01-08T01:26:42Z

/cc @martinkennelly

amorenoz · 2021-01-08T14:32:59Z

That is one strange CI error. Has anyone seen this before?

Resource manager

/home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp/manager_test.go:48

  discovering devices

  /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/pkg/mod/github.com/onsi/ginkgo@v1.12.0/extensions/table/table.go:92

    no devices [It]

    /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/pkg/mod/github.com/onsi/ginkgo@v1.12.0/extensions/table/table_entry.go:43

    Unexpected error:

        <*errors.errorString | 0xc0004c5990>: {

            s: "discoverDevices(): error getting PCI info: gzip: invalid header",

        }

        discoverDevices(): error getting PCI info: gzip: invalid header

    occurred

    /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp/manager_test.go:315

------------------------------

zshi-redhat · 2021-01-12T10:17:40Z

That is one strange CI error. Has anyone seen this before?

Resource manager

/home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp/manager_test.go:48

  discovering devices

  /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/pkg/mod/github.com/onsi/ginkgo@v1.12.0/extensions/table/table.go:92

    no devices [It]

    /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/pkg/mod/github.com/onsi/ginkgo@v1.12.0/extensions/table/table_entry.go:43

    Unexpected error:

        <*errors.errorString | 0xc0004c5990>: {

            s: "discoverDevices(): error getting PCI info: gzip: invalid header",

        }

        discoverDevices(): error getting PCI info: gzip: invalid header

    occurred

    /home/travis/gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/.gopath/src/github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp/manager_test.go:315

------------------------------

I think @martinkennelly mentioned this in last resource mgmt meeting, he might be working on the fix (to update ghw lib).
@adrianchiris suspected this might be due to the VM image update in travis.

martinkennelly · 2021-01-12T11:09:05Z

Couldn't reproduce consistently locally so doesn't seem like ghw issue. I was hoping it was transient error so gave up. @adrianchiris suggested upgrading to bionic (18.04) VM.

Ill post a test PR with updated travis file to see if it fixes it.
Edit: Updating travis VM failed.

I looked at the Ghw code and it seems to fetch pci-ids file in gzip format from a network location if it cannot find pci ids file locally: https://github.com/jaypipes/pcidb/blob/98ef3ee36c69f20650fe7855047ad042338c6983/discover.go#L36 referenced by const PCIIDS_URI. If this fails, you'd see the error shown by @amorenoz

adrianchiris

some comments on the first two commits

docs/vdpa/README.md

pkg/netdevice/vdpa.go

pkg/types/types.go

pkg/netdevice/pciNetDevice.go

docs/vdpa/README.md

jaypipes · 2021-01-22T14:50:06Z

Couldn't reproduce consistently locally so doesn't seem like ghw issue. I was hoping it was transient error so gave up. @adrianchiris suggested upgrading to bionic (18.04) VM.

Ill post a test PR with updated travis file to see if it fixes it.
Edit: Updating travis VM failed.

I looked at the Ghw code and it seems to fetch pci-ids file in gzip format from a network location if it cannot find pci ids file locally: https://github.com/jaypipes/pcidb/blob/98ef3ee36c69f20650fe7855047ad042338c6983/discover.go#L36 referenced by const PCIIDS_URI. If this fails, you'd see the error shown by @amorenoz

Hi folks! Hey, please check out this PR from @pearsonk in the pcidb library that addresses systems that store the PCIIDS file in gzip-format:

jaypipes/pcidb#23

I plan to merge that today along with some other PRs from @martinkennelly and cut a new release for pcidb and the upstream ghw project today.

Best,
-jay

jaypipes · 2021-01-22T14:51:37Z

/cc @fromanirh you might be interested in this repo/PR as well

pkg/netdevice/vdpa.go

README.md

pkg/netdevice/netResourcePool.go

pkg/netdevice/pciNetDevice.go

pkg/netdevice/vdpa.go

docs/vdpa/README.md

pkg/netdevice/netResourcePool.go

pkg/netdevice/pciNetDevice_test.go

pkg/netdevice/vdpa.go

amorenoz · 2021-12-23T14:02:05Z

Updated to pull govdpa from NPWG organization

docs/vdpa/README.md

amorenoz · 2022-01-10T12:31:26Z

@adrianchiris , do you have any further comments on this PR?

Billy99

/lgtm
Few minor things in the documentation.

docs/vdpa/README.md

docs/README.md

amorenoz · 2022-01-24T15:53:48Z

Thanks @Billy99

amorenoz · 2022-01-25T10:26:40Z

@adrianchiris if you want to try this PR, I've tested it with linux v5.14.

adrianchiris · 2022-01-25T13:17:17Z

@amorenoz planning to look at it today, sorry for the delay

adrianchiris

Thanks for working on it Adrian !

overall i think it looks good, added some comments

pkg/types/types.go

pkg/netdevice/vdpa.go

pkg/netdevice/pciNetDevice.go

pkg/netdevice/netDeviceProvider.go

pkg/netdevice/netResourcePool.go

amorenoz · 2022-01-28T16:31:22Z

@adrianchiris at your convenience, PTAL

adrianchiris

a couple of nits but otherwise LGTM.

Thank you for your patience Adrian !

adrianchiris · 2022-02-13T15:56:28Z

pkg/types/types.go

+
+// VdpaDevice is an interface to access vDPA device information
+type VdpaDevice interface {
+	GetDriver() string


GetDriver() is currently not being used by sriov-network-device-plugin. i think we can remove it for now WDYT ?

when filling device information i see that the VDPA type is used as driver:

in netResourcePool.go L#88
Driver: string(vdpaDev.GetType()),

Right, I'll remove it from the interface

adrianchiris · 2022-02-13T16:03:58Z

pkg/types/types.go

@@ -159,6 +171,8 @@ type DeviceProvider interface {
 	GetDevices(*ResourceConfig) []PciDevice

 	GetFilteredDevices([]PciDevice, *ResourceConfig) ([]PciDevice, error)
+
+	ValidConfig(*ResourceConfig) bool


nit: could you add a docstring to this interface method ?

was not addressed, however will not block on it. it can be addressed as followup.

Sorry, I added it to the implementations in netDeviceProvider and accelDeviceProvider

Now added it to the interface method

Low level vdpa management is performed on github.com/k8snetworkplumbingwg/govdpa Two new types are introduced: - VdpaDevice is an interface to expose vDPA device information. - VdpaType is used to define the types of supported vdpa devices to use It can have two values: "vhost" or "virtio". Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

The vdpaInfoProvider determines device information (as required by the API) from a vdpa device. Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

If a vdpaType is given, select the appropriate infoProvider Also, filter devices based on the vdpaType configured Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

adrianchiris

LGTM !

Add a config validation function to the DeviceProvider API so each one of them can do DeviceType-specific config validation. Specifically, the netDevice config validation checks that both IsRdma and VdpaType are not configured simultaneously. Doing this at the manager level makes the rest of the code cleaner. Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

adrianchiris · 2022-03-01T06:38:02Z

@zshi-redhat @SchSeba PTAL

Its been a long ride with this PR :)

zshi-redhat · 2022-03-01T13:04:41Z

/cc @bn222

amorenoz · 2022-03-09T06:38:05Z

@zshi-redhat do you have any further comments on this PR?

amorenoz mentioned this pull request Dec 14, 2020

Treat vhost-vdpa devices as DPDK devices k8snetworkplumbingwg/sriov-cni#163

Merged

mcoquelin reviewed Dec 22, 2020

View reviewed changes

docs/vdpa/README.md Outdated Show resolved Hide resolved

pkg/netdevice/vdpa.go Outdated Show resolved Hide resolved

adrianchiris reviewed Jan 3, 2021

View reviewed changes

pkg/netdevice/netResourcePool.go Outdated Show resolved Hide resolved

zshi-redhat reviewed Jan 4, 2021

View reviewed changes

pkg/factory/factory.go Outdated Show resolved Hide resolved

docs/vdpa/README.md Outdated Show resolved Hide resolved

docs/vdpa/README.md Outdated Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch from 069d2ac to 581c03c Compare January 7, 2021 14:54

amorenoz force-pushed the vdpaInfoProvider branch from 581c03c to 5a58d13 Compare January 12, 2021 08:48

amorenoz changed the title ~~WIP: Add vDPA support~~ Add vDPA support Jan 12, 2021

adrianchiris requested changes Jan 21, 2021

View reviewed changes

adrianchiris reviewed Jan 21, 2021

View reviewed changes

docs/vdpa/README.md Show resolved Hide resolved

martinkennelly mentioned this pull request Jan 22, 2021

Fix fetching of PCI IDs file jaypipes/pcidb#27

Merged

adrianchiris reviewed Jan 31, 2021

View reviewed changes

pkg/netdevice/vdpa.go Outdated Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch from 5a58d13 to d2889e2 Compare February 1, 2021 17:17

ipatrykx reviewed Jul 2, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

pkg/netdevice/netResourcePool.go Outdated Show resolved Hide resolved

pkg/netdevice/pciNetDevice.go Show resolved Hide resolved

pkg/netdevice/vdpa.go Show resolved Hide resolved

pkg/netdevice/vdpa.go Outdated Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch 2 times, most recently from d287384 to b17bc28 Compare August 19, 2021 15:44

zshi-redhat reviewed Aug 23, 2021

View reviewed changes

docs/vdpa/README.md Outdated Show resolved Hide resolved

pkg/netdevice/netResourcePool.go Outdated Show resolved Hide resolved

pkg/netdevice/pciNetDevice_test.go Outdated Show resolved Hide resolved

pkg/netdevice/vdpa.go Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch from b17bc28 to 8b1c583 Compare September 15, 2021 12:47

amorenoz force-pushed the vdpaInfoProvider branch from 8b1c583 to f3681b5 Compare October 29, 2021 10:18

amorenoz mentioned this pull request Nov 18, 2021

Add support to virtio-vDPA devices ovn-org/ovn-kubernetes#2664

Merged

amorenoz force-pushed the vdpaInfoProvider branch from f3681b5 to bf92414 Compare December 16, 2021 09:46

amorenoz force-pushed the vdpaInfoProvider branch from 7085a49 to 87bf902 Compare December 23, 2021 14:01

SchSeba reviewed Jan 6, 2022

View reviewed changes

docs/vdpa/README.md Outdated Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch from 87bf902 to 5a1ce79 Compare January 10, 2022 12:34

Billy99 reviewed Jan 21, 2022

View reviewed changes

docs/vdpa/README.md Outdated Show resolved Hide resolved

docs/vdpa/README.md Outdated Show resolved Hide resolved

docs/vdpa/README.md Outdated Show resolved Hide resolved

docs/vdpa/README.md Outdated Show resolved Hide resolved

docs/README.md Outdated Show resolved Hide resolved

amorenoz force-pushed the vdpaInfoProvider branch from 5a1ce79 to e86c05f Compare January 24, 2022 15:53

adrianchiris reviewed Jan 25, 2022

View reviewed changes

amorenoz force-pushed the vdpaInfoProvider branch from e86c05f to e48b4e4 Compare January 27, 2022 17:53

adrianchiris reviewed Feb 13, 2022

View reviewed changes

amorenoz added 3 commits February 17, 2022 17:24

pciNetDevice: introduce vdpaInfoProvider

7a19de3

The vdpaInfoProvider determines device information (as required by the API) from a vdpa device. Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

pciNetDevice: introduce vdpaType selector

6eadfe8

If a vdpaType is given, select the appropriate infoProvider Also, filter devices based on the vdpaType configured Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

amorenoz force-pushed the vdpaInfoProvider branch from e48b4e4 to 4219f1c Compare February 17, 2022 16:27

adrianchiris approved these changes Feb 28, 2022

View reviewed changes

amorenoz added 4 commits March 1, 2022 07:15

pciNetDevice: add vdpa unit tests

a2e677b

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

netResourcePool: add vdpa to Device Info file

bdb4720

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

docs: document vdpa device type

d51e6a1

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

amorenoz force-pushed the vdpaInfoProvider branch from 4219f1c to d51e6a1 Compare March 1, 2022 06:16

adrianchiris mentioned this pull request Mar 1, 2022

Devlink Error message #413

Merged

zshi-redhat approved these changes Mar 9, 2022

View reviewed changes

zshi-redhat merged commit 6b69239 into k8snetworkplumbingwg:master Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vDPA support #306

Add vDPA support #306

amorenoz commented Dec 10, 2020 •

edited

Loading

amorenoz commented Dec 11, 2020

amorenoz commented Dec 14, 2020

mcoquelin left a comment

zshi-redhat left a comment

amorenoz commented Jan 7, 2021

zshi-redhat commented Jan 8, 2021

zshi-redhat commented Jan 8, 2021

amorenoz commented Jan 8, 2021 •

edited

Loading

zshi-redhat commented Jan 12, 2021

martinkennelly commented Jan 12, 2021 •

edited

Loading

adrianchiris left a comment

jaypipes commented Jan 22, 2021

jaypipes commented Jan 22, 2021

amorenoz commented Dec 23, 2021

amorenoz commented Jan 10, 2022

Billy99 left a comment

amorenoz commented Jan 24, 2022

amorenoz commented Jan 25, 2022

adrianchiris commented Jan 25, 2022

adrianchiris left a comment

amorenoz commented Jan 28, 2022

adrianchiris left a comment

adrianchiris Feb 13, 2022

amorenoz Feb 17, 2022

adrianchiris Feb 13, 2022

adrianchiris Feb 28, 2022

amorenoz Mar 1, 2022

amorenoz Mar 1, 2022

adrianchiris left a comment

adrianchiris commented Mar 1, 2022

zshi-redhat commented Mar 1, 2022

amorenoz commented Mar 9, 2022

Add vDPA support #306

Add vDPA support #306

Conversation

amorenoz commented Dec 10, 2020 • edited Loading

amorenoz commented Dec 11, 2020

amorenoz commented Dec 14, 2020

mcoquelin left a comment

Choose a reason for hiding this comment

zshi-redhat left a comment

Choose a reason for hiding this comment

amorenoz commented Jan 7, 2021

zshi-redhat commented Jan 8, 2021

zshi-redhat commented Jan 8, 2021

amorenoz commented Jan 8, 2021 • edited Loading

zshi-redhat commented Jan 12, 2021

martinkennelly commented Jan 12, 2021 • edited Loading

adrianchiris left a comment

Choose a reason for hiding this comment

jaypipes commented Jan 22, 2021

jaypipes commented Jan 22, 2021

amorenoz commented Dec 23, 2021

amorenoz commented Jan 10, 2022

Billy99 left a comment

Choose a reason for hiding this comment

amorenoz commented Jan 24, 2022

amorenoz commented Jan 25, 2022

adrianchiris commented Jan 25, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

amorenoz commented Jan 28, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris Feb 13, 2022

Choose a reason for hiding this comment

amorenoz Feb 17, 2022

Choose a reason for hiding this comment

adrianchiris Feb 13, 2022

Choose a reason for hiding this comment

adrianchiris Feb 28, 2022

Choose a reason for hiding this comment

amorenoz Mar 1, 2022

Choose a reason for hiding this comment

amorenoz Mar 1, 2022

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris commented Mar 1, 2022

zshi-redhat commented Mar 1, 2022

amorenoz commented Mar 9, 2022

amorenoz commented Dec 10, 2020 •

edited

Loading

amorenoz commented Jan 8, 2021 •

edited

Loading

martinkennelly commented Jan 12, 2021 •

edited

Loading