ON-15256: Report topology info from device plugin

[tcrawley-xilinx]

[1/2] ON-15180: Read sysfs to check for NICs

Rather than rely on an external tool (lshw) which requires extra
dependencies in container images and doesn't provide an simple way of
determining the vendor of a nic, it would be better to search through
sysfs directly (since that is basically all lshw is doing). This
approach also allows us to easily get other information from sysfs (such
as numa nodes).

[2/2] ON-15256: Report topology info from device plugin

The device plugin will now try to read the numa information from sysfs
while finding the nics present in the system. The device plugin will
then add all of the present numa node information to the list of
advertised devices. Because the device plugin is only advertising a
pseudo-device it is unclear how to proceed when multiple nics are
present on different nodes.

Testing done

Some manual testing in the cluster, no unit tests (yet) trying to think of a way of mocking sysfs files.

[pcolledg-amd]

Good to see this exposing topology via the official Device Plugin API and querying sysfs directly rather than lshw. Embedding sfc assumptions into Onload won't make life simpler when implementing non-sfc features, but changing the data source can be done later, the output is more important.

Consider using ghw go library to avoid the mass of filesystem handling boilerplate -- it supports PCIe vendors, network devices, and topology.

[krishd-amd]

Would it be worth using isOnloadableNic or isSupportedNic?

Just in case we are asked to support another vendor sometime in the future.

[tcrawley-xilinx]

Consider using ghw go library to avoid the mass of filesystem handling boilerplate -- it supports PCIe vendors, network devices, and topology.

I had a look into it, and it would work if we just cared about netdevices, but I don't think it will work for our purposes due to how it deals with pci devices. Rather than reading the files as I have done in this change, it wants to use a pci database (either from the file system or through the network).
When testing this is in the cluster it would fail to enumerate the pci devices, complaining about the lack of the pci database.

[ivatet-amd]

Looks good, thank you!

I would also suggest filing a Jira ticket for the queryNics() unit test coverage. For instance, we could create a mock sysfs tree in the repo and feed it to the tests by replacing sysClassNetPath (or similarly, whatever suits better).

[tcrawley-xilinx]

I would also suggest filing a Jira ticket for the queryNics() unit test coverage.

ON-15228?

For instance, we could create a mock sysfs tree in the repo and feed it to the tests by replacing sysClassNetPath (or similarly, whatever suits better).

I do want to increase test coverage for parts of the code that interact with the filesystem (this, mounts.go and maybe some bits in onload-worker). Another idea I had was to create a wrapper interface around common filesystem calls (read, write, stat etc) then provide a mocked implementation so that tests don't have to interact with the real filesystem.

[ivatet-amd]

Awesome! ON-15228 seems like the perfect fit. I bet you're already typing a comment over there with suggestions to improve the test coverage if anyone else wants to take a look at it unless you're planning to implement it in this PR (I assumed not, given it's ready for review)?!

[pcolledg-amd]

... When testing this is in the cluster it would fail to enumerate the pci devices, complaining about the lack of the pci database.

Thanks for taking a look. The missing PCI database is a logical consequence of a minimal container base image and easily surmountable. Just adding RUN microdnf install -y ... hwdata to deviceplugin.Dockerfile should fix that. cf. go pcidb.

[tcrawley-xilinx]

The missing PCI database is a logical consequence of a minimal container base image and easily surmountable.

I'm not saying that it is an insurmountable problem, but I'm not really convinced whether it is worth it. If ghw would just read the files in sysfs for pci information and make the db lookups optional then I would be more happy with it.
Also, we would still have to consider how to unit test these functions. Which means if we were to use ghw then we would be forced to write our tests to conform with what ghw (and any of its dependents) are expecting.

[pcolledg-amd]

If we're not using a package manager we can go distroless. Does ubi-micro suffice?

[tcrawley-xilinx]

Yes it does! I did some testing initially using scratch as the base image, but that would fail with the initContainer/lifecycle commands. I had forgotten about ubi-micro 🤦‍♂️