Support VFIO passthrough

[varunrsekar]

Design

1. Introduce new featuregate PassthroughSupport
2. Discover GPUs using github.com/NVIDIA/go-nvlib/nvpci pkg
3. Publish them as vfio devices in the node resource slice of type vfio with canonicalName gpu-vfio-<idx>
4. Create a new deviceclass vfio.gpu.nvidia.com
5. Create a new set of CDI devices in a new file k8s.gpu.nvidia.com-device_vfio.yaml:

k8s.gpu.nvidia.com/device=gpu-vfio-0
k8s.gpu.nvidia.com/device=gpu-vfio-1

6. A successful NodePrepare will cause the device of type gpu for the same PCIBusID to be removed from the resourceslice.
7. A successful NodeUnprepare will cause the device of type gpu for the same PCIBusID to be rediscovered and added back to the resourceslice

GPU Discovery

1. Invoke nvpci.GetGPUs (github.com/NVIDIA/go-nvlib/nvpci)
2. Publish all discovered GPUs as devices of type vfio in the resourceslice
3. Set the parent of the vfio device to the device of type gpu discovered from NVML (is nil if device is not bound to the nvidia driver)
4. Publish new CDI file (k8s.gpu.nvidia.com-device_vfio.yaml) with the discovered vfio devices

NodePrepare

1. Verify there are no active gpu clients on the device by checking for open FDs against <driver-root>/dev/nvidia<minor> (timeout after 60s)
2. Verify there are no VFs on the GPU
3. Configure the GPU for passthrough using introduced scripts/unbind_from_driver.sh, scripts/bind_to_driver.sh shell scripts.
4. Remove the parent GPU device of type gpu in the resourceslice

NodeUnprepare

