PCI address based filtering for accelerator device type is not supported

[svallala]

Filtering based on PCI addresses is missing

[zshi-redhat]

@svallala pciAddresses selector was added in favor of using sriov in VM environmnet, what is the use case for accelerator?

[svallala]

We would like to group the FPGA resources based on NUMA topology and since NUMA node is not part of the selectors the only way to achieve this via PCI Addresses. The documentation states that standard selectors are supported for accelerators and PCI Addresses is part of the standard selectors.

[zshi-redhat]

We would like to group the FPGA resources based on NUMA topology and since NUMA node is not part of the selectors the only way to achieve this via PCI Addresses. The documentation states that standard selectors are supported for accelerators and PCI Addresses is part of the standard selectors.

Would k8s topology manager be helpful here if devices are not grouped based on NUMA?

[svallala]

We don't want to rely on Topology Manager, since we have advanced planning algorithm. This seems to be a genuine miss because the documentation explicitly mentions common selectors being supported for the accelerator device type.

The "accelerator" device type currently supports only the common selectors.

[svallala]

The use case could also be, to create resource grouping where a customer would like to have a few VFs reserved for a tenant. If for some reason this pull request is not approved, then at least the documentation should be fixed to indicate that PCI address based filtering is not supported for "accelerator" device types. I didn't suspect a plugin issue and had to spend a lot of time debugging our platform before I realized it's the plugin issue. Correct documentation will save others time. Thanks

[ahalimx86]

I think the pciAddress filter is applicable to the accelerators as well and not having that implemented for accelerator was genuine miss IMO. We definitely should consider this PR. Thanks for @svallala for flagging this and submitting the solution.

[adrianchiris]

Straightforward change

As stated in : https://github.com/k8snetworkplumbingwg/sriov-network-device-plugin#common-selectors

pciAddresses selector is common

we should really have a common method to filter devices according to defined common filters, (thought for improvement... not related to this PR.)

overall looks good, it would be great if you could extend accelDeviceProvider_test.go to include filtering with this selector.

sriov-network-device-plugin/pkg/accelerator/accelDeviceProvider_test.go

Line 126 in fb9fa79

Describe("getting Filtered devices", func() {

[zshi-redhat]

@ahalim-intel @adrianchiris if you can give an approve, I will merge it.

[ahalimx86]

/lgtm
echoing @adrianchiris comment above about extending the test.

[svallala]

A completely different test is failing, locally if I reran the tests everything passed, so I assumed it is some transient failure that I could ignore. Can someone help me with this failure?

[svallala]

@zshi-redhat running the tests locally is working fine. Can you please help me with this error? Thanks

[zshi-redhat]

@zshi-redhat running the tests locally is working fine. Can you please help me with this error? Thanks

@svallala Martin is attempting to fix the ghw test error at: #312, once that's done, I will re-run the test in this PR.

[svallala]

Thanks @zshi-redhat

[zshi-redhat]

@svallala could you rebase against latest master and see if CI passes? There were two CI issues fixed last week.

[svallala]

@zshi-redhat I rebased and the scripts have passed now.

[zshi-redhat]

@zshi-redhat I rebased and the scripts have passed now.

thanks!

[zshi-redhat]

Merge it since we already have 3 approves and the latest change is just rebase against master (for including CI fixs).

[svallala]

thanks @zshi-redhat