1. Configure the GPU back to the nvidia driver using introduced scripts/unbind_from_driver.sh, scripts/bind_to_driver.sh shell scripts.
2. Rediscover the parent GPU device (as its device minor might've changed) and re-add it back to the resourceslice

Deployment

1. kubeletplugin is a privileged pod running as root
2. Mount host path / (read-only), /sys/ (read-write) and /proc/ (read-write)

Testing

1. Verified with Kubevirt VM using sample resourceclaimtemplate spec from demo/specs/quickstart/gpu-test-vfiopci.yaml
2. Regression tests with the specs under https://github.com/NVIDIA/k8s-dra-driver-gpu/tree/main/demo/specs/quickstart/:
   1. With FG enabled: PASS
   2. With FG disabled: PASS

TODO

1. Use standard attribute name for PCI Bus ID
2. Use nvidia-container-toolkit library to generate the CDI device file (PENDING Add vfio mode to generate CDI specs for NVIDIA passthrough GPUs nvidia-container-toolkit#315)
3. active GPU VF enablement/disablement
4. Verify there are no VGPU/MIG before advertising VFIO device.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[varunrsekar]

Updates:

• Use chroot to use modprobe from nvidiaDriverRoot
• During NodePrepare/NodeUnprepare, sync advertised resources to prevent different device types from being allocatable for the same GPU
• Add soft-check for VFs to be disabled before attempting driver unbind

[shivamerla]

LGTM! thanks @varunrsekar for being patient with this change. We have to follow up on some of the blockers as below when we move this feature out of alpha.

• Need a robust way to evict all GPU clients (as pods or systemd services) before performing unbind from the nvidia driver.
• Need to identify ways to restart nvidia-persistenced without any side-effects to other GPU workloads.
• Need to handle potential blocking unbind calls in the kernel if the device is busy.
• Need to ensure we don't setup health monitoring for devices bound to vfio-pci usign nvml event watcher.

cc @klueska to further review this feature to support behind an alpha feature-gate.

[varunrsekar]

Updates:

• Squashed all previous commits
• If MIGs are present, then VFIO devices wouldn't be advertised.
• Updated regression testing in description

[shivamerla]

LGTM.

[shivamerla]

merging this as discussed with @klueska

[jgehrcke]

#724

merging this as discussed with @klueska

We have a test suite now. It's a pity that we don't have it yield automatic pre-merge signal yet (and we have to push harder towards getting there).

In the absence of that, we should at least remind ourselves that significant patches like this should be tested with that test suite before merging. (with the result of the test run reported on the PR)

[jgehrcke]

I wondered why we don't do this here, maybe this would have deserved a code comment..

Now I see. You do this elsewhere, and here we would duplicate that effort.

[varunrsekar]

I’ll be polishing the feature in some follow-up PRs. I’ll remember to add a comment. Pls continue to post any reviews!

[jgehrcke]

Why are we doing this here, as the last step of unprepare?

[jgehrcke]

Why are we (re)publishing resources as part of preparing a device?

[jgehrcke]

If this has to be done to reflect that something has changed, we should do this only when something (the set of allocatable devices) actually has changed (is that what's happening)?

[varunrsekar]

That's exactly it. I've outlined the steps in the PR description.

We've both a gpu device and a vfio device corresponding to the same physical GPU. So when we driver-switch and reconfigure the GPU for the vfio type, we dont want the scheduler to allocate the gpu type (and vice versa). Granted that the scheduler can still race between the time it allocated a device and when NodePrepare actually completed for it and republished the new state. But this may be an acceptable limitation until we get PartitionableDevices support. Once we have that, I'll rework this.

(Btw we currently dont do this when we have MIGs enabled. We publish both the full gpu and the migs even though the full gpu is never usable when migs are statically configured on the node)

[klueska]

Having to republish the resource slice is something we should strive to never be doing. The publishing of the full GPU beside the MIG device is a bug / regression introduces recently. I understand the need to do the republishing for now under the alpha feature gate for vfio devices before the partitinable devices feature is GAd upstream, but it should not be on the default code path -- it should be behind the feature gate just like all other code specific to the vfio device implementation.

[jgehrcke]

1. We had so far tried to not create a top-level scripts directory, but instead put shell scripts into /hack. I am not too opinionated, just saying.

2. I like to make shell scripts validate against shellcheck.

3. I like to use the options errexit, nounset and pipefail

[varunrsekar]

I plan to rework this into go code in a follow-up. So I guess that will eliminate your concerns here

[cdesiniotis]

@varunrsekar we already maintain a very similar utility that binds / unbinds GPUs to the vfio module. I recently ported this script to Go in NVIDIA/k8s-driver-manager#127 and then added support for GB200 in NVIDIA/k8s-driver-manager#128 (as virtualization on Grace requires a different VFIO module, and so will Rubin). Can you take a look and see if / how you can re-use it?

[varunrsekar]

@cdesiniotis Thanks. the library is very close to what we need here. Should this be migrated to NVIDIA/go-nvlib so that it can be consumed in both places?

[jgehrcke]

I would appreciate a bit of documentation here describing the purpose / relevance of the script, and explaining the context.

Just a few-word summary you would give to someone who doesn't have the context that you have.

[jgehrcke]

Thanks for these statements in the PR description:

A successful NodePrepare will cause the device of type gpu for the same PCIBusID to be removed from the resourceslice.

A successful NodeUnprepare will cause the device of type gpu for the same PCIBusID to be rediscovered and added back to the resourceslice

We should perform these resource slice mutations (explicitly) only conditionally when

1. a vfio dev was actually prepared or unprepared
2. the featuregate is enabled

Also, there may be an interesting race condition here -- what if the re-publishing is 'too slow', and a prepare() request is coming in asking for the now-occupied device? What's the current behavior in that case?

---

Not asking for a very quick change, but just wanted to note that down somewhere. For now, I'd like to reconcile this PR with my work on dynamic MIG device management (as you all were working on this PR, I had been working on the dynamic MIG device management branch and of course I have been iterating on the very same regions of code, resulting in a rather monstrous merge conflict :-)). I would appreciate if we can pause iterating on this feature for a bit until I got an opportunity to reconcile and land a dynamic MIG device management patch.

[varunrsekar]

We should perform these resource slice mutations (explicitly) only conditionally when a vfio dev was actually prepared or unprepared
the featuregate is enabled

quick comment on this: if the allocatables is not changed, then the Publish operation is a no-op. I remember digging through the helper code and seeing this. Will need to reverify.

But it makes sense for this to be feature-gated. I missed doing it. I’ll prepare a fix for that.

(Once your partitionable devices code is in, I’ll incorporate that work and we’ll be able to get rid of this)

[varunrsekar]

Also, there may be an interesting race condition here -- what if the re-publishing is 'too slow', and a prepare() request is coming in asking for the now-occupied device? What's the current behavior in that case?

Yes this is a known race. In terms of the behavior, I haven’t documented it yet. Should be fairly easy to repro, I’ll share some findings here later.

But the hope is that we move to Partitionable devices soon and rework this.

[klueska]

This should not be here. This should be done outside the call to this function. With the goal being that it is very obvious to someone reading the main flow of the code that all code paths for this feature are feature gated. They shouldn't have to look into this component itself to discover that.

[klueska]

Having to republish the resource slice is something we should strive to never be doing. The publishing of the full GPU beside the MIG device is a bug / regression introduces recently. I understand the need to do the republishing for now under the alpha feature gate for vfio devices before the partitinable devices feature is GAd upstream, but it should not be on the default code path -- it should be behind the feature gate just like all other code specific to the vfio device implementation.

[klueska]

Can we drop this?

[klueska]

Can you move this back to the top.

[klueska]

Can we group the functions together that are only here because we dont yet support this feature via PartitionableDevices? Then have a comment before that block saying we can remove these once we support that.

[klueska]

Can we pslit this into two separate calls so that the creation mimics what we do for th eother managers above exactly. And then only do the precheck call on the object if it is non-nil. We should also consider renaming Prechecks to something else. It feels as if it could be more descriptive.

[klueska]

Can we put allocatable back in the struct?

[klueska]

Can we wrap this error in a fmt.Errorf()

[klueska]

Can we just drop this?

[klueska]

Maybe we drop this for now and revisit this if/when we allow for parallel prepare / unprepare calls